[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 06:38:43 +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=H9hg44KZ9/LdNjWTbRUEVjLA25bXz8mez1mEnkRsLVo=; b=aOIPG+o2nN6/J/WOpOeZ7et2ZlFdBHK/CSko2apd8t23CJDevJ7q3TbCDbo04TUWgQmEPcB4aV9ZiSO6g13Qxw3bLEmjCf85S8QftWk5aeyAnLAY6kPwHY0ZcoBjc9bZ4YLdxmql1TSwQANLuMEF8ifjs2WbBzdHPlKI18+p8e7SGh9Bq6QXCUYS9IFzrnzEDul+Dnnr9ORziKFQQQqiaARd1u1OY5998svwL/h22ckfdONTLtQbyLrB8oOhASNKwk75vzBY3LiF68ZbqQNAI7rJZiL/YWAlF1WL4197ZZ79yS9rDzAL+W6FnEIooEYW/XyQedEeZ7nq2nISbU+1wA==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=NDS3GMioLO+zUJ1AyP1O2jSfAYvcjMwOUNfsNijc/Mh31LpLaqtKbJQ9MDdAMuc9yvkuja2/eyK6kioYRPT0mQRIpfVHFSny31hbVQeQ1t0BZtU9boXHBsWwb8E+inGiwBQEGfjy/mIsyzXt6fGKhlIixTaivwpVv+q0FQZSqWk4ixG/12I6+JpyGgnISFiduvimi2tgm90z/8VWWjma19ZWAXPAPDiOP1nInZqZVKS+1XFf3JOBl8EPOdXQGFMLukEIoYVz8EjsV0Z3EvUBEp2HrdwQGhUmy7bRjVQFic5Zh2CxG0jgfa1bHu/acK9fDXBAmEqBhzvrsVz6IhSy+Q==
  • 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 06:39:11 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Thread-index: AQHXq8QnOfGIukpGqE2hMWXmS9bdmKuow0KAgAOkhICAAGZ9AIAAAmOAgADCywCAAGXhAIAACLuAgAAEUwCAAAi6AA==
  • Thread-topic: [PATCH] xen-pciback: allow compiling on other archs than x86

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 
CONFIG_XEN_PCIDEV_BACKEND

and CONFIG_XEN_PCI_STUB, so part of the driver is not compiled.


>
>> Gating also means that we are not thinking about splitting the backend 
>> driver into
>> two different ones, e.g. one for "common" code and one for PV stuff.
>> Otherwise this ifdefery won't be needed.
>
> I just wanted to avoid the xen_pv_domain() tests creeping in, as
> they are wrong IMO.
>
I understand that
>
> Juergen

 


Rackspace

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