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

[Alexandru Stefan ISAILA <aisaila@xxxxxxxxxxxxxxx>]

On 17.02.2020 16:14, Jan Beulich wrote:
> On 30.01.2020 14:07, Alexandru Stefan ISAILA wrote:
>> @@ -4814,6 +4815,30 @@ static int do_altp2m_op(
>>           break;
>>       }
>>   
>> +    case HVMOP_altp2m_set_visibility:
>> +    {
>> +        uint16_t altp2m_idx = a.u.set_visibility.altp2m_idx;
>> +
>> +        if ( a.u.set_visibility.pad || a.u.set_visibility.pad2 )
>> +            rc = -EINVAL;
>> +        else
>> +        {
>> +            if ( !altp2m_active(d) || !hap_enabled(d) )
> 
> Doesn't altp2m_active() imply hap_enabled()? At the very least
> there's no other use of hap_enabled() in do_altp2m_op().

Yes, the hap_enabled can be dropped.

> 
>> +            {
>> +                rc = -EOPNOTSUPP;
>> +                break;
>> +            }
>> +
>> +            if ( a.u.set_visibility.visible )
>> +                d->arch.altp2m_working_eptp[altp2m_idx] =
>> +                d->arch.altp2m_eptp[altp2m_idx];
>> +            else
>> +                d->arch.altp2m_working_eptp[altp2m_idx] =
>> +                mfn_x(INVALID_MFN);
>> +        }
>> +        break;
> 
> Also the code here lends itself to reduction of indentation
> depth:
> 
>      case HVMOP_altp2m_set_visibility:
>      {
>          uint16_t altp2m_idx = a.u.set_visibility.altp2m_idx;
> 
>          if ( a.u.set_visibility.pad || a.u.set_visibility.pad2 )
>              rc = -EINVAL;
>          else if ( !altp2m_active(d) || !hap_enabled(d) )
>                  rc = -EOPNOTSUPP;
>          else if ( a.u.set_visibility.visible )
>              d->arch.altp2m_working_eptp[altp2m_idx] =
>                  d->arch.altp2m_eptp[altp2m_idx];
>          else
>              d->arch.altp2m_working_eptp[altp2m_idx] =
>                  mfn_x(INVALID_MFN);
> 
>          break;
>      }
> 
> 
> Also note the altered indentation of the assignments.

I will fix the else if alignment as well as the indentation for the 
assignments.

> 
>> --- a/xen/arch/x86/mm/hap/hap.c
>> +++ b/xen/arch/x86/mm/hap/hap.c
>> @@ -488,8 +488,17 @@ int hap_enable(struct domain *d, u32 mode)
>>               goto out;
>>           }
>>   
>> +        if ( (d->arch.altp2m_working_eptp = alloc_xenheap_page()) == NULL )
>> +        {
>> +            rv = -ENOMEM;
>> +            goto out;
>> +        }
> 
> Isn't there a pre-existing error handling issue here which you
> widen, in that later encountered errors don't cause clean up
> of what had already succeeded before?

It seems non of the errors perform a cleanup. It might be better to have 
a general cleanup done at "out:" if ( !rv ) and then check what should 
be "p2m_teardown()" or "free_xenheap_page()".

> 
>> @@ -2651,6 +2652,8 @@ 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);
>> +            d->arch.altp2m_working_eptp[array_index_nospec(idx, MAX_EPTP)] =
>> +            mfn_x(INVALID_MFN);
> 
> Like above, and irrespective of you cloning existing code -
> indentation of the 2nd line is wrong here.
> 
>> --- a/xen/include/public/hvm/hvm_op.h
>> +++ b/xen/include/public/hvm/hvm_op.h
>> @@ -317,6 +317,13 @@ struct xen_hvm_altp2m_get_vcpu_p2m_idx {
>>       uint16_t altp2m_idx;
>>   };
>>   
>> +struct xen_hvm_altp2m_set_visibility {
>> +    uint16_t altp2m_idx;
>> +    uint8_t visible;
>> +    uint8_t pad;
>> +    uint32_t pad2;
>> +};
> 
> What is pad2 good/intended for? 32-bit padding fields in some
> other structures are needed because one or more uint64_t
> fields follow, but this isn't the case here.

Right, pad2 can be dropped.


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