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

[Alexandru Stefan ISAILA <aisaila@xxxxxxxxxxxxxxx>]

On 03.03.2020 11:30, Jan Beulich wrote:
> On 26.02.2020 14:18, Alexandru Stefan ISAILA wrote:
>> @@ -4840,6 +4841,19 @@ static int do_altp2m_op(
>>           break;
>>       }
>>   
>> +    case HVMOP_altp2m_set_visibility:
>> +    {
>> +        uint16_t idx = a.u.set_visibility.altp2m_idx;
> 
> Why a fixed width type (and even one inefficient to deal with)?
> (One might even ask - why a local variable in the first place,
> when it's used ...
> 
>> +        if ( a.u.set_visibility.pad )
>> +            rc = -EINVAL;
>> +        else if ( !altp2m_active(d) )
>> +            rc = -EOPNOTSUPP;
>> +        else
>> +            rc = p2m_set_altp2m_view_visibility(d, idx,
>> +                                                a.u.set_visibility.visible);
> 
> ... just once here.) The function takes "unsigned int" in any
> event.

Sure, I can have this idx dropped and use the value in the structure.
I had that in place to have line size smaller and the code easy to read.

> 
>> @@ -3145,6 +3148,35 @@ int p2m_get_suppress_ve(struct domain *d, gfn_t gfn, 
>> bool *suppress_ve,
>>   
>>       return rc;
>>   }
>> +
>> +int p2m_set_altp2m_view_visibility(struct domain *d, unsigned int 
>> altp2m_idx,
>> +                                   uint8_t visible)
>> +{
>> +    altp2m_list_lock(d);
>> +
>> +    /*
>> +     * Eptp index is correlated with altp2m index and should not exceed
>> +     * min(MAX_ALTP2M, MAX_EPTP).
>> +     */
>> +    if ( altp2m_idx >= min(ARRAY_SIZE(d->arch.altp2m_p2m), MAX_EPTP) ||
>> +         d->arch.altp2m_eptp[array_index_nospec(altp2m_idx, MAX_EPTP)] ==
>> +         mfn_x(INVALID_MFN) )
>> +    {
>> +        altp2m_list_unlock(d);
> 
> I think it would be nice if this went the normal function exit path.
> Would be pretty simple to arrange for by introducing a local variable
> holding the function return value.
> 

I had the return here so as not to have boundary issues if the 
altp2m_idx is wrong  and then I have to manipulate  altp2m_eptp[].


But sure, it can have a local rc var that is returned at the end of the 
function and drop this unlock just to use a single one before the return.

Alex

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