Re: [PATCH v10 45/59] i386/xen: Implement HYPERVISOR_grant_table_op and GNTTABOP_[gs]et_verson

[David Woodhouse]

On 14 February 2023 10:59:12 CET, Paul Durrant <xadimgnik@gmail.com> wrote:
>On 01/02/2023 14:31, David Woodhouse wrote:
>> From: David Woodhouse <dwmw@amazon.co.uk>
>> 
>> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
>> ---
>>   hw/i386/kvm/xen_gnttab.c  | 31 ++++++++++++++++++++
>>   hw/i386/kvm/xen_gnttab.h  |  5 ++++
>>   target/i386/kvm/xen-emu.c | 60 +++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 96 insertions(+)
>> 
>> diff --git a/hw/i386/kvm/xen_gnttab.c b/hw/i386/kvm/xen_gnttab.c
>> index cd8c3ae60d..9dfce91f6a 100644
>> --- a/hw/i386/kvm/xen_gnttab.c
>> +++ b/hw/i386/kvm/xen_gnttab.c
>> @@ -181,3 +181,34 @@ int xen_gnttab_map_page(uint64_t idx, uint64_t gfn)
>>       return 0;
>>   }
>>   +int xen_gnttab_set_version_op(struct gnttab_set_version *set)
>> +{
>> +    int ret;
>> +
>> +    switch (set->version) {
>> +    case 1:
>> +        ret = 0;
>> +        break;
>> +
>> +    case 2:
>> +        /* Behave as before set_version was introduced. */
>> +        ret = -ENOSYS;
>> +        break;
>> +
>> +    default:
>> +        ret = -EINVAL;
>> +    }
>> +
>> +    set->version = 1;
>
>Seems like an odd semantic. Only a version of 1 can result in a non-error 
>exit. I suspect no harm will come from setting it in the error case though 
>so...

Yeah, this is behaviour I saw in an actual Xen code base when gnttab v2 was 
disabled. The logic (in the comment) seemed sane enough.