Message-ID: <ddb3a17a-1fd2-7108-20ad-73487a308954@epam.com>
Date:   Tue, 21 Sep 2021 04:51:36 +0000
From:   Oleksandr Andrushchenko <Oleksandr_Andrushchenko@...m.com>
To:     Stefano Stabellini <sstabellini@...nel.org>
CC:     Juergen Gross <jgross@...e.com>,
        "xen-devel@...ts.xenproject.org" <xen-devel@...ts.xenproject.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "boris.ostrovsky@...cle.com" <boris.ostrovsky@...cle.com>,
        "julien@....org" <julien@....org>,
        "jbeulich@...e.com" <jbeulich@...e.com>,
        Anastasiia Lukianenko <Anastasiia_Lukianenko@...m.com>,
        Oleksandr Andrushchenko <andr2000@...il.com>
Subject: Re: [PATCH] xen-pciback: allow compiling on other archs than x86

Hi, Stefano!

On 21.09.21 02:16, Stefano Stabellini wrote:
> 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?

I think that it needs to be an additional patch and the PV check seems

reasonable to me. We need to check if gating only part of the initialization

with xen_pv_domain is just enough, e.g. if the rest of the code is ok that

something was not initialized and won't be touched at run-time.

Let's see what other think about the approach

>
>
> 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);
>   }

Powered by blists - more mailing lists