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

[Alexandru Stefan ISAILA <aisaila@xxxxxxxxxxxxxxx>]

On 18.12.2019 12:18, Jan Beulich wrote:
> On 18.12.2019 09:45, Alexandru Stefan ISAILA wrote:
>>
>>
>> On 18.12.2019 10:13, Alexandru Stefan ISAILA wrote:
>>>
>>>>> +/*
>>>>> + * 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?
>>>
>>> I've done this so it will have fewer lines but sure, I can have the
>>> ->last_gfn check before the loop.
>>
>> Wouldn't there be a issue if start goes over ->last_gfn and there is no
>> break for preemption? Then the loop will run until max_phys_addr.
> 
> I'm not sure I understand. My guess is a misunderstanding - I'm
> asking to replace the right side of the &&, and it looks you
> understood me to mean the least side. Note how I said "clip" in
> my earlier reply, meaning you to update ->last_gfn ahead of the
> loop if it's above (1UL << d->arch.cpuid->extd.maxphysaddr) - 1.
> Perhaps this could even be done in the caller together with (and
> possibly ahead of) the other sanity checking of incoming values.
> 

Then I could have  "sve->last_gfn = min(sve->last_gfn, max_phys_addr)"
and then drop the "start < max_phys_addr" check from the while loop.

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