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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
