On Thu, 14 Apr 2022, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko <[email protected]>
>
> 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 <[email protected]>
> ---
> 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 <[email protected]>
>>
>> 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 <[email protected]>
>> ---
>> 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 */
--
Regards,
Oleksandr Tyshchenko
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?
--
Regards,
Oleksandr Tyshchenko
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.