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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 17.12.2019 16:12, Alexandru Stefan ISAILA wrote:
> @@ -4711,6 +4712,20 @@ 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.first_error_code ||
> +             a.u.suppress_ve_multi.first_error ||
> +             a.u.suppress_ve_multi.first_gfn > 
> a.u.suppress_ve_multi.last_gfn )
> +            rc = -EINVAL;

An error having occurred doesn't prevent scheduling of a
continuation. When you come back here, you'll then return
-EINVAL instead of continuing the prior operation.

> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -3064,6 +3064,70 @@ 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 = host_p2m;
> +    uint64_t start = sve->first_gfn;
> +    int rc = 0;
> +    uint64_t max_phys_addr = (1UL << d->arch.cpuid->extd.maxphysaddr) - 1;
> +
> +    if ( sve->view > 0 )
> +    {
> +        if ( sve->view >= MAX_ALTP2M ||
> +             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(sve->view,
> +                                                           MAX_ALTP2M)];
> +    }
> +
> +    p2m_lock(host_p2m);
> +
> +    if ( ap2m )
> +        p2m_lock(ap2m);
> +
> +    while ( sve->last_gfn >= start && start < max_phys_addr )

Why don't you clip ->last_gfn ahead of the loop, saving one
comparison per iteration?

> +    {
> +        p2m_access_t a;
> +        p2m_type_t t;
> +        mfn_t mfn;
> +        int err = 0;
> +
> +        if ( altp2m_get_effective_entry(p2m, _gfn(start), &mfn, &t, &a, 
> AP2MGET_query) )
> +            a = p2m->default_access;
> +
> +        if ( (err = 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 of the first error */
> +            sve->first_error_code = err; /* Save the first error code */
> +        }

What if the first error occurs on GFN 0? I guess you want to check
->first_error_code against zero in the condition.

> --- a/xen/include/public/hvm/hvm_op.h
> +++ b/xen/include/public/hvm/hvm_op.h
> @@ -46,6 +46,16 @@ 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 first_error_code; /* Must be set to 0 . */

int32_t perhaps, since error codes are negative?

Jan

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