diff mbox series

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

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

Commit Message

Oleksandr Tyshchenko April 14, 2022, 7:19 p.m. UTC
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(+)

Comments

Stefano Stabellini April 15, 2022, 10:02 p.m. UTC | #1
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).
Christoph Hellwig April 16, 2022, 6:07 a.m. UTC | #2
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?
Oleksandr Tyshchenko April 17, 2022, 7:20 p.m. UTC | #3
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 */
Oleksandr Tyshchenko April 17, 2022, 9:05 p.m. UTC | #4
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?
Stefano Stabellini April 18, 2022, 7:11 p.m. UTC | #5
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.
Oleksandr Tyshchenko April 19, 2022, 12:17 p.m. UTC | #6
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?
Juergen Gross April 19, 2022, 2:48 p.m. UTC | #7
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
Oleksandr Tyshchenko April 19, 2022, 5:11 p.m. UTC | #8
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
Stefano Stabellini April 20, 2022, 12:23 a.m. UTC | #9
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
Oleksandr Tyshchenko April 20, 2022, 9 a.m. UTC | #10
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
Stefano Stabellini April 20, 2022, 10:49 p.m. UTC | #11
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 mbox series

Patch

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 */