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

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

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.

> +- "enabled": The grants are always enabled.
> +
> +- "disabled": The grants are always disabled.

> diff --git a/tools/libs/light/libxl_types.idl 
> b/tools/libs/light/libxl_types.idl
> index c10292e0d7e3..17228817f9b7 100644
> --- a/tools/libs/light/libxl_types.idl
> +++ b/tools/libs/light/libxl_types.idl
> @@ -283,6 +283,12 @@ libxl_virtio_transport = Enumeration("virtio_transport", 
> [
>      (1, "MMIO"),
>      ])
>  
> +libxl_virtio_grant_usage = Enumeration("virtio_grant_usage", [
> +    (0, "DEFAULT"),
> +    (1, "DISABLED"),
> +    (2, "ENABLED"),

libxl already provide this type, it's call "libxl_defbool". It can be
set to "default", "false" or "true".



> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
> index 97c80d7ed0fa..9cd7dbef0237 100644
> --- a/tools/libs/light/libxl_arm.c
> +++ b/tools/libs/light/libxl_arm.c
> @@ -1363,22 +1365,29 @@ static int libxl__prepare_dtb(libxl__gc *gc, 
> libxl_domain_config *d_config,
>                      iommu_needed = true;
>  
>                  FDT( make_virtio_mmio_node(gc, fdt, disk->base, disk->irq,
> -                                           disk->backend_domid) );
> +                                           disk->backend_domid,
> +                                           disk->backend_domid != 
> LIBXL_TOOLSTACK_DOMID) );
>              }
>          }
>  
>          for (i = 0; i < d_config->num_virtios; i++) {
>              libxl_device_virtio *virtio = &d_config->virtios[i];
> +            bool use_grant = false;
>  
>              if (virtio->transport != LIBXL_VIRTIO_TRANSPORT_MMIO)
>                  continue;
>  
> -            if (virtio->backend_domid != LIBXL_TOOLSTACK_DOMID)
> +            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.

> diff --git a/tools/libs/light/libxl_virtio.c b/tools/libs/light/libxl_virtio.c
> index eadcb7124c3f..0a0fae967a0f 100644
> --- a/tools/libs/light/libxl_virtio.c
> +++ b/tools/libs/light/libxl_virtio.c
> @@ -46,11 +46,13 @@ static int libxl__set_xenstore_virtio(libxl__gc *gc, 
> uint32_t domid,
>                                        flexarray_t *ro_front)
>  {
>      const char *transport = 
> libxl_virtio_transport_to_string(virtio->transport);
> +    const char *grant_usage = 
> libxl_virtio_grant_usage_to_string(virtio->grant_usage);
>  
>      flexarray_append_pair(back, "irq", GCSPRINTF("%u", virtio->irq));
>      flexarray_append_pair(back, "base", GCSPRINTF("%#"PRIx64, virtio->base));
>      flexarray_append_pair(back, "type", GCSPRINTF("%s", virtio->type));
>      flexarray_append_pair(back, "transport", GCSPRINTF("%s", transport));
> +    flexarray_append_pair(back, "grant_usage", GCSPRINTF("%s", grant_usage));

Currently, this mean that we store "default" in this node. That mean
that both the virtio backend and libxl have to do computation in order
to figure out if "default" mean "true" or "false". And both have to find
the same result. I don't think this is necessary, and libxl can just
tell enabled or disable. This would be done in libxl before we run this
function. See previous comment on this patch.

Thanks,

-- 
Anthony PERARD