Re: [Xen-devel] [PATCH 2/3] x86/altp2m: Add a hvmop for setting the suppress #VE bit

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 18.05.17 at 17:07, <apop@xxxxxxxxxxxxxxx> wrote:
> --- a/xen/arch/x86/mm/mem_access.c
> +++ b/xen/arch/x86/mm/mem_access.c
> @@ -466,6 +466,54 @@ int p2m_get_mem_access(struct domain *d, gfn_t gfn, 
> xenmem_access_t *access)
>  }
>  
>  /*
> + * Set/clear the #VE suppress bit for a page.  Only available on VMX.
> + */
> +int p2m_set_suppress_ve(struct domain *d, gfn_t gfn, uint8_t suppress_ve,

suppress_ve presumably is meant to be boolean.

> +                        unsigned int altp2m_idx)
> +{
> +    struct p2m_domain *host_p2m = p2m_get_hostp2m(d);
> +    struct p2m_domain *ap2m = NULL;
> +    struct p2m_domain *p2m = NULL;

Pointless initializer.

> +    mfn_t mfn;
> +    p2m_access_t a;
> +    p2m_type_t t;
> +    unsigned long gfn_l;

Please avoid this local variable and use gfn_x() in the two places
you need to.

> +    int rc = 0;

Pointless initializer again.

> +
> +    if ( !cpu_has_vmx )
> +        return -EOPNOTSUPP;

Is this enough? Wouldn't it be better to signal the caller whenever
hardware (or even software) isn't going to honor the request?

> +    if ( altp2m_idx > 0 )
> +    {
> +        if ( altp2m_idx >= MAX_ALTP2M ||
> +                d->arch.altp2m_eptp[altp2m_idx] == mfn_x(INVALID_MFN) )

Indentation.

> +            return -EINVAL;
> +
> +        p2m = ap2m = d->arch.altp2m_p2m[altp2m_idx];
> +    }
> +    else
> +    {
> +        p2m = host_p2m;
> +    }

Unnecessary braces.

> +    p2m_lock(host_p2m);
> +    if ( ap2m )
> +        p2m_lock(ap2m);
> +
> +    gfn_l = gfn_x(gfn);
> +    mfn = p2m->get_entry(p2m, gfn_l, &t, &a, 0, NULL, NULL);
> +    if ( !mfn_valid(mfn) )
> +        return -ESRCH;
> +    rc = p2m->set_entry(p2m, gfn_l, mfn, PAGE_ORDER_4K, t, a,
> +                        suppress_ve);
> +    if ( ap2m )
> +        p2m_unlock(ap2m);
> +    p2m_unlock(host_p2m);

To fiddle with a single gfn, this looks to be very heavy locking.
While for now gfn_lock() is the same as p2m_lock(), from an
abstract perspective I'd expect gfn_lock() to suffice here at 
least in the non-altp2m case.

And then there are two general questions: Without a libxc layer
function, how is one supposed to use this new sub-op? Is it
really intended to permit a guest to call this for itself?

Jan


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