Re: [PATCH] libxl: arm: Allow grant mappings for backends running on Dom0

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Thu, Mar 30, 2023 at 02:13:08PM +0530, Viresh Kumar wrote:
> diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
> index 10f37990be57..4879f136aab8 100644
> --- a/docs/man/xl.cfg.5.pod.in
> +++ b/docs/man/xl.cfg.5.pod.in
> @@ -1616,6 +1616,10 @@ properties in the Device Tree, the type field must be 
> set to "virtio,device".
>  Specifies the transport mechanism for the Virtio device, only "mmio" is
>  supported for now.
>  
> +=item B<forced_grant=BOOLEAN>
> +
> +Allows Xen Grant memory mapping to be done from Dom0.

I think this description is missing context. I'm not sure what it would
means "from dom0" without reading the patch. Also, it says "allows",
maybe this doesn't convey the meaning of "forced". How about something
like:

    Always use grant mapping, even when the backend is run in dom0.
    (grant are already used if the backend is in another domain.)

> +
>  =back
>  
>  =item B<tee="STRING">
> diff --git a/tools/libs/light/libxl_virtio.c b/tools/libs/light/libxl_virtio.c
> index faada49e184e..e1f15344ef97 100644
> --- a/tools/libs/light/libxl_virtio.c
> +++ b/tools/libs/light/libxl_virtio.c
> @@ -48,11 +48,13 @@ static int libxl__set_xenstore_virtio(libxl__gc *gc, 
> uint32_t domid,
>      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, "forced_grant", GCSPRINTF("%u", 
> virtio->forced_grant));
>  
>      flexarray_append_pair(front, "irq", GCSPRINTF("%u", virtio->irq));
>      flexarray_append_pair(front, "base", GCSPRINTF("%#"PRIx64, 
> virtio->base));
>      flexarray_append_pair(front, "type", GCSPRINTF("%s", virtio->type));
>      flexarray_append_pair(front, "transport", GCSPRINTF("%s", transport));
> +    flexarray_append_pair(front, "forced_grant", GCSPRINTF("%u", 
> virtio->forced_grant));

This "forced_grant" feels weird to me in the protocol, I feel like this
use of grant or not could be handled by the backend. For example in
"blkif" protocol, there's plenty of "feature-*" which allows both
front-end and back-end to advertise which feature they can or want to
use.
But maybe the fact that the device tree needs to be modified to be able
to accommodate grant mapping means that libxl needs to ask the backend to
use grant or not, and the frontend needs to now if it needs to use
grant.

>  
>      return 0;
>  }
> @@ -104,6 +106,15 @@ static int libxl__virtio_from_xenstore(libxl__gc *gc, 
> const char *libxl_path,
>          }
>      }
>  
> +    tmp = NULL;
> +    rc = libxl__xs_read_checked(gc, XBT_NULL,
> +                             GCSPRINTF("%s/forced_grant", be_path), &tmp);
> +    if (rc) goto out;
> +
> +    if (tmp) {
> +        virtio->forced_grant = strtoul(tmp, NULL, 0);
> +    }
> +
>      tmp = NULL;
>      rc = libxl__xs_read_checked(gc, XBT_NULL,
>                               GCSPRINTF("%s/type", be_path), &tmp);
> diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
> index 1f6f47daf4e1..3e34da099785 100644
> --- a/tools/xl/xl_parse.c
> +++ b/tools/xl/xl_parse.c
> @@ -1215,6 +1215,8 @@ static int parse_virtio_config(libxl_device_virtio 
> *virtio, char *token)
>      } else if (MATCH_OPTION("transport", token, oparg)) {
>          rc = libxl_virtio_transport_from_string(oparg, &virtio->transport);
>          if (rc) return rc;
> +    } else if (MATCH_OPTION("forced_grant", token, oparg)) {
> +        virtio->forced_grant = strtoul(oparg, NULL, 0);

Maybe store only !!strtoul() ?
I don't think having values other that 0 or 1 is going to be good.

>      } else {
>          fprintf(stderr, "Unknown string \"%s\" in virtio spec\n", token);
>          return -1;

Thanks,

-- 
Anthony PERARD