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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.11.2019 14:39, Alexandru Stefan ISAILA wrote:
> On 12.11.2019 13:54, Jan Beulich wrote:
>> On 06.11.2019 16:35, Alexandru Stefan ISAILA wrote:
>>> @@ -4693,8 +4694,23 @@ static int do_altp2m_op(
>>>           }
>>>           break;
>>>   
>>> +    case HVMOP_altp2m_set_suppress_ve_multi:
>>> +        if ( a.u.suppress_ve.pad1 || !a.u.suppress_ve.nr )
>>
>> A count of zero typically is taken as a no-op, not an error.
> 
> I will return -EPERM for !nr.

How is -EPERM better than ...

>>> +            rc = -EINVAL;

... this, and hence how is it addressing my remark?

>>> +        else
>>> +        {
>>> +            rc = p2m_set_suppress_ve_multi(d, &a.u.suppress_ve);
>>> +
>>> +            if ( rc == -ERESTART )
>>> +                if ( __copy_field_to_guest(guest_handle_cast(arg,
>>> +                                           xen_hvm_altp2m_op_t),
>>> +                                           &a, u.suppress_ve.opaque) )
>>> +                    rc = -EFAULT;
>>
>> If the operation is best effort, _some_ indication of failure should
>> still be handed back to the caller. Whether that's through the opaque
>> field or by some other means is secondary. If not via that field
>> (which would make the outer of the two if()-s disappear), please fold
>> the if()-s.
> 
> This can be solved by having a int error_list that will get 
> "copy_to_guest_offset()" at the end.

I was actually not meaning to suggest to go _that_ far, but I
wouldn't mind such a full solution. Since there's a "get"
counterpart, I was rather thinking that an indication of "there
was _some_ error" might suffice, suggesting to the caller to
inspect which settings actually took effect. Such an indication
could e.g. be an index value identifying the first failed
operation.

>>> --- a/xen/include/public/hvm/hvm_op.h
>>> +++ b/xen/include/public/hvm/hvm_op.h
>>> @@ -42,8 +42,9 @@ struct xen_hvm_altp2m_suppress_ve {
>>>       uint16_t view;
>>>       uint8_t suppress_ve; /* Boolean type. */
>>>       uint8_t pad1;
>>> -    uint32_t pad2;
>>> +    uint32_t nr;
>>>       uint64_t gfn;
>>> +    uint64_t opaque;
>>>   };
>>
>> How is this addition of a field going to work compatibly with old
>> and new callers on old and new hypervisors? Recall in particular
>> that these operations are (almost?) all potentially usable by the
>> guest itself.
> 
> For this HVMOP_ALTP2M_INTERFACE_VERSION shout be increased. I will leave 
> it to Tamas to decide if we will need a different structure for 
> xen_hvm_altp2m_suppress_ve_multi to keep the compatibility.

Wasn't is that due to the possible guest exposure it was decided
that the interface version approach was not suitable here, and hence
it shouldn't be bumped any further?

Jan

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