| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <4552e4b6-21a8-4829-16b4-7cda8ba0c0d1@epam.com>
Date: Thu, 23 Sep 2021 09:02:00 +0000
From: Oleksandr Andrushchenko <Oleksandr_Andrushchenko@...m.com>
To: Stefano Stabellini <sstabellini@...nel.org>,
"jgross@...e.com" <jgross@...e.com>
CC: "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>,
Oleksandr Andrushchenko <andr2000@...il.com>
Subject: Re: [PATCH 2/2] xen-pciback: prepare for the split for stub and PV
On 23.09.21 00:10, Stefano Stabellini wrote:
> On Wed, 22 Sep 2021, Oleksandr Andrushchenko wrote:
>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@...m.com>
>>
>> Currently PCI backend implements multiple functionalities at a time.
>> To name a few:
>> 1. it 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. it is used to hold the unbound PCI devices list, 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 for the devices being passed through
>> 4. para-virtualized use-cases support
>>
>> The para-virtualized part of the driver is not always needed as some
>> architectures, e.g. Arm or x86 PVH Dom0, are not using backend-frontend
>> model for PCI device passthrough. For such use-cases make the very
>> first step in splitting the xen-pciback driver into two parts: extended
>> PCI stub and PCI PV backend drivers. At the moment x86 platform will
>> continue using CONFIG_XEN_PCIDEV_BACKEND for the fully featured backend
>> driver and new platforms may build a driver with limited functionality
>> (no PV) by enabling CONFIG_XEN_PCIDEV_STUB.
>>
>> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@...m.com>
> Please make this the first patch of the series so that...
>
>> ---
>> New in v2
>> ---
>> drivers/xen/Kconfig | 26 +++++++++++++++++++++++++-
>> drivers/xen/Makefile | 2 +-
>> drivers/xen/xen-pciback/Makefile | 1 +
>> drivers/xen/xen-pciback/pciback.h | 5 +++++
>> drivers/xen/xen-pciback/xenbus.c | 6 +++++-
>> 5 files changed, 37 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/xen/Kconfig b/drivers/xen/Kconfig
>> index 057ddf61ef61..6e92c6be19f1 100644
>> --- a/drivers/xen/Kconfig
>> +++ b/drivers/xen/Kconfig
>> @@ -180,10 +180,34 @@ config SWIOTLB_XEN
>> select DMA_OPS
>> select SWIOTLB
>>
>> +config XEN_PCI_STUB
>> + bool
>> +
>> +config XEN_PCIDEV_STUB
>> + tristate "Xen PCI-device stub driver"
>> + depends on PCI && !X86 && XEN
>> + depends on XEN_BACKEND
>> + select XEN_PCI_STUB
>> + default m
>> + help
>> + The PCI device stub driver provides limited version of the PCI
>> + device backend driver without para-virtualized support for guests.
>> + If you select this to be a module, you will need to make sure no
>> + other driver has bound to the device(s) you want to make visible to
>> + other guests.
>> +
>> + The "hide" parameter (only applicable if backend driver is compiled
>> + into the kernel) allows you to bind the PCI devices to this module
>> + from the default device drivers. The argument is the list of PCI BDFs:
>> + xen-pciback.hide=(03:00.0)(04:00.0)
>> +
>> + If in doubt, say m.
>> +
>> config XEN_PCIDEV_BACKEND
>> tristate "Xen PCI-device backend driver"
>> - depends on PCI && XEN
>> + depends on PCI && X86 && XEN
>> depends on XEN_BACKEND
> ...we don't need this
Yes, I didn't like this change too
>
>
>> + select XEN_PCI_STUB
>> default m
>> help
>> The PCI device backend driver allows the kernel to export arbitrary
>> diff --git a/drivers/xen/Makefile b/drivers/xen/Makefile
>> index 3434593455b2..5aae66e638a7 100644
>> --- a/drivers/xen/Makefile
>> +++ b/drivers/xen/Makefile
>> @@ -24,7 +24,7 @@ obj-$(CONFIG_XEN_SYS_HYPERVISOR) += sys-hypervisor.o
>> obj-$(CONFIG_XEN_PVHVM_GUEST) += platform-pci.o
>> obj-$(CONFIG_SWIOTLB_XEN) += swiotlb-xen.o
>> obj-$(CONFIG_XEN_MCE_LOG) += mcelog.o
>> -obj-$(CONFIG_XEN_PCIDEV_BACKEND) += xen-pciback/
>> +obj-$(CONFIG_XEN_PCI_STUB) += xen-pciback/
>> obj-$(CONFIG_XEN_PRIVCMD) += xen-privcmd.o
>> obj-$(CONFIG_XEN_ACPI_PROCESSOR) += xen-acpi-processor.o
>> obj-$(CONFIG_XEN_EFI) += efi.o
>> diff --git a/drivers/xen/xen-pciback/Makefile b/drivers/xen/xen-pciback/Makefile
>> index e8d981d43235..e2cb376444a6 100644
>> --- a/drivers/xen/xen-pciback/Makefile
>> +++ b/drivers/xen/xen-pciback/Makefile
>> @@ -1,5 +1,6 @@
>> # SPDX-License-Identifier: GPL-2.0
>> obj-$(CONFIG_XEN_PCIDEV_BACKEND) += xen-pciback.o
>> +obj-$(CONFIG_XEN_PCIDEV_STUB) += xen-pciback.o
>>
>> xen-pciback-y := pci_stub.o pciback_ops.o xenbus.o
>> xen-pciback-y += conf_space.o conf_space_header.o \
>> diff --git a/drivers/xen/xen-pciback/pciback.h b/drivers/xen/xen-pciback/pciback.h
>> index 95e28ee48d52..9a64196e831d 100644
>> --- a/drivers/xen/xen-pciback/pciback.h
>> +++ b/drivers/xen/xen-pciback/pciback.h
>> @@ -71,6 +71,11 @@ struct pci_dev *pcistub_get_pci_dev(struct xen_pcibk_device *pdev,
>> struct pci_dev *dev);
>> void pcistub_put_pci_dev(struct pci_dev *dev);
>>
>> +static inline bool xen_pcibk_pv_support(void)
>> +{
>> + return IS_ENABLED(CONFIG_XEN_PCIDEV_BACKEND);
>> +}
>> +
>> /* Ensure a device is turned off or reset */
>> void xen_pcibk_reset_device(struct pci_dev *pdev);
>>
>> diff --git a/drivers/xen/xen-pciback/xenbus.c b/drivers/xen/xen-pciback/xenbus.c
>> index da34ce85dc88..bde63ef677b8 100644
>> --- a/drivers/xen/xen-pciback/xenbus.c
>> +++ b/drivers/xen/xen-pciback/xenbus.c
>> @@ -743,6 +743,9 @@ const struct xen_pcibk_backend *__read_mostly xen_pcibk_backend;
>>
>> int __init xen_pcibk_xenbus_register(void)
>> {
>> + if (!xen_pcibk_pv_support())
>> + return 0;
> Is this truly enough to stop the PV backend from initializing? Have you
> actually tested it to make sure? If it works, amazing! I am quite happy
> about this approach :-)
Well, I put some logs into the driver and saw nothing obvious pointing
to any backend activities (probably this is also because I don't have any
frontend). I see that the xenbus driver is not registered. In XenStore I see:
root@...0:~# xenstore-ls -f | grep pci
/local/domain/0/backend/pci = ""
/local/domain/0/backend/pci/2 = ""
/local/domain/0/backend/pci/2/0 = ""
/local/domain/0/backend/pci/2/0/frontend = "/local/domain/2/device/pci/0"
/local/domain/0/backend/pci/2/0/frontend-id = "2"
/local/domain/0/backend/pci/2/0/online = "1"
/local/domain/0/backend/pci/2/0/state = "1"
/local/domain/0/backend/pci/2/0/domain = "DomU"
/local/domain/0/backend/pci/2/0/key-0 = "0000:03:00.0"
/local/domain/0/backend/pci/2/0/dev-0 = "0000:03:00.0"
/local/domain/0/backend/pci/2/0/opts-0 = "msitranslate=0,power_mgmt=0,permissive=0,rdm_policy=strict"
/local/domain/0/backend/pci/2/0/state-0 = "1"
/local/domain/0/backend/pci/2/0/num_devs = "1"
/local/domain/2/device/pci = ""
/local/domain/2/device/pci/0 = ""
/local/domain/2/device/pci/0/backend = "/local/domain/0/backend/pci/2/0"
/local/domain/2/device/pci/0/backend-id = "0"
/local/domain/2/device/pci/0/state = "1"
/libxl/pci = ""
/libxl/pci/0000-03-00-0 = ""
/libxl/pci/0000-03-00-0/domid = "2"
But IIUIC these come from the toolstack
@Juergen, do you know how to check if the backend is indeed not running
or the above should be enough to prove?
>
>
>
>
>> xen_pcibk_backend = &xen_pcibk_vpci_backend;
>> if (passthrough)
>> xen_pcibk_backend = &xen_pcibk_passthrough_backend;
>> @@ -752,5 +755,6 @@ int __init xen_pcibk_xenbus_register(void)
>>
>> void __exit xen_pcibk_xenbus_unregister(void)
>> {
>> - xenbus_unregister_driver(&xen_pcibk_driver);
>> + if (xen_pcibk_pv_support())
>> + xenbus_unregister_driver(&xen_pcibk_driver);
>> }
>> --
>> 2.25.1
>>
Thank you,
Oleksandr
Powered by blists - more mailing lists