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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
