On 10.01.2020 18:20, Jan Beulich wrote:
> 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 ...
I just saw that now, yes you are right, from when I changed the field names.
>
>> Changes since V6:
>> - Fix commit message
>
> ... has this really happened?
This was done for the "braek" typo and for large/big ranges but I will
fix the field names as well, thanks for pointing that out.
>
>> --- 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?
Sure, it's no trouble.
>
>> + if ( !(rc = p2m_set_suppress_ve_multi(d, &sve)) && sve.first_error )
>> + rc = sve.first_error;
>
> Why the right side of the && ?
This is intended to have p2m_set_suppress_ve() call
p2m_set_suppress_ve_multi(). So here first I call the _multi version and
the check if there was any error from the set/get (that is what
p2m_set_suppress_ve did before). I don't know why git made the patch so
ugly.
>
>> +/*
>> + * 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.
>
George requested this as well as having the single version call the
_multi version.
Alex
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
