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

[Alexandru Stefan ISAILA <aisaila@xxxxxxxxxxxxxxx>]

On 29.11.2019 13:31, Jan Beulich wrote:
> 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.

I will update the commit message.

> 
>> @@ -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?

I thought about this and the only error that can be replaced here is 
EINVAL. A error on __copy_to_guest has a grater importance if this fails.

> 
>> --- 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).

Sure, this can change to p2m = ap2m = 
d->arch.altp2m_p2m[array_index_nospec(sve->view, MAX_ALTP2M).

But what preexisting uses are you talking about? All the places where 
d->arch.altp2m_p2m[idx] is used? If so, there will be a handful of 
changes in that new 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.

The only reason I can see for this pattern is that p2m will be assigned 
a value but ap2m can never get a value. But I agree with you and I will 
have them both initialized with NULL.

> 
>> +    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.

I was following the pattern from range_share() after Tamas requested the 
opaque field. I agree that it would be simpler to have ->first_gfn 
update and I can change to that in the next version.

> 
> 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().

Do you mean to check ->first_gfn(that will be updated in the next 
version) against domain_get_maximum_gpfn() and bail after that range?

> 
>> +    {
>> +        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?

That sound good.

> 
>> +    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.

I will update the comments.

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