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

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Mon, Feb 05, 2024 at 04:52:16PM +0000, Oleksandr Tyshchenko wrote:
> On 05.02.24 17:10, Anthony PERARD wrote:
> > On Fri, Feb 02, 2024 at 12:49:03PM +0200, Oleksandr Tyshchenko wrote:
> >> +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).
> 
> >> 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"?

Let's go with "grant_usage=*", at least this will be consistent with the
option for "virtio".

Cheers,

-- 
Anthony PERARD