Re: [Xen-devel] [PATCH 2/2] altp2m: Implement p2m_get_mem_access for altp2m views

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 27.01.16 at 21:06, <tlengyel@xxxxxxxxxxx> wrote:
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -1572,7 +1572,9 @@ void p2m_mem_access_emulate_check(struct vcpu *v,
>          bool_t violation = 1;
>          const struct vm_event_mem_access *data = &rsp->u.mem_access;
>  
> -        if ( p2m_get_mem_access(v->domain, _gfn(data->gfn), &access) == 0 )
> +        if ( p2m_get_mem_access(v->domain, _gfn(data->gfn),
> +                                altp2m_active(v->domain) ? 
> vcpu_altp2m(v).p2midx : 0,
> +                                &access) == 0 )

This looks to be a behavioral change beyond what title and
description say, and it's not clear whether that's actually the
behavior everyone wants.

> @@ -1918,9 +1920,10 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn, 
> uint32_t nr,
>   * Get access type for a gfn.
>   * If gfn == INVALID_GFN, gets the default access type.
>   */
> -int p2m_get_mem_access(struct domain *d, gfn_t gfn, xenmem_access_t *access)
> +int p2m_get_mem_access(struct domain *d, gfn_t gfn, unsigned long altp2m_idx,
> +                       xenmem_access_t *access)
>  {
> -    struct p2m_domain *p2m = p2m_get_hostp2m(d);
> +    struct p2m_domain *hp2m = p2m_get_hostp2m(d), *p2m = NULL;
>      p2m_type_t t;
>      p2m_access_t a;
>      mfn_t mfn;
> @@ -1943,10 +1946,22 @@ int p2m_get_mem_access(struct domain *d, gfn_t gfn, 
> xenmem_access_t *access)
>      /* If request to get default access. */
>      if ( gfn_x(gfn) == INVALID_GFN )
>      {
> -        *access = memaccess[p2m->default_access];
> +        *access = memaccess[hp2m->default_access];
>          return 0;
>      }

And following the above - why would this not use the altp2m's
default access?

> +    /* altp2m view 0 is treated as the hostp2m */
> +    if ( altp2m_idx )
> +    {
> +        if ( altp2m_idx >= MAX_ALTP2M ||
> +             d->arch.altp2m_eptp[altp2m_idx] == INVALID_MFN )
> +            return -EINVAL;
> +
> +        p2m = d->arch.altp2m_p2m[altp2m_idx];
> +    }
> +    else
> +        p2m = hp2m;

And I don't see why you need separate p2m and hp2m local
variables.

> --- a/xen/include/xen/p2m-common.h
> +++ b/xen/include/xen/p2m-common.h
> @@ -56,6 +56,7 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn, 
> uint32_t nr,
>   * Get access type for a gfn.
>   * If gfn == INVALID_GFN, gets the default access type.
>   */
> -int p2m_get_mem_access(struct domain *d, gfn_t gfn, xenmem_access_t *access);
> +int p2m_get_mem_access(struct domain *d, gfn_t gfn, unsigned long altp2m_idx,
> +                       xenmem_access_t *access);

Same question as for patch 1 regarding the "unsigned long" here.

Jan


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