Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 20 Apr 2022 15:49:38 -0700 (PDT)
From:   Stefano Stabellini <sstabellini@kernel.org>
To:     Oleksandr <olekstysh@gmail.com>
cc:     Stefano Stabellini <sstabellini@kernel.org>,
        Juergen Gross <jgross@suse.com>,
        Christoph Hellwig <hch@infradead.org>,
        xen-devel@lists.xenproject.org, linux-kernel@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org,
        Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>,
        Boris Ostrovsky <boris.ostrovsky@oracle.com>,
        Julien Grall <julien@xen.org>,
        "Michael S. Tsirkin" <mst@redhat.com>
Subject: Re: [RFC PATCH 6/6] arm/xen: Assign xen-virtio DMA ops for virtio
 devices in Xen guests
In-Reply-To: <7047c7a7-47a0-d446-3b99-94bd552ec90f@gmail.com>
Message-ID: <alpine.DEB.2.22.394.2204201549270.915916@ubuntu-linux-20-04-desktop>
References: <1649963973-22879-1-git-send-email-olekstysh@gmail.com> <1649963973-22879-7-git-send-email-olekstysh@gmail.com> <alpine.DEB.2.22.394.2204151305050.915916@ubuntu-linux-20-04-desktop> <YlpdBHKT1bYzZe2e@infradead.org> <f879622e-5656-deb1-1930-f0cd180a4ab1@gmail.com>
 <alpine.DEB.2.22.394.2204181202080.915916@ubuntu-linux-20-04-desktop> <6a04cc34-fbb3-44d8-c1a4-03bda5b3deb1@gmail.com> <b68163be-ad43-7773-22ff-e83191886626@suse.com> <5afb9e61-4164-9cc9-278a-911fc21f4f6c@gmail.com> <alpine.DEB.2.22.394.2204191717020.915916@ubuntu-linux-20-04-desktop>
 <7047c7a7-47a0-d446-3b99-94bd552ec90f@gmail.com>
User-Agent: Alpine 2.22 (DEB 394 2020-01-19)
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="8323329-1919739536-1650494979=:915916"
Precedence: bulk

  This message is in MIME format.  The first part should be readable text,
  while the remaining parts are likely unreadable without MIME-aware tools.

--8323329-1919739536-1650494979=:915916
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8BIT

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
--8323329-1919739536-1650494979=:915916--