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

[Alexandru Stefan ISAILA <aisaila@xxxxxxxxxxxxxxx>]

On 24.12.2019 10:30, George Dunlap wrote:
> On 12/23/19 2:04 PM, 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 brake in case
> 
> *break

Sorry for the typo.

> 
>> 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 big ranges.
> 
> Weird English quirk: this should be "large".  ("Big" and "large" are
> both adjectives, and "ranges" is a noun, so theoretically it should be
> OK; but if you ask almost any native English speaker they'll say that
> "big" sounds wrong in this case.  No real idea why.)
> 
> Both of these could be fixed on check-in.
> 
>> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
>> index 4fc919a9c5..de832dcc6d 100644
>> --- a/xen/arch/x86/mm/p2m.c
>> +++ b/xen/arch/x86/mm/p2m.c
>> @@ -3070,6 +3070,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;
>> +
>> +    if ( sve->view > 0 )
>> +    {
>> +        if ( sve->view >= MAX_ALTP2M ||
>> +             d->arch.altp2m_eptp[array_index_nospec(sve->view, MAX_ALTP2M)] 
>> ==
>> +             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 )
>> +    {
>> +        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;
> 
> So in the single-entry version, if altp2m_get_effective_entry() returns
> an error, you pass that error up the stack; but in the multiple-entry
> version, you ignore the error and simply set the access to
> default_access?  I don't think that can be right.  If it is right, then
> it definitely needs a comment.
> 

The idea behind this was to have a best effort try and signal the first 
error. If the get_entry fails then the best way to go is with 
default_access but this is open for debate.

Another way to solve this is to update the first_error_gfn/first_error 
and then continue. I think this ca be used to make p2m_set_suppress_ve() 
call p2m_set_suppress_ve_multi.

> This points out another issue: implementing this functionality twice
> risks having this sort of drift between the single-entry version and the
> multiple-entry version.  Would it make sense instead to implement the
> single-entry version hypercall using p2m_set_suppress_ve_multi?
> 

In the single version there is no best-effort idea because the user can 
make use of every single error.

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