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

[Alexandru Stefan ISAILA <aisaila@xxxxxxxxxxxxxxx>]

On 18.11.2019 16:09, Jan Beulich wrote:
> 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.

This sound good, I can use the return for this or have a separate field 
in the structure.

> 
>>>> --- 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?
> 

That is correct but there was also requested to add the new opaque field 
so I don't know how to have that an still keep the same version.

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