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

[Oleksandr <olekstysh@gmail.com>]

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


-- 
Regards,

Oleksandr Tyshchenko