Re: [Xen-devel] [PATCH V1 1/2] 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.
>> + */
>> +long p2m_set_suppress_ve_multi(struct domain *d, uint32_t start, uint32_t 
>> nr,
>> +                               bool suppress_ve, unsigned int altp2m_idx)
>> +{
>> +    struct p2m_domain *host_p2m = p2m_get_hostp2m(d);
>> +    struct p2m_domain *ap2m = NULL;
>> +    struct p2m_domain *p2m;
>> +    long rc = 0;
>> +
>> +    if ( altp2m_idx > 0 )
>> +    {
>> +        if ( altp2m_idx >= MAX_ALTP2M ||
>> +             d->arch.altp2m_eptp[altp2m_idx] == mfn_x(INVALID_MFN) )
>> +            return -EINVAL;
>> +
>> +        p2m = ap2m = d->arch.altp2m_p2m[altp2m_idx];
>> +    }
>> +    else
>> +        p2m = host_p2m;
>> +
>> +    p2m_lock(host_p2m);
>> +
>> +    if ( ap2m )
>> +        p2m_lock(ap2m);
>> +
>> +
>> +    while ( start < nr )
>> +    {
>> +        p2m_access_t a;
>> +        p2m_type_t t;
>> +        mfn_t mfn;
>> +
>> +        rc = altp2m_get_effective_entry(p2m, _gfn(start), &mfn, &t, &a, 
>> AP2MGET_query);
>> +
>> +        if ( rc )
>> +            a = p2m->default_access;
>> +
>> +        rc = p2m->set_entry(p2m, _gfn(start), mfn, PAGE_ORDER_4K, t, a, 
>> suppress_ve);
>> +
>> +        /* Try best effort for setting the whole range. */
>> +        if ( rc )
>> +            continue;
>> +
>> +        /* Check for continuation if it's not the last iteration. */
>> +        if ( nr > ++start && hypercall_preempt_check() )
>> +        {
>> +            rc = start;
>> +            break;
>> +        }
> 
> What's the point of the "if ( rc ) continue;"?  All it's doing is
> preventing the loop from being preempted at that point; but there
> doesn't seem to be a good reason for that.  In fact, if an attacker
> could engineer a situation where large swaths could fail, it could use
> this to lock up the cpu for an unreasonable amount of time.

Yes, that could be an issue. It will go in v2

> 

> Everything else looks OK to me.
> 

If the changes requested by Tamas are also ok with you then I will have 
them all go in v2.

Alex

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