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

[Alexandru Stefan ISAILA <aisaila@xxxxxxxxxxxxxxx>]

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

> 
>> +    {
>> +        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;
>> +
>> +        if ( (err = 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 of the first error */
>> +            sve->first_error_code = err; /* Save the first error code */
>> +        }
> 
> What if the first error occurs on GFN 0? I guess you want to check
> ->first_error_code against zero in the condition.

That is right, I will change to check ->first_error_code instead of 
->first_error.

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