Re: [PATCH V6 3/3] docs: Add documentation for generic virtio devices

[Oleksandr Tyshchenko <olekstysh@xxxxxxxxx>]

On 08.11.22 13:24, Viresh Kumar wrote:

Hello Viresh


[sorry for the possible format issues if any]

> This patch updates xl.cfg man page with details of generic Virtio device
> related information.

So as I understand current series adds support for two virtio devices 
(i2c/gpio) that require specific device-tree sub node with specific 
compatible in it [1]. Those backends are standalone userspace 
applications (daemons) that do not require any additional configuration 
parameters from the toolstack other than just virtio-mmio irq and base 
(please correct me if I am wrong).
Well, below just some thoughts (which might be wrong) regarding the 
possible extensions for future use. Please note, I do not suggest the 
following to be implemented right now (I mean within the context of 
current series):
1. For supporting usual virtio devices that don't require specific 
device-tree sub node with specific compatible in it [2] we would 
probably need to either make "compatible" (or type?) string optional or 
to reserve some value for it ("common" for the instance).
2. For supporting Qemu based virtio devices we would probably need to 
add "backendtype" string (with "standalone" value for daemons like yours 
and "qemu" value for Qemu backends).
3. For supporting additional configuration parameters for Qemu based 
virtio devices we could probably reuse "device_model_args" (although it 
is not clear to me what alternative to use for daemons).
Any other thoughts?

> Signed-off-by: Viresh Kumar <viresh.kumar@xxxxxxxxxx>
> ---
>   docs/man/xl.cfg.5.pod.in | 21 +++++++++++++++++++++
>   1 file changed, 21 insertions(+)
>
> diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
> index 31e58b73b0c9..1056b03df846 100644
> --- a/docs/man/xl.cfg.5.pod.in
> +++ b/docs/man/xl.cfg.5.pod.in
> @@ -1585,6 +1585,27 @@ Set maximum height for pointer device.
>   
>   =back
>   
> +=item B<virtio=[ "VIRTIO_DEVICE_STRING", "VIRTIO_DEVICE_STRING", ...]>
> +
> +Specifies the Virtio devices to be provided to the guest.
> +
> +Each B<VIRTIO_DEVICE_STRING> is a comma-separated list of C<KEY=VALUE>
> +settings from the following list:
> +
> +=over 4
> +
> +=item B<compatible=STRING>
Shouldn't it be "type" instead (the parsing code is looking for type and 
the example below suggests the type)?
> +
> +Specifies the compatible string for the specific Virtio device. The same will 
> be
> +written in the Device Tree compatible property of the Virtio device. For
> +example, "type=virtio,device22" for the I2C device > +
> +=item B<transport=STRING>
> +
> +Specifies the transport mechanism for the Virtio device, like "mmio" or "pci".
> +
> +=back
> +
>   =item B<tee="STRING">
>   
>   B<Arm only.> Set TEE type for the guest. TEE is a Trusted Execution


Also the commit description for #1/3 mentions that Virtio backend could 
run in any domain. So looks like the "backend" string is missing here. I 
would add the following:
=item B<backend=domain-id>

Specify the backend domain name or id, defaults to dom0.


P.S. I am wondering do i2c/gpio virtio backends support Xen grant 
mappings for the virtio? Have you tried to run the backends in 
non-hardware domain with CONFIG_XEN_VIRTIO=y in Linux?

[1]
https://www.kernel.org/doc/Documentation/devicetree/bindings/i2c/i2c-virtio.yaml
https://www.kernel.org/doc/Documentation/devicetree/bindings/gpio/gpio-virtio.yaml
[2]
https://www.kernel.org/doc/Documentation/devicetree/bindings/virtio/mmio.yaml