Re: [PATCH v2 18/25] arm/altp2m: Add HVMOP_altp2m_set_mem_access.

[Julien Grall <julien.grall@arm.com>]

Hello Sergej,

On 01/08/16 18:10, Sergej Proskurin wrote:
>  int p2m_alloc_table(struct p2m_domain *p2m)
>  {
>      unsigned int i;
> @@ -1920,7 +1948,7 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr,
>                          uint32_t start, uint32_t mask, xenmem_access_t access,
>                          unsigned int altp2m_idx)
>  {
> -    struct p2m_domain *p2m = p2m_get_hostp2m(d);
> +    struct p2m_domain *hp2m = p2m_get_hostp2m(d), *ap2m = NULL;
>      p2m_access_t a;
>      long rc = 0;
>
> @@ -1939,33 +1967,60 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr,
>  #undef ACCESS
>      };
>
> +    /* altp2m view 0 is treated as the hostp2m */
> +    if ( altp2m_idx )
> +    {
> +        if ( altp2m_idx >= MAX_ALTP2M ||
> +             d->arch.altp2m_vttbr[altp2m_idx] == INVALID_VTTBR )
> +            return -EINVAL;
> +
> +        ap2m = d->arch.altp2m_p2m[altp2m_idx];
> +    }
> +
>      switch ( access )
>      {
>      case 0 ... ARRAY_SIZE(memaccess) - 1:
>          a = memaccess[access];
>          break;
>      case XENMEM_access_default:
> -        a = p2m->default_access;
> +        a = hp2m->default_access;

Why the default_access is set the host p2m and not the alt p2m?

>          break;
>      default:
>          return -EINVAL;
>      }
>

[...]

> diff --git a/xen/include/asm-arm/altp2m.h b/xen/include/asm-arm/altp2m.h
> index a6496b7..dc41f93 100644
> --- a/xen/include/asm-arm/altp2m.h
> +++ b/xen/include/asm-arm/altp2m.h
> @@ -71,4 +71,14 @@ void altp2m_flush(struct domain *d);
>  int altp2m_destroy_by_id(struct domain *d,
>                           unsigned int idx);
>
> +/* Set memory access attributes of the gfn in the altp2m view. If the altp2m
> + * view does not contain the particular entry, copy it first from the hostp2m.
> + *
> + * Currently supports memory attribute adoptions of only one (4K) page. */

Coding style:

/*
  *  Foo
  *  Bar
  */

> +int altp2m_set_mem_access(struct domain *d,
> +                          struct p2m_domain *hp2m,
> +                          struct p2m_domain *ap2m,
> +                          p2m_access_t a,
> +                          gfn_t gfn);
> +
>  #endif /* __ASM_ARM_ALTP2M_H */
> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
> index 32326cb..9859ad1 100644
> --- a/xen/include/asm-arm/p2m.h
> +++ b/xen/include/asm-arm/p2m.h
> @@ -180,6 +180,17 @@ void p2m_dump_info(struct domain *d);
>  /* Look up the MFN corresponding to a domain's GFN. */
>  mfn_t p2m_lookup(struct domain *d, gfn_t gfn, p2m_type_t *t);
>
> +/* Lookup the MFN, memory attributes, and page table level corresponding to a
> + * domain's GFN. */
> +mfn_t p2m_lookup_attr(struct p2m_domain *p2m, gfn_t gfn, p2m_type_t *t,
> +                      unsigned int *level, unsigned int *mattr,
> +                      xenmem_access_t *xma);

I don't want to see a such interface expose outside of p2m. The outside 
world may not know what means the level. And I don't understand why you 
return "mattr" here.

> +
> +/* Modify an altp2m view's entry or its attributes. */
> +int modify_altp2m_entry(struct domain *d, struct p2m_domain *p2m,
> +                        paddr_t gpa, paddr_t maddr, unsigned int level,
> +                        p2m_type_t t, p2m_access_t a);
> +
>  /* Clean & invalidate caches corresponding to a region of guest address space */
>  int p2m_cache_flush(struct domain *d, gfn_t start, unsigned long nr);
>
> @@ -303,6 +314,16 @@ static inline int get_page_and_type(struct page_info *page,
>  /* get host p2m table */
>  #define p2m_get_hostp2m(d) (&(d)->arch.p2m)
>
> +static inline bool_t p2m_is_hostp2m(const struct p2m_domain *p2m)
> +{
> +    return p2m->p2m_class == p2m_host;
> +}
> +
> +static inline bool_t p2m_is_altp2m(const struct p2m_domain *p2m)
> +{
> +    return p2m->p2m_class == p2m_alternate;
> +}
> +

Why those helpers are only added here? They should be at the same place 
you define the p2m_class.

>  /* vm_event and mem_access are supported on any ARM guest */
>  static inline bool_t p2m_mem_access_sanity_check(struct domain *d)
>  {
>

Regards,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel