Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 20 Sep 2021 16:16:06 -0700 (PDT)
From:   Stefano Stabellini <sstabellini@kernel.org>
To:     Oleksandr Andrushchenko <Oleksandr_Andrushchenko@epam.com>
cc:     Juergen Gross <jgross@suse.com>,
        Stefano Stabellini <sstabellini@kernel.org>,
        "xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "boris.ostrovsky@oracle.com" <boris.ostrovsky@oracle.com>,
        "julien@xen.org" <julien@xen.org>,
        "jbeulich@suse.com" <jbeulich@suse.com>,
        Anastasiia Lukianenko <Anastasiia_Lukianenko@epam.com>,
        Oleksandr Andrushchenko <andr2000@gmail.com>
Subject: Re: [PATCH] xen-pciback: allow compiling on other archs than x86
In-Reply-To: <7f873e38-0362-1f60-7347-a490c9dc8572@epam.com>
Message-ID: <alpine.DEB.2.21.2109201444040.17979@sstabellini-ThinkPad-T480s>
References: <20210917130123.1764493-1-andr2000@gmail.com> <alpine.DEB.2.21.2109171442070.21985@sstabellini-ThinkPad-T480s> <d81486bc-9a2b-8675-ba4d-828d3adc75fc@epam.com> <35e2e36a-bade-d801-faa1-c9953678bb9d@suse.com>
 <7f873e38-0362-1f60-7347-a490c9dc8572@epam.com>
User-Agent: Alpine 2.21 (DEB 202 2017-01-01)
MIME-Version: 1.0
Content-Type: multipart/mixed; BOUNDARY="8323329-1040504615-1632174404=:17979"
Content-ID: <alpine.DEB.2.21.2109201454550.17979@sstabellini-ThinkPad-T480s>
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-1040504615-1632174404=:17979
Content-Type: text/plain; CHARSET=UTF-8
Content-Transfer-Encoding: 8BIT
Content-ID: <alpine.DEB.2.21.2109201454551.17979@sstabellini-ThinkPad-T480s>

On Mon, 20 Sep 2021, Oleksandr Andrushchenko wrote:
> On 20.09.21 14:30, Juergen Gross wrote:
> > On 20.09.21 07:23, Oleksandr Andrushchenko wrote:
> >> Hello, Stefano!
> >>
> >> On 18.09.21 00:45, Stefano Stabellini wrote:
> >>> Hi Oleksandr,
> >>>
> >>> Why do you want to enable pciback on ARM? Is it only to "disable" a PCI
> >>> device in Dom0 so that it can be safely assigned to a DomU?
> >> Not only that
> >>>
> >>> I am asking because actually I don't think we want to enable the PV PCI
> >>> backend feature of pciback on ARM, right? That would clash with the PCI
> >>> assignment work you have been doing in Xen. They couldn't both work at
> >>> the same time.
> >> Correct, it is not used
> >>>
> >>> If we only need pciback to "park" a device in Dom0, wouldn't it be
> >>> possible and better to use pci-stub instead?
> >>
> >> Not only that, so pci-stub is not enough
> >>
> >> The functionality which is implemented by the pciback and the toolstack
> >> and which is relevant/missing/needed for ARM:
> >>
> >> 1. pciback is used as a database for assignable PCI devices, e.g. xl
> >>      pci-assignable-{add|remove|list} manipulates that list. So, whenever the
> >>      toolstack needs to know which PCI devices can be passed through it reads
> >>      that from the relevant sysfs entries of the pciback.
> >>
> >> 2. pciback is used to hold the unbound PCI devices, e.g. when passing through
> >>      a PCI device it needs to be unbound from the relevant device driver and bound
> >>      to pciback (strictly speaking it is not required that the device is bound to
> >>      pciback, but pciback is again used as a database of the passed through PCI
> >>      devices, so we can re-bind the devices back to their original drivers when
> >>      guest domain shuts down)
> >>
> >> 3. Device reset
> >>
> >> We have previously discussed on xen-devel ML possible solutions to that as from the
> >> above we see that pciback functionality is going to be only partially used on Arm.
> >>
> >> Please see [1] and [2]:
> >>
> >> 1. It is not acceptable to manage the assignable list in Xen itself
> >>
> >> 2. pciback can be split into two parts: PCI assignable/bind/reset handling and
> >> the rest like vPCI etc.
> >>
> >> 3. pcifront is not used on Arm
> >
> > It is neither in x86 PVH/HVM guests.
> Didn't know that, thank you for pointing
> >
> >> So, limited use of the pciback is one of the bricks used to enable PCI passthrough
> >> on Arm. It was enough to just re-structure the driver and have it run on Arm to achieve
> >> all the goals above.
> >>
> >> If we still think it is desirable to break the pciback driver into "common" and "pcifront specific"
> >> parts then it can be done, yet the patch is going to be the very first brick in that building.
> >
> > Doing this split should be done, as the pcifront specific part could be
> > omitted on x86, too, in case no PV guests using PCI passthrough have to
> > be supported.
> Agree, that the final solution should have the driver split
> >
> >> So, I think this patch is still going to be needed besides which direction we take.
> >
> > Some kind of this patch, yes. It might look different in case the split
> > is done first.
> >
> > I don't mind doing it in either sequence.
> >
> With this patch we have Arm on the same page as the above mentioned x86 guests,
> 
> e.g. the driver has unused code, but yet allows Arm to function now.
> 
> At this stage of PCI passthrough on Arm it is yet enough. Long term, when
> 
> the driver gets split, Arm will benefit from that split too, but unfortunately I do not
> 
> have enough bandwidth for that piece of work at the moment.

That's fair and I don't want to scope-creep this simple patch asking for
an enormous rework. At the same time I don't think we should enable the
whole of pciback on ARM because it would be erroneous and confusing.

I am wonder if there is a simple:

if (!xen_pv_domain())
    return;

That we could add in a couple of places in pciback to stop it from
initializing the parts we don't care about. Something along these lines
(untested and probably incomplete).

What do you guys think?


diff --git a/drivers/xen/xen-pciback/xenbus.c b/drivers/xen/xen-pciback/xenbus.c
index da34ce85dc88..991ba0a9b359 100644
--- a/drivers/xen/xen-pciback/xenbus.c
+++ b/drivers/xen/xen-pciback/xenbus.c
@@ -15,6 +15,7 @@
 #include <xen/xenbus.h>
 #include <xen/events.h>
 #include <xen/pci.h>
+#include <xen/xen.h>
 #include "pciback.h"
 
 #define INVALID_EVTCHN_IRQ  (-1)
@@ -685,8 +686,12 @@ static int xen_pcibk_xenbus_probe(struct xenbus_device *dev,
 				const struct xenbus_device_id *id)
 {
 	int err = 0;
-	struct xen_pcibk_device *pdev = alloc_pdev(dev);
+	struct xen_pcibk_device *pdev;
+
+	if (!xen_pv_domain())
+		return 0;
 
+	pdev = alloc_pdev(dev);
 	if (pdev == NULL) {
 		err = -ENOMEM;
 		xenbus_dev_fatal(dev, err,
@@ -743,6 +748,9 @@ const struct xen_pcibk_backend *__read_mostly xen_pcibk_backend;
 
 int __init xen_pcibk_xenbus_register(void)
 {
+	if (!xen_pv_domain())
+		return 0;
+
 	xen_pcibk_backend = &xen_pcibk_vpci_backend;
 	if (passthrough)
 		xen_pcibk_backend = &xen_pcibk_passthrough_backend;
@@ -752,5 +760,7 @@ int __init xen_pcibk_xenbus_register(void)
 
 void __exit xen_pcibk_xenbus_unregister(void)
 {
+	if (!xen_pv_domain())
+		return;
 	xenbus_unregister_driver(&xen_pcibk_driver);
 }
--8323329-1040504615-1632174404=:17979--