Re: [PATCH] libxl: Add "grant_usage" parameter for virtio disk devices

[Oleksandr Tyshchenko <Oleksandr_Tyshchenko@xxxxxxxx>]

On 05.02.24 17:10, Anthony PERARD wrote:

Hello Anthony


> On Fri, Feb 02, 2024 at 12:49:03PM +0200, Oleksandr Tyshchenko wrote:
>> diff --git a/tools/libs/util/libxlu_disk_l.l 
>> b/tools/libs/util/libxlu_disk_l.l
>> index 6d53c093a3..f37dd443bd 100644
>> --- a/tools/libs/util/libxlu_disk_l.l
>> +++ b/tools/libs/util/libxlu_disk_l.l
>> @@ -220,6 +220,9 @@ hidden-disk=[^,]*,?      { STRIP(','); 
>> SAVESTRING("hidden-disk", hidden_disk, FROMEQU
>>   trusted,?          { libxl_defbool_set(&DPC->disk->trusted, true); }
>>   untrusted,?                { libxl_defbool_set(&DPC->disk->trusted, 
>> false); }
>>   
>> +grant_usage=1,?             { libxl_defbool_set(&DPC->disk->grant_usage, 
>> true); }
>> +grant_usage=0,?             { libxl_defbool_set(&DPC->disk->grant_usage, 
>> false); }
> 
> For other boolean type for the disk, we have "trusted/untrusted",
> "discard/no-discard", "direct-io-save/", but you are adding
> "grant_usage=1/grant_usage=0". Is that fine? But I guess having the new
> option spelled "grant_usage" might be better, so it match the other
> virtio devices and the implementation. 


Yes, I noticed that how booleans are described for the disk. I decided 
to use the same representation of this option as it was already used for 
virtio=[...]. But I would be ok with other variants ...


But maybe
> "use-grant/no-use-grant" might be ok?

   ... like that, but preferably with leaving libxl_device_disk's field 
named "grant_usage" (if no objection).


> 
> In any case, the implementation need to match the documentation, and
> vice versa. See below.


Sure.


> 
>> diff --git a/docs/man/xl-disk-configuration.5.pod.in 
>> b/docs/man/xl-disk-configuration.5.pod.in
>> index bc945cc517..3c035456d5 100644
>> --- a/docs/man/xl-disk-configuration.5.pod.in
>> +++ b/docs/man/xl-disk-configuration.5.pod.in
>> @@ -404,6 +404,31 @@ Virtio frontend driver (virtio-blk) to be used. Please 
>> note, the virtual
>> +=item B<grant_usage=BOOLEAN>
>>
>> +=over 4
>> +
>> +=item Description
>> +
>> +Specifies the usage of Xen grants for accessing guest memory. Only 
>> applicable
>> +to specification "virtio".
>> +
>> +=item Supported values
>> +
>> +If this option is B<true>, the Xen grants are always enabled.
>> +If this option is B<false>, the Xen grants are always disabled.
> 
> Unfortunately, this is wrong, the implementation in the patch only
> support two values: 1 / 0, nothing else, and trying to write "true" or
> "false" would lead to an error. (Well actually it's "grant_usage=1" or
> "grant_usage=0", there's nothing that cut that string at the '='.)


You are right, only 1 / 0 can be set unlike for virtio=[...] which seems 
happy with false/true.


> 
> Also, do we really need the extra verbal description of each value here?
> Is simply having the following would be enough?
> 
>      =item Supported values
> 
>      1, 0
> 
> The description in "Description" section would hopefully be enough.


I think, this makes sense.

So, shall I leave "grant_usage=1/grant_usage=0" or use proposed option 
"use-grant/no-use-grant"?


> 
>> +=item Mandatory
>> +
>> +No
>> +
>> +=item Default value
>> +
>> +If this option is missing, then the default grant setting will be used,
>> +i.e. enable grants if backend-domid != 0.
>> +
>> +=back
>> +
>> diff --git a/tools/libs/light/libxl_disk.c b/tools/libs/light/libxl_disk.c
>> index ea3623dd6f..f39f427091 100644
>> --- a/tools/libs/light/libxl_disk.c
>> +++ b/tools/libs/light/libxl_disk.c
>> @@ -181,6 +181,9 @@ static int libxl__device_disk_setdefault(libxl__gc *gc, 
>> uint32_t domid,
>>               return ERROR_INVAL;
>>           }
>>           disk->transport = LIBXL_DISK_TRANSPORT_MMIO;
>> +
>> +        libxl_defbool_setdefault(&disk->grant_usage,
>> +                                 disk->backend_domid != 
>> LIBXL_TOOLSTACK_DOMID);
>>       }
>>   
>>       if (hotplug && disk->specification == LIBXL_DISK_SPECIFICATION_VIRTIO) 
>> {
>> @@ -429,6 +432,8 @@ static void device_disk_add(libxl__egc *egc, uint32_t 
>> domid,
>>               flexarray_append(back, 
>> libxl__device_disk_string_of_transport(disk->transport));
>>               flexarray_append_pair(back, "base", GCSPRINTF("%"PRIu64, 
>> disk->base));
>>               flexarray_append_pair(back, "irq", GCSPRINTF("%u", disk->irq));
>> +            flexarray_append_pair(back, "grant_usage",
>> +                                  libxl_defbool_val(disk->grant_usage) ? 
>> "1" : "0");
>>           }
>>   
>>           flexarray_append(front, "backend-id");
>> @@ -623,6 +628,14 @@ static int libxl__disk_from_xenstore(libxl__gc *gc, 
>> const char *libxl_path,
>>               goto cleanup;
>>           }
>>           disk->irq = strtoul(tmp, NULL, 10);
>> +
>> +        tmp = libxl__xs_read(gc, XBT_NULL,
>> +                             GCSPRINTF("%s/grant_usage", libxl_path));
>> +        if (!tmp) {
>> +            LOG(ERROR, "Missing xenstore node %s/grant_usage", libxl_path);
>> +            goto cleanup;
> 
> I wonder if it's such a good idea to make this node mandatory. Could we
> just apply the default value if the path is missing? I don't think the
> value is going to be used anyway because I don't think from_xenstore() is
> used during guest creation, and it looks like "grant_usage" is only
> useful during guest creation. Also, the "grant_usage" node isn't
> mandatory in "libxl_virtio.c", so no need to do something different
> for disk.

I agree with your analysis, no need to raise an error if missing, let's 
apply a default value which is the result of "disk->backend_domid != 
LIBXL_TOOLSTACK_DOMID".


> 
>> +        }
>> +        libxl_defbool_set(&disk->grant_usage, strtoul(tmp, NULL, 0));
>>       }
>>   
>>       disk->vdev = xs_read(ctx->xsh, XBT_NULL,
> 
> Thanks,
>