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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 08.01.2020 15:08, Alexandru Stefan ISAILA wrote:
> By default the sve bits are not set.
> This patch adds a new hypercall, xc_altp2m_set_supress_ve_multi(),
> to set a range of sve bits.
> The core function, p2m_set_suppress_ve_multi(), does not break in case
> of a error and it is doing a best effort for setting the bits in the
> given range. A check for continuation is made in order to have
> preemption on large ranges.
> The gfn of the first error is stored in
> xen_hvm_altp2m_suppress_ve_multi.first_error and the error code is
> stored in xen_hvm_altp2m_suppress_ve_multi.first_error_code.
> If no error occurred the values will be 0.

These last two sentences must have been stale for a while. IOW ...

> Changes since V6:
>       - Fix commit message

... has this really happened?

> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -3030,45 +3030,87 @@ out:
>   */
>  int p2m_set_suppress_ve(struct domain *d, gfn_t gfn, bool suppress_ve,
>                          unsigned int altp2m_idx)
> +{
> +    int rc;
> +    struct xen_hvm_altp2m_suppress_ve_multi sve = {0};
> +
> +    sve.view = altp2m_idx;
> +    sve.suppress_ve = suppress_ve;
> +    sve.first_gfn = gfn_x(gfn);
> +    sve.last_gfn = gfn_x(gfn);

Can't all of these move into the initializer, instead of the
somewhat odd 0?

> +    if ( !(rc = p2m_set_suppress_ve_multi(d, &sve)) && sve.first_error )
> +        rc = sve.first_error;

Why the right side of the && ?

> +/*
> + * 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;
> -    mfn_t mfn;
> -    p2m_access_t a;
> -    p2m_type_t t;
> -    int rc;
> +    struct p2m_domain *p2m = host_p2m;
> +    uint64_t start = sve->first_gfn;
> +    int rc = 0;
>  
> -    if ( altp2m_idx > 0 )
> +    if ( sve->view > 0 )
>      {
> -        if ( altp2m_idx >= min(ARRAY_SIZE(d->arch.altp2m_p2m), MAX_EPTP) ||
> -             d->arch.altp2m_eptp[array_index_nospec(altp2m_idx, MAX_EPTP)] ==
> +        if ( sve->view >= min(ARRAY_SIZE(d->arch.altp2m_p2m), MAX_EPTP) ||
> +             d->arch.altp2m_eptp[array_index_nospec(sve->view, MAX_EPTP)] ==
>               mfn_x(INVALID_MFN) )
>              return -EINVAL;
>  
> -        p2m = ap2m = d->arch.altp2m_p2m[array_index_nospec(altp2m_idx,
> +        p2m = ap2m = d->arch.altp2m_p2m[array_index_nospec(sve->view,
>                                          ARRAY_SIZE(d->arch.altp2m_p2m))];
>      }
> -    else
> -        p2m = host_p2m;
>  
> -    gfn_lock(host_p2m, gfn, 0);
> +    p2m_lock(host_p2m);
>  
>      if ( ap2m )
>          p2m_lock(ap2m);
>  
> -    rc = altp2m_get_effective_entry(p2m, gfn, &mfn, &t, &a, AP2MGET_query);
> +    while ( sve->last_gfn >= start )
> +    {
> +        p2m_access_t a;
> +        p2m_type_t t;
> +        mfn_t mfn;
> +        int err = 0;
>  
> -    if ( rc )
> -        goto out;
> +        if ( (err = altp2m_get_effective_entry(p2m, _gfn(start), &mfn, &t, 
> &a,
> +                                               AP2MGET_query)) &&
> +             !sve->first_error )
> +        {
> +            sve->first_error_gfn = start; /* Save the gfn of the first error 
> */
> +            sve->first_error = err; /* Save the first error code */
> +        }
>  
> -    rc = p2m->set_entry(p2m, gfn, mfn, PAGE_ORDER_4K, t, a, suppress_ve);
> +        if ( !err && (err = p2m->set_entry(p2m, _gfn(start), mfn,
> +                                           PAGE_ORDER_4K, t, a,
> +                                           sve->suppress_ve)) &&
> +             !sve->first_error )
> +        {
> +            sve->first_error_gfn = start; /* Save the gfn of the first error 
> */
> +            sve->first_error = err; /* Save the first error code */
> +        }

I think it would help readability if you didn't do this error
saving twice.

Jan

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