Re: [Xen-devel] [PATCH V3 1/2] x86/altp2m: Add hypercall to set a range of sve bits

[Jan Beulich <jbeulich@xxxxxxxx>]

On 21.11.2019 16:02, Alexandru Stefan ISAILA wrote:
> Changes since V2:
>       - Add a new structure "xen_hvm_altp2m_suppress_ve_multi"
>       - Copy the gfn of the first error to the caller
>       - Revert xen_hvm_altp2m_suppress_ve
>       - Add a mechanism to save the first error.

And I guess you want to adjust the commit message to cover this
fact.

> @@ -4711,6 +4712,18 @@ static int do_altp2m_op(
>          }
>          break;
>  
> +    case HVMOP_altp2m_set_suppress_ve_multi:
> +        if ( a.u.suppress_ve_multi.pad1 || !a.u.suppress_ve_multi.pad2 )
> +            rc = -EINVAL;
> +        else
> +        {
> +            rc = p2m_set_suppress_ve_multi(d, &a.u.suppress_ve_multi);
> +
> +            if ( __copy_to_guest(arg, &a, 1) )
> +                rc = -EFAULT;

Do you really want to replace a possible prior error here?

> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -3059,6 +3059,66 @@ out:
>      return rc;
>  }
>  
> +/*
> + * Set/clear the #VE suppress bit for multiple pages.  Only available on VMX.
> + */
> +int p2m_set_suppress_ve_multi(struct domain *d,
> +                              struct xen_hvm_altp2m_suppress_ve_multi *sve)
> +{
> +    struct p2m_domain *host_p2m = p2m_get_hostp2m(d);
> +    struct p2m_domain *ap2m = NULL;
> +    struct p2m_domain *p2m;
> +    uint64_t start = sve->opaque ?: sve->first_gfn;
> +    int rc = 0;
> +
> +    if ( sve->view > 0 )
> +    {
> +        if ( sve->view >= MAX_ALTP2M ||
> +             d->arch.altp2m_eptp[sve->view] == mfn_x(INVALID_MFN) )
> +            return -EINVAL;
> +
> +        p2m = ap2m = d->arch.altp2m_p2m[sve->view];

These want array_index_nospec() or alike used (and the pre-existing
similar uses taken care of in a separate patch).

> +    }
> +    else
> +        p2m = host_p2m;

Each time I see yet another instance of this pattern appear, I
wonder why this is. Use (or not) of initializers should be
consistent at least within individual functions. I.e. either
you initialize both ap2m and p2m in their declaration, or you
do so for neither of them.

> +    p2m_lock(host_p2m);
> +
> +    if ( ap2m )
> +        p2m_lock(ap2m);
> +
> +

Please no two blank lines next to one another.

> +    while ( sve->last_gfn >= start )

There are no checks on ->last_gfn, ->first_gfn, or ->opaque.
At the very least a bogus ->opaque should result in an error.
I wonder though why you don't simply update ->first_gfn,
omitting the need for ->opaque. All this would need is a
comment in the public header clarifying that callers should
expect the values to change.

Furthermore I think it would be helpful to bail on entirely
out of range ->first_gfn. This being a 64-bit field, only
40 of the bits are actually usable from an architecture pov
(in reality it may be even less). Otherwise you potentially
invoke p2m_ept_set_entry() perhaps trillions of times just
for it to return -EINVAL from its first if().

> +    {
> +        p2m_access_t a;
> +        p2m_type_t t;
> +        mfn_t mfn;
> +
> +        if ( altp2m_get_effective_entry(p2m, _gfn(start), &mfn, &t, &a, 
> AP2MGET_query) )
> +            a = p2m->default_access;
> +
> +        if ( p2m->set_entry(p2m, _gfn(start), mfn, PAGE_ORDER_4K, t, a,
> +                            sve->suppress_ve) && !sve->first_error )
> +            sve->first_error = start; /* Save the gfn from of the first 
> error */

Drop either "from" or "of"?

> --- a/xen/include/public/hvm/hvm_op.h
> +++ b/xen/include/public/hvm/hvm_op.h
> @@ -46,6 +46,17 @@ struct xen_hvm_altp2m_suppress_ve {
>      uint64_t gfn;
>  };
>  
> +struct xen_hvm_altp2m_suppress_ve_multi {
> +    uint16_t view;
> +    uint8_t suppress_ve; /* Boolean type. */
> +    uint8_t pad1;
> +    uint32_t pad2;

Perhaps use this field to report the error code of the first
error encountered?

> +    uint64_t first_gfn;
> +    uint64_t last_gfn;
> +    uint64_t opaque;

Afaics there's a requirement that the caller put zero in here
for the initial invocation. This should be noted in a comment.

> +    uint64_t first_error; /* Gfn of the first error. */

Actually the same appears to apply to this one.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel