| Message ID | 24a0278313ea9a9e6c093880dead835184025734.1667906228.git.viresh.kumar@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Virtio toolstack support for I2C and GPIO on Arm | expand |
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@linaro.org> > --- > 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
On 04-12-22, 20:52, Oleksandr Tyshchenko wrote: > 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). For now, yes. But we may want to link these devices with other devices in DT, like GPIO line consumers. I am not pushing a half informed solution for that right now and that can be taken up later. > 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). I agree. Maybe we can use "virtio,device" without a number for the device in this case. > 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). Hmm, I realize now that my patch did define a new type for this, libxl_virtio_backend, which defines STANDALONE already, but it isn't used currently. Maybe I should remove it too. And I am not sure sure how to use these values, STANDALONE or QEMU. Should the DT nodes be created only for STANDALONE and never for QEMU ? Maybe we can add these fields and a config param, once someone wants to reuse this stuff for QEMU ? > 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). I would leave it for the person who will make use of this eventually, as then we will have more information on the same. > > +=item B<compatible=STRING> > > Shouldn't it be "type" instead (the parsing code is looking for type and the > example below suggests the type)? Yes. > > +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. I haven't used the backend in any other domain for now, just Dom0, but the idea is definitely there to run backends in separate user domains. > P.S. I am wondering do i2c/gpio virtio backends support Xen grant mappings > for the virtio? Not yet, we haven't made much progress in that area until now, but it is very much part of what we intend to do. > Have you tried to run the backends in non-hardware domain > with CONFIG_XEN_VIRTIO=y in Linux? Not yet.
On 05.12.22 11:11, Viresh Kumar wrote: Hello Viresh > On 04-12-22, 20:52, Oleksandr Tyshchenko wrote: >> 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). > > For now, yes. But we may want to link these devices with other devices > in DT, like GPIO line consumers. I am not pushing a half informed > solution for that right now and that can be taken up later. I got it, ok. > >> 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). > > I agree. Maybe we can use "virtio,device" without a number for the > device in this case. Fine with me. > >> 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). > > Hmm, I realize now that my patch did define a new type for this, > libxl_virtio_backend, which defines STANDALONE already, but it isn't > used currently. Maybe I should remove it too. > > And I am not sure sure how to use these values, STANDALONE or QEMU. > Should the DT nodes be created only for STANDALONE and never for QEMU > ? If we expose virtio-mmio device to the guest via device-tree on Arm, then I think the DT nodes should be always created here, no matter where the corresponding virtio backend is located itself (either STANDALONE or QEMU). > > Maybe we can add these fields and a config param, once someone wants > to reuse this stuff for QEMU ? I don't know what to suggest here, sorry. On the one hand, it is an extra work for you trying to add functionality you don't need at the moment. On the other hand if we add "backendtype" config param right now with default to STANDALONE it might simplify work for someone who ends up adding other type (in particular, the QEMU). Let's see what the maintainers will say. > >> 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). > > I would leave it for the person who will make use of this eventually, > as then we will have more information on the same. Sure, these are just thoughts for now. > >>> +=item B<compatible=STRING> >> >> Shouldn't it be "type" instead (the parsing code is looking for type and the >> example below suggests the type)? > > Yes. > >>> +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. > > I haven't used the backend in any other domain for now, just Dom0, but > the idea is definitely there to run backends in separate user domains. ok, good. My point is the following: if backend domain is configurable then it should be documented here. > >> P.S. I am wondering do i2c/gpio virtio backends support Xen grant mappings >> for the virtio? > > Not yet, we haven't made much progress in that area until now, but it > is very much part of what we intend to do. Thanks for the information. > >> Have you tried to run the backends in non-hardware domain >> with CONFIG_XEN_VIRTIO=y in Linux? > > Not yet. ok >
On 06-12-22, 17:53, Oleksandr Tyshchenko wrote: > On 05.12.22 11:11, Viresh Kumar wrote: > > Maybe we can add these fields and a config param, once someone wants > > to reuse this stuff for QEMU ? > > > I don't know what to suggest here, sorry. > > On the one hand, it is an extra work for you trying to add functionality you > don't need at the moment. On the other hand if we add "backendtype" config > param right now with default to STANDALONE it might simplify work for > someone who ends up adding other type (in particular, the QEMU). Let's see > what the maintainers will say. The extra work is minimal and that isn't something that worries me. What worries me, based on past experience, is adding code that _may_ be required in future, and is never used eventually. And for that I always vouch for not adding something unless we are sure we need it and there is some code which makes use of that configuration right away.
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> + +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
This patch updates xl.cfg man page with details of generic Virtio device related information. Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> --- docs/man/xl.cfg.5.pod.in | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+)