Re: [Xen-devel] [PATCH V4] x86/altp2m: Hypercall to set altp2m view visibility

[Alexandru Stefan ISAILA <aisaila@xxxxxxxxxxxxxxx>]

On 21.02.2020 18:39, Jan Beulich wrote:
> On 21.02.2020 09:30, Alexandru Stefan ISAILA wrote:
>> @@ -4835,6 +4836,26 @@ static int do_altp2m_op(
>>           break;
>>       }
>>   
>> +    case HVMOP_altp2m_set_visibility:
>> +    {
>> +        uint16_t idx = a.u.set_visibility.altp2m_idx;
>> +
>> +        if ( a.u.set_visibility.pad ||
>> +             idx >= min(ARRAY_SIZE(d->arch.altp2m_p2m), MAX_EPTP) ||
> 
> Why min() here? You only access MAX_EPTP-dimensioned arrays afaics. If
> this is intentional, I think it deserves a comment.

I have min() here because the altp2m index should not be grater then 
MAX_ALTP2M. I know this is used as altp2m_eptp() index but the two are 
coupled.

> 
>> +             d->arch.altp2m_eptp[array_index_nospec(idx, MAX_EPTP)] ==
>> +             mfn_x(INVALID_MFN) )
>> +            rc = -EINVAL;
>> +        else if ( !altp2m_active(d) )
>> +            rc = -EOPNOTSUPP;
>> +        else if ( a.u.set_visibility.visible )
>> +            d->arch.altp2m_working_eptp[array_index_nospec(idx, MAX_EPTP)] =
>> +                d->arch.altp2m_eptp[array_index_nospec(idx, MAX_EPTP)];
>> +        else
>> +            d->arch.altp2m_working_eptp[array_index_nospec(idx, MAX_EPTP)] =
>> +                mfn_x(INVALID_MFN);
>> +        break;
> 
> You don't seem to be holding any locks. At the very least this means
> the in-range-index-is-valid check further up will have become stale
> by the time you actually consume the slot.
> 

Good thing to point this out here. I will add altp2m_list_lock/unlock(d) 
to guard this check and operation.

>> @@ -2638,7 +2639,9 @@ int p2m_destroy_altp2m_by_id(struct domain *d, 
>> unsigned int idx)
>>           {
>>               p2m_reset_altp2m(d, idx, ALTP2M_DEACTIVATE);
>>               d->arch.altp2m_eptp[array_index_nospec(idx, MAX_EPTP)] =
>> -            mfn_x(INVALID_MFN);
>> +                mfn_x(INVALID_MFN);
>> +            d->arch.altp2m_working_eptp[array_index_nospec(idx, MAX_EPTP)] =
>> +                mfn_x(INVALID_MFN);
> 
> I appreciate you also adjusting the bogus indentation of the pre-
> existing line.
> 

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