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