Re: [RFC PATCH 6/6] arm/xen: Assign xen-virtio DMA ops for virtio devices in Xen guests

[Oleksandr <olekstysh@xxxxxxxxx>]

Hello Stefano, Juergen


On 19.04.22 17:48, Juergen Gross wrote:
> On 19.04.22 14:17, Oleksandr wrote:
> > Hello Stefano, Juergen
> >
> >
> > On 18.04.22 22:11, Stefano Stabellini wrote:
> > > On Mon, 18 Apr 2022, Oleksandr wrote:
> > > > On 16.04.22 09:07, Christoph Hellwig wrote:
> > > >
> > > > Hello Christoph
> > > >
> > > > > On Fri, Apr 15, 2022 at 03:02:45PM -0700, Stefano Stabellini wrote:
> > > > > > This makes sense overall. Considering that the swiotlb-xen case 
> > > > > > and the
> > > > > > virtio case are mutually exclusive, I would write it like this:
> > > > > Curious question:  Why can't the same grant scheme also be used for
> > > > > non-virtio devices?  I really hate having virtio hooks in the arch
> > > > > dma code.  Why can't Xen just say in DT/ACPI that grants can be used
> > > > > for a given device?
> > > [...]
> > >
> > > > This patch series tries to make things work with "virtio" devices 
> > > > in Xen
> > > > system without introducing any modifications to code under 
> > > > drivers/virtio.
> > > Actually, I think Christoph has a point.
> > >
> > > There is nothing inherently virtio specific in this patch series or in
> > > the "xen,dev-domid" device tree binding.
> >
> > Although the main intention of this series was to enable using virtio 
> > devices in Xen guests, I agree that nothing in new DMA ops layer 
> > (xen-virtio.c) is virtio specific (at least at the moment). Regarding 
> > the whole patch series I am not quite sure, as it uses 
> > arch_has_restricted_virtio_memory_access(). >
> > >   Assuming a given device is
> > > emulated by a Xen backend, it could be used with grants as well.
> > >
> > > For instance, we could provide an emulated e1000 NIC with a
> > > "xen,dev-domid" property in device tree. Linux could use grants with it
> > > and the backend could map the grants. It would work the same way as
> > > virtio-net/block/etc. Passthrough devices wouldn't have the
> > > "xen,dev-domid" property, so no problems.
> > >
> > > So I think we could easily generalize this work and expand it to any
> > > device. We just need to hook on the "xen,dev-domid" device tree
> > > property.
> > >
> > > I think it is just a matter of:
> > > - remove the "virtio,mmio" check from xen_is_virtio_device
> > > - rename xen_is_virtio_device to something more generic, like
> > >    xen_is_grants_device
> xen_is_grants_dma_device, please. Normal Xen PV devices are covered by
> grants, too, and I'd like to avoid the confusion arising from this.

yes, this definitely makes sense as we need to distinguish


> > > - rename xen_virtio_setup_dma_ops to something more generic, like
> > >    xen_grants_setup_dma_ops
> > >
> > > And that's pretty much it.
> > + likely renaming everything in that patch series not to mention 
> > virtio (mostly related to xen-virtio.c internals).
> >
> > Stefano, thank you for clarifying Christoph's point.
> >
> > Well, I am not against going this direction. Could we please make a 
> > decision on this? @Juergen, what is your opinion?
> Yes, why not.

ok, thank you for confirming.


> Maybe rename xen-virtio.c to grant-dma.c?

Personally I don't mind.


> I'd keep the XEN_VIRTIO related config option, as this will be the 
> normal use
> case. grant-dma.c should be covered by a new hidden config option 
> XEN_GRANT_DMA
> selected by XEN_VIRTIO.

I got it, ok


> CONFIG_XEN_VIRTIO should still guard 
> xen_has_restricted_virtio_memory_access().

ok


So a few questions to clarify:

1. What is the best place to keep "xen,dev-domid" binding's description 
now? I think that proposed in current series place 
(Documentation/devicetree/bindings/virtio/) is not good fit now.
2. I assume the logic in the current patch will remain the same, I mean 
we will still assign Xen grant DMA ops from xen_setup_dma_ops() here?

> Juergen
--
Regards,

Oleksandr Tyshchenko