[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] xen-pciback: allow compiling on other archs than x86

  • To: Juergen Gross <jgross@xxxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>
  • From: Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>
  • Date: Tue, 21 Sep 2021 07:00:39 +0000
  • Accept-language: en-US
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=epam.com; dmarc=pass action=none header.from=epam.com; dkim=pass header.d=epam.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version; bh=armrUEo1vMEssjuJiia6IZ1nZsZE7fL6+asrzL9lEG4=; b=Qm75susJezlVxqJIhLDD2BMtGeiUb5JpYV00LiE0bzicejjDPw1USgCb275OhbEDRwodI/6EYg73DtspO6LrNuQ21txNg5Stz7XtWurd2qBxYd64X+dC86pXeqcujjm1vG6XI0R+RY1fJBcpnl/YYhi2S4BnR1qJn1Pg8/INHSus3ux6kCtHpMGOPB//WjvDV0qBdcEHMktUQJoPkwefrAWSHuIF8Z7AWV/3vpsnEm0K3C19rZlaabrpWesuzpDss08LEeruNJ6ikowjx0fMSCACj/EwTQVpUYXQu8S19O/uHTBtWxtG6qG3vu7usspgc5x2zvThPS/hYp2hUVEJ4g==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=c1MwUlmkvIQ8upGAfqnvvqjWkf3nc8H985l2bk4UwqzUwxMTuniCBfxtslZ+Vg59sPsGhJw5A6LG6u26NITS4PNBAgrUWkBdmWzK9aBpapjyUlMTBy5bnuv1sotYCBarNh0PyZAw2QuF/dgCWh76rVu+jLLugFp0lnYdeS5yfFEn2KcD8gSv12o7Lqid5mU4p2ehnKOo5ua82ccsdYS2vkjJFE8QqDYXqmEuJGWxhYSSNnXiHkZdn+zsDWCYn2TA+VJ0hqN//WOKG327YEqL+8vvPJIblMSO5cp0+DPAuaa6bzCkEdokVXyzsEcI1up7WTzoWOU9AjZ4pXN5Sa3fYA==
  • Authentication-results: suse.com; dkim=none (message not signed) header.d=none;suse.com; dmarc=none action=none header.from=epam.com;
  • Cc: "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>, "linux-kernel@xxxxxxxxxxxxxxx" <linux-kernel@xxxxxxxxxxxxxxx>, "boris.ostrovsky@xxxxxxxxxx" <boris.ostrovsky@xxxxxxxxxx>, "julien@xxxxxxx" <julien@xxxxxxx>, "jbeulich@xxxxxxxx" <jbeulich@xxxxxxxx>, Anastasiia Lukianenko <Anastasiia_Lukianenko@xxxxxxxx>, Oleksandr Andrushchenko <andr2000@xxxxxxxxx>
  • Delivery-date: Tue, 21 Sep 2021 07:00:51 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Thread-topic: [PATCH] xen-pciback: allow compiling on other archs than x86

On 21.09.21 09:49, Juergen Gross wrote:
> On 21.09.21 08:38, Oleksandr Andrushchenko wrote:
>> On 21.09.21 09:07, Juergen Gross wrote:
>>> On 21.09.21 07:51, Oleksandr Andrushchenko wrote:
>>>> On 21.09.21 08:20, Juergen Gross wrote:
>>>>> On 21.09.21 01: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.
>>>> As the first stage before the driver is split or ifdef's used - can we 
>>>> take the patch
>>>> as is now? In either way we chose this needs to be done, e.g. enable 
>>>> compiling
>>>> for other architectures and common code move.
>>> Fine with me in principle. I need to take a more thorough look
>>> at the patch, though.
>> Of course
>>>>>> 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?
>>>>> Uh no, not in this way, please. This will kill pci passthrough on x86
>>>>> with dom0 running as PVH. I don't think this is working right now, but
>>>>> adding more code making it even harder to work should be avoided.
>>>>>> 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);
>>>>> This hunk isn't needed, as with bailing out of xen_pcibk_xenbus_register
>>>>> early will result in xen_pcibk_xenbus_probe never being called.
>>>>>>         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;
>>>>>> +
>>>>> Use #ifdef CONFIG_X86 instead.
>>>> The title of this patch says that we want to allow this driver for other 
>>>> archs
>>>> and now we want to introduce "#ifdef CONFIG_X86" which doesn't sound
>>>> right with that respect. Instead, we may want having something like a
>>>> dedicated gate for this, e.g. "#ifdef CONFIG_XEN_PCIDEV_BACKEND_SUPP_PV"
>>>> or something which is architecture agnostic.
>>> Something like that, yes. But I'd rather use CONFIG_XEN_PCIDEV_BACKEND
>>> acting as this gate and introduce CONFIG_XEN_PCI_STUB for the stub
>>> functionality needed on Arm. XEN_PCIDEV_BACKEND would depend on X86 and
>>> select XEN_PCI_STUB, while on Arm XEN_PCI_STUB could be configured if
>>> wanted. The splitting of the driver can still be done later.
>> Hm, pciback is now compiled when CONFIG_XEN_PCIDEV_BACKEND  is enabled
>> and we want to skip some parts of its code when CONFIG_XEN_PCI_STUB is set.
>> So, I imagine that for x86 we just enable CONFIG_XEN_PCIDEV_BACKEND and the
>> driver compiles in its current state. For Arm we enable both 
>> and CONFIG_XEN_PCI_STUB, so part of the driver is not compiled.
> No, I'd rather switch to compiling xen-pciback when CONFIG_XEN_PCI_STUB
> is set and compile only parts of it when CONFIG_XEN_PCIDEV_BACKEND is
> not set (this will be the case on Arm).

But this will require that the existing kernel configurations out there have to 

define CONFIG_XEN_PCI_STUB to get what they had before with simply enabling

CONFIG_XEN_PCIDEV_BACKEND. My point was that it is probably desirable not to 

the things while doing the split/re-work.

I also thought that "compile only parts of it when CONFIG_XEN_PCIDEV_BACKEND is 
not set"

may have more code gated rather than with gating unwanted code with 

I am not quite sure about this though.

> This is another step in the right direction preparing the split.
> But as said before, this is not a requirement by me to take your patch.
Thank you
> Juergen



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.