| Message ID | 1649963973-22879-7-git-send-email-olekstysh@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | virtio: Solution to restrict memory access under Xen using xen-virtio DMA ops layer | expand |
On Thu, 14 Apr 2022, Oleksandr Tyshchenko wrote: > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > > Call xen_virtio_setup_dma_ops() only for Xen-aware virtio devices > in Xen guests if restricted access to the guest memory is enabled. > > Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > --- > include/xen/arm/xen-ops.h | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/include/xen/arm/xen-ops.h b/include/xen/arm/xen-ops.h > index 621da05..28b2ad3 100644 > --- a/include/xen/arm/xen-ops.h > +++ b/include/xen/arm/xen-ops.h > @@ -2,12 +2,19 @@ > #ifndef _ASM_ARM_XEN_OPS_H > #define _ASM_ARM_XEN_OPS_H > > +#include <linux/virtio_config.h> > #include <xen/swiotlb-xen.h> > +#include <xen/xen-ops.h> > > static inline void xen_setup_dma_ops(struct device *dev) > { > if (xen_swiotlb_detect()) > dev->dma_ops = &xen_swiotlb_dma_ops; > + > +#ifdef CONFIG_XEN_VIRTIO > + if (arch_has_restricted_virtio_memory_access() && xen_is_virtio_device(dev)) > + xen_virtio_setup_dma_ops(dev); > +#endif This makes sense overall. Considering that the swiotlb-xen case and the virtio case are mutually exclusive, I would write it like this: if (arch_has_restricted_virtio_memory_access() && xen_is_virtio_device(dev)) xen_virtio_setup_dma_ops(dev); else if (xen_swiotlb_detect()) dev->dma_ops = &xen_swiotlb_dma_ops; There is no need for #ifdef (also see other comments).
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?
On 16.04.22 01:02, Stefano Stabellini wrote: Hello Stefano > On Thu, 14 Apr 2022, Oleksandr Tyshchenko wrote: >> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >> >> Call xen_virtio_setup_dma_ops() only for Xen-aware virtio devices >> in Xen guests if restricted access to the guest memory is enabled. >> >> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >> --- >> include/xen/arm/xen-ops.h | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/include/xen/arm/xen-ops.h b/include/xen/arm/xen-ops.h >> index 621da05..28b2ad3 100644 >> --- a/include/xen/arm/xen-ops.h >> +++ b/include/xen/arm/xen-ops.h >> @@ -2,12 +2,19 @@ >> #ifndef _ASM_ARM_XEN_OPS_H >> #define _ASM_ARM_XEN_OPS_H >> >> +#include <linux/virtio_config.h> >> #include <xen/swiotlb-xen.h> >> +#include <xen/xen-ops.h> >> >> static inline void xen_setup_dma_ops(struct device *dev) >> { >> if (xen_swiotlb_detect()) >> dev->dma_ops = &xen_swiotlb_dma_ops; >> + >> +#ifdef CONFIG_XEN_VIRTIO >> + if (arch_has_restricted_virtio_memory_access() && xen_is_virtio_device(dev)) >> + xen_virtio_setup_dma_ops(dev); >> +#endif > This makes sense overall. thank you > Considering that the swiotlb-xen case and the > virtio case are mutually exclusive, I would write it like this: > > if (arch_has_restricted_virtio_memory_access() && xen_is_virtio_device(dev)) > xen_virtio_setup_dma_ops(dev); > else if (xen_swiotlb_detect()) > dev->dma_ops = &xen_swiotlb_dma_ops; Agree, will do > > There is no need for #ifdef (also see other comments). Agree, if !CONFIG_XEN_VIRTIO then xen_virtio_ are just stubs. #ifdef CONFIG_XEN_VIRTIO void xen_virtio_setup_dma_ops(struct device *dev); bool xen_is_virtio_device(struct device *dev); #else static inline void xen_virtio_setup_dma_ops(struct device *dev) { } static inline bool xen_is_virtio_device(struct device *dev) { return false; } #endif /* CONFIG_XEN_VIRTIO */
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? In Xen system: - the grants are not used for "non-virtualized" devices at all (platform devices for the passthrough). - the grants are widely used for "virtualized, but non-virtio" devices (traditional Xen PV devices), but grants for these Xen PV devices are used in a different way and *not* at the DMA ops level like in current approach Or I misunderstood your question? This patch series tries to make things work with "virtio" devices in Xen system without introducing any modifications to code under drivers/virtio. We could avoid having virtio hooks in the arch DMA code, but we need to trigger setting xen-virtio DMA ops for the virtio device from some other place. For example, the following code would also work, but requires altering virtio_mmio_probe(): diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c index 56128b9..8f48491 100644 --- a/drivers/virtio/virtio_mmio.c +++ b/drivers/virtio/virtio_mmio.c @@ -615,6 +615,9 @@ static int virtio_mmio_probe(struct platform_device *pdev) DMA_BIT_MASK(32 + PAGE_SHIFT)); } else { rc = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64)); + + if (arch_has_restricted_virtio_memory_access()) + xen_virtio_setup_dma_ops(&pdev->dev); } if (rc) rc = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32)); Another possible option could be to introduce local init function in drivers/xen/xen-virtio.c to scan the device tree and set xen-virtio DMA ops for all devices with the "xen,dev-domid" property. What do you think?
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. 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 - rename xen_virtio_setup_dma_ops to something more generic, like xen_grants_setup_dma_ops And that's pretty much it.
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 > - 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?
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. >> - 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. Maybe rename xen-virtio.c to grant-dma.c? 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. CONFIG_XEN_VIRTIO should still guard xen_has_restricted_virtio_memory_access(). Juergen
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
On Tue, 19 Apr 2022, Oleksandr wrote: > 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. I would probably add it to the existing Documentation/devicetree/bindings/arm/xen.txt. > 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? Yes I think so
Hello Stefano, Juergen On 20.04.22 03:23, Stefano Stabellini wrote: > On Tue, 19 Apr 2022, Oleksandr wrote: >> 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. > I would probably add it to the existing > Documentation/devicetree/bindings/arm/xen.txt. > > >> 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? > Yes I think so Stefano, thank you for clarifying! Regarding new naming scheme... As there is an existing Kconfig option XEN_GRANT_DMA_ALLOC used for different purpose, we need to clarify naming scheme here a bit to avoid possible confusion. For example, I am happy with proposed by Juergen ... ... Kconfig option: XEN_GRANT_DMA_OPS and ... file: grant-dma-ops.c
On Wed, 20 Apr 2022, Oleksandr wrote: > On 20.04.22 03:23, Stefano Stabellini wrote: > > On Tue, 19 Apr 2022, Oleksandr wrote: > > > 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. > > I would probably add it to the existing > > Documentation/devicetree/bindings/arm/xen.txt. > > > > > > > 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? > > Yes I think so > > > Stefano, thank you for clarifying! > > > Regarding new naming scheme... > > As there is an existing Kconfig option XEN_GRANT_DMA_ALLOC used for different > purpose, we need to clarify naming scheme here a bit to avoid possible > confusion. > > For example, I am happy with proposed by Juergen ... > > ... Kconfig option: XEN_GRANT_DMA_OPS > > and > > ... file: grant-dma-ops.c I think that's fine by me
diff --git a/include/xen/arm/xen-ops.h b/include/xen/arm/xen-ops.h index 621da05..28b2ad3 100644 --- a/include/xen/arm/xen-ops.h +++ b/include/xen/arm/xen-ops.h @@ -2,12 +2,19 @@ #ifndef _ASM_ARM_XEN_OPS_H #define _ASM_ARM_XEN_OPS_H +#include <linux/virtio_config.h> #include <xen/swiotlb-xen.h> +#include <xen/xen-ops.h> static inline void xen_setup_dma_ops(struct device *dev) { if (xen_swiotlb_detect()) dev->dma_ops = &xen_swiotlb_dma_ops; + +#ifdef CONFIG_XEN_VIRTIO + if (arch_has_restricted_virtio_memory_access() && xen_is_virtio_device(dev)) + xen_virtio_setup_dma_ops(dev); +#endif } #endif /* _ASM_ARM_XEN_OPS_H */