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. > >>> >>> 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 >>>   #include >>>   #include >>> +#include >>>   #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. > 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. Juergen