Re: [PATCH V2 2/2] libxl: arm: Add grant_usage parameter for virtio devices

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Mon, May 15, 2023 at 05:36:00PM +0530, Viresh Kumar wrote:
> On 12-05-23, 11:43, Anthony PERARD wrote:
> > On Thu, May 11, 2023 at 01:20:43PM +0530, Viresh Kumar wrote:
> > > diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
> > > index 24ac92718288..0405f6efe62a 100644
> > > --- a/docs/man/xl.cfg.5.pod.in
> > > +++ b/docs/man/xl.cfg.5.pod.in
> > > @@ -1619,6 +1619,18 @@ hexadecimal format, without the "0x" prefix and 
> > > all in lower case, like
> > >  Specifies the transport mechanism for the Virtio device, only "mmio" is
> > >  supported for now.
> > >  
> > > +=item B<grant_usage=STRING>
> > > +
> > > +Specifies the grant usage details for the Virtio device. This can be set 
> > > to
> > > +following values:
> > > +
> > > +- "default": The default grant setting will be used, enable grants if
> > > +  backend-domid != 0.
> > 
> > I don't think this "default" setting is useful. We could just describe
> > what the default is when "grant_usage" setting is missing from the
> > configuration.
> 
> This is what I suggested earlier [1], maybe I misunderstood what
> Juergen said.

To me, as a user of any program, setting default to a configuration
option is when we don't select a configuration option. Like when
starting a program for the first time and let it set things up based on
the environment if that make senses. But I guess sometime when there's
multiple choice, we can select default.

Anyway, I've looked in the xl.cfg man page and there's already plenty of
example where "default" is an option, so I guess it doesn't really hurt
to have the option to choose to not choose. You still need to write in
the man page that "default" is the default option, as in the absence of
the option in the configuration the default option will be used (unless
you managed somehow to make the option mandatory, but is they a reason
for that?).

In any case, there's going to be a 3-state option between xl and libxl
which are going to be default, false, true. It doesn't matter whether a
user can write default or not.

> > > +- "enabled": The grants are always enabled.
> > > +
> > > +- "disabled": The grants are always disabled.
> > 
> > > +            if ((virtio->grant_usage == 
> > > LIBXL_VIRTIO_GRANT_USAGE_ENABLED) ||
> > > +                ((virtio->grant_usage == 
> > > LIBXL_VIRTIO_GRANT_USAGE_DEFAULT) &&
> > > +                 (virtio->backend_domid != LIBXL_TOOLSTACK_DOMID))) {
> > 
> > I think libxl can select what the default value should be replace with
> > before we start to setup the guest. There's a *_setdefault() phase were
> > we set the correct value when a configuration value hasn't been set and
> > thus a default value is used. I think this can be done in
> >     libxl__device_virtio_setdefault().
> > After that, virtio->grant_usage will be true or false, and that's the
> > value that should be given to the virtio backend via xenstore.
> 
> I am not great with Xen's flow of stuff and so would like to clarify a
> few things here since I am confused.
> 
> In my case, parse_virtio_config() gets called first followed by
> libxl__prepare_dtb(), where I need to use the "grant_usage" field.
> Later libxl__device_virtio_setdefault() gets called, anything done
> there isn't of much use in my case I guess.

:-(, I feel like something is missing. I would think that
libxl__prepare_dtb() would be called after any _setdefault() function.
Maybe something isn't calling setdefault for virtio devices soon enough
in libxl.

> Setting the default value of grant_usage in
> libxl__device_virtio_setdefault() doesn't work for me (since
> libxl__prepare_dtb() is already called), and I need to set this in
> parse_virtio_config() only.

I don't think that `xl` should set any default, that would be better
done in libxl. libxl could be used by another program, such as
`libvirt`.

> Currently, virtio->backend_domid is getting set via
> libxl__resolve_domid() in libxl__device_virtio_setdefault(), which is
> too late for me, but is working fine, accidentally I think, since the
> default value of the field is 0, which is same as domain id in my
> case. I would like to understand though how it works for Disk device
> for Oleksandr, since they must also face similar issues. I must be
> doing something wrong here :)

No, I think something is missing for virtio devices.

For disk, there's code in initiate_domain_create() which call
libxl__disk_devtype.set_default() for every disk, and this happen before
libxl__prepare_dtb(). I don't know how other device types are doing this
defaulting, I need to search.

There's also a special case for nic, a call of
libxl__device_nic_set_devids() does call set_default for nics.
Otherwise, I think set_default() is called whenever something calls
add().


So, for virtio devices in libxl, I think we will also want to call
set_default() early. Add a call to libxl__virtio_devtype.set_default()
in libxl__domain_config_setdefault() similar to the one done for disk.
(For disk, at the moment it is done in initiate_domain_create() but
let's use the new libxl__domain_config_setdefault() function for
virtio.)

This means that libxl__device_virtio_setdefault() would be called twice
for each device, but that shouldn't be an issue.

Would that work?

> Lastly, libxl__virtio_from_xenstore() is never called in my case.
> Should I just remove it ? I copied it from some other implementation.

I don't think from_xenstore() is normally called when creating a guest,
but if we had an `xl` command called "virtio-list", like there's
"block-list", then from_xenstore() would be use I think. It could also be
use when doing *-detach, even if virtio doesn't have the option.


Cheers,

-- 
Anthony PERARD