On 6/17/2021 2:56 AM, Jason Gunthorpe wrote:
On Thu, Jun 17, 2021 at 02:51:09AM +0300, Max Gurtovoy wrote:
On 6/17/2021 2:44 AM, Jason Gunthorpe wrote:
On Thu, Jun 17, 2021 at 02:42:46AM +0300, Max Gurtovoy wrote:
Do you see a reason not adding this alias for stub drivers but
adding it to vfio_pci drivers ?
It creates uABI without a userspace user and that is strongly
discouraged. The 'stub_pci:' prefix becomes fixed ABI.
so is it better to have "pci:v*d*sv*sd*bc*sc*i*" for stub drivers ?
No, we don't want to convey any new information about stub drivers to
userspace.
or not adding alias at all if stub flag is set ?
Yes, just don't change it at all, IMHO.
Ok.
I've attached 2 patches that I would like to agree on before we'll send
the V5.
They include the pci-pf-stub additions and keep searching for matching
static ids as we discussed.
Max.
Jason
From 67be1e3a6525c73fa582e8f5aa703b982d6d8114 Mon Sep 17 00:00:00 2001
From: Max Gurtovoy <mgurtovoy@xxxxxxxxxx>
Date: Thu, 13 May 2021 13:53:19 +0300
Subject: [PATCH 1/2] PCI: add flags field to pci_device_id structure
This field will be used to allow pci modules to set some specific data
that can be used for matching, aliases and other hints. Add example for
"driver_override" pci drivers that may want to set a special prefix in
the modules.alias table. In the future, this flag will enforce
"driver_override" to work on drivers that specifically opt into this
feature. In this case, the udev utility will not try to load these
drivers automatically in order to bind to new discovered devices. This
will be because the modalias tables populated by those drivers will be
different from "regular" pci modalias tables. Userspace utilities, such
as libvirt for vfio devices, will need to adjust and bind devices
according to new matching mechanism with taking "driver_override"
enforcement into consideration. Add vfio and stub to "driver_override"
PCI family and create new alias for "driver_override" vfio devices.
Signed-off-by: Max Gurtovoy <mgurtovoy@xxxxxxxxxx>
---
Documentation/PCI/pci.rst | 1 +
include/linux/mod_devicetable.h | 11 +++++++++++
include/linux/pci.h | 27 +++++++++++++++++++++++++++
scripts/mod/devicetable-offsets.c | 1 +
scripts/mod/file2alias.c | 10 ++++++++--
5 files changed, 48 insertions(+), 2 deletions(-)
diff --git a/Documentation/PCI/pci.rst b/Documentation/PCI/pci.rst
index 814b40f8360b..0855657daf93 100644
--- a/Documentation/PCI/pci.rst
+++ b/Documentation/PCI/pci.rst
@@ -103,6 +103,7 @@ need pass only as many optional fields as necessary:
- subvendor and subdevice fields default to PCI_ANY_ID (FFFFFFFF)
- class and classmask fields default to 0
- driver_data defaults to 0UL.
+ - flags field defaults to 0.
Note that driver_data must match the value used by any of the pci_device_id
entries defined in the driver. This makes the driver_data field mandatory
diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
index 7d45b5f989b0..51b852a2d32b 100644
--- a/include/linux/mod_devicetable.h
+++ b/include/linux/mod_devicetable.h
@@ -16,6 +16,15 @@ typedef unsigned long kernel_ulong_t;
#define PCI_ANY_ID (~0)
+
+enum pci_id_flags {
+ PCI_ID_F_VFIO_DRIVER_OVERRIDE = 1 << 0,
+ PCI_ID_F_STUB_DRIVER_OVERRIDE = 1 << 1,
+};
+
+#define PCI_ID_F_DRIVER_OVERRIDE \
+ (PCI_ID_F_VFIO_DRIVER_OVERRIDE | PCI_ID_F_STUB_DRIVER_OVERRIDE)
+
/**
* struct pci_device_id - PCI device ID structure
* @vendor: Vendor ID to match (or PCI_ANY_ID)
@@ -34,12 +43,14 @@ typedef unsigned long kernel_ulong_t;
* Best practice is to use driver_data as an index
* into a static list of equivalent device types,
* instead of using it as a pointer.
+ * @flags: PCI flags of the driver. Bitmap of pci_id_flags enum.
*/
struct pci_device_id {
__u32 vendor, device; /* Vendor and device ID or PCI_ANY_ID*/
__u32 subvendor, subdevice; /* Subsystem ID's or PCI_ANY_ID */
__u32 class, class_mask; /* (class,subclass,prog-if) triplet */
kernel_ulong_t driver_data; /* Data private to the driver */
+ __u32 flags;
};
diff --git a/include/linux/pci.h b/include/linux/pci.h
index c20211e59a57..4af761552068 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -898,6 +898,33 @@ struct pci_driver {
.vendor = (vend), .device = (dev), \
.subvendor = PCI_ANY_ID, .subdevice = PCI_ANY_ID
+/**
+ * PCI_DEVICE_FLAGS - macro used to describe a PCI device with specific flags.
+ * @vend: the 16 bit PCI Vendor ID
+ * @dev: the 16 bit PCI Device ID
+ * @fl: PCI Device flags as a bitmap of pci_id_flags enum
+ *
+ * This macro is used to create a struct pci_device_id that matches a
+ * specific device. The subvendor and subdevice fields will be set to
+ * PCI_ANY_ID.
+ */
+#define PCI_DEVICE_FLAGS(vend, dev, fl) \
+ .vendor = (vend), .device = (dev), .subvendor = PCI_ANY_ID, \
+ .subdevice = PCI_ANY_ID, .flags = (fl)
+
+/**
+ * PCI_DRIVER_OVERRIDE_DEVICE_VFIO - macro used to describe a VFIO
+ * "driver_override" PCI device.
+ * @vend: the 16 bit PCI Vendor ID
+ * @dev: the 16 bit PCI Device ID
+ *
+ * This macro is used to create a struct pci_device_id that matches a
+ * specific device. The subvendor and subdevice fields will be set to
+ * PCI_ANY_ID and the flags will be set to PCI_ID_F_VFIO_DRIVER_OVERRIDE.
+ */
+#define PCI_DRIVER_OVERRIDE_DEVICE_VFIO(vend, dev) \
+ PCI_DEVICE_FLAGS(vend, dev, PCI_ID_F_VFIO_DRIVER_OVERRIDE)
+
/**
* PCI_DEVICE_SUB - macro used to describe a specific PCI device with subsystem
* @vend: the 16 bit PCI Vendor ID
diff --git a/scripts/mod/devicetable-offsets.c b/scripts/mod/devicetable-offsets.c
index 9bb6c7edccc4..b927c36b8333 100644
--- a/scripts/mod/devicetable-offsets.c
+++ b/scripts/mod/devicetable-offsets.c
@@ -42,6 +42,7 @@ int main(void)
DEVID_FIELD(pci_device_id, subdevice);
DEVID_FIELD(pci_device_id, class);
DEVID_FIELD(pci_device_id, class_mask);
+ DEVID_FIELD(pci_device_id, flags);
DEVID(ccw_device_id);
DEVID_FIELD(ccw_device_id, match_flags);
diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
index 7c97fa8e36bc..2b2b7d875416 100644
--- a/scripts/mod/file2alias.c
+++ b/scripts/mod/file2alias.c
@@ -426,7 +426,7 @@ static int do_ieee1394_entry(const char *filename,
return 1;
}
-/* Looks like: pci:vNdNsvNsdNbcNscNiN. */
+/* Looks like: pci:vNdNsvNsdNbcNscNiN or <prefix>_pci:vNdNsvNsdNbcNscNiN. */
static int do_pci_entry(const char *filename,
void *symval, char *alias)
{
@@ -440,8 +440,14 @@ static int do_pci_entry(const char *filename,
DEF_FIELD(symval, pci_device_id, subdevice);
DEF_FIELD(symval, pci_device_id, class);
DEF_FIELD(symval, pci_device_id, class_mask);
+ DEF_FIELD(symval, pci_device_id, flags);
- strcpy(alias, "pci:");
+ if (!flags)
+ strcpy(alias, "pci:");
+ else if (flags & PCI_ID_F_VFIO_DRIVER_OVERRIDE)
+ strcpy(alias, "vfio_pci:");
+ else
+ return 0;
ADD(alias, "v", vendor != PCI_ANY_ID, vendor);
ADD(alias, "d", device != PCI_ANY_ID, device);
ADD(alias, "sv", subvendor != PCI_ANY_ID, subvendor);
--
2.18.1
From dbad3ab97af39455c4e4f6967bdd47ef1ea67198 Mon Sep 17 00:00:00 2001
From: Max Gurtovoy <mgurtovoy@xxxxxxxxxx>
Date: Thu, 13 May 2021 15:38:27 +0300
Subject: [PATCH 2/2] PCI: add matching checks for driver_override binding
Allowing any driver in the system to be unconditionally bound to any
PCI HW is dangerous. Connecting a driver to a physical HW device it was
never intended to operate may trigger exploitable kernel bugs, or worse.
It also allows userspace to load and run kernel code that otherwise
would never be runnable on the system.
Enable the option for drivers to specifically opt into driver_override
feature. These drivers will expose a proper matching table that indicates
what HW it can properly support. vfio-pci and pci stub drivers continues
to support everything.
The modalias tables to be populated with dedicated match table and
userspace now becomes aware that vfio-pci can be loaded against any PCI
device using driver_override (modalias for pci stub drivers can be added
in the future, if needed).
The dynamic new_id option is still might be in use for these drivers to
preserve backward compatibility. For now, also preserve the old
driver_override mechanism for non "driver_override" modules.
Signed-off-by: Max Gurtovoy <mgurtovoy@xxxxxxxxxx>
---
Documentation/ABI/testing/sysfs-bus-pci | 5 ++++-
drivers/pci/pci-driver.c | 26 +++++++++++++++++++++----
drivers/pci/pci-pf-stub.c | 1 +
drivers/pci/pci-stub.c | 9 ++++++++-
drivers/vfio/pci/vfio_pci.c | 9 ++++++++-
5 files changed, 43 insertions(+), 7 deletions(-)
diff --git a/Documentation/ABI/testing/sysfs-bus-pci b/Documentation/ABI/testing/sysfs-bus-pci
index ef00fada2efb..ffb31fda597d 100644
--- a/Documentation/ABI/testing/sysfs-bus-pci
+++ b/Documentation/ABI/testing/sysfs-bus-pci
@@ -289,7 +289,10 @@ Description:
will not bind to any driver. This also allows devices to
opt-out of driver binding using a driver_override name such as
"none". Only a single driver may be specified in the override,
- there is no support for parsing delimiters.
+ there is no support for parsing delimiters. For static IDs
+ matching, this binding mechanism is allowed if the matching
+ driver has specifically opt into this feature (by setting a
+ suitable flag in the "flags" field of pci_device_id structure).
What: /sys/bus/pci/devices/.../numa_node
Date: Oct 2014
diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index ec44a79e951a..ba4494499931 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -136,7 +136,7 @@ static const struct pci_device_id *pci_match_device(struct pci_driver *drv,
struct pci_dev *dev)
{
struct pci_dynid *dynid;
- const struct pci_device_id *found_id = NULL;
+ const struct pci_device_id *found_id = NULL, *ids;
/* When driver_override is set, only bind to the matching driver */
if (dev->driver_override && strcmp(dev->driver_override, drv->name))
@@ -152,10 +152,28 @@ static const struct pci_device_id *pci_match_device(struct pci_driver *drv,
}
spin_unlock(&drv->dynids.lock);
- if (!found_id)
- found_id = pci_match_id(drv->id_table, dev);
+ if (found_id)
+ return found_id;
- /* driver_override will always match, send a dummy id */
+ ids = drv->id_table;
+ while ((found_id = pci_match_id(ids, dev))) {
+ /*
+ * if we found id in the static table, we must fulfill the
+ * matching flags (i.e. if PCI_ID_F_DRIVER_OVERRIDE flag is
+ * set, driver_override should be provided).
+ */
+ bool is_driver_override =
+ (found_id->flags & PCI_ID_F_DRIVER_OVERRIDE) != 0;
+ if (is_driver_override && !dev->driver_override)
+ ids = found_id + 1; /* continue searching */
+ else
+ break; /* found a match ! */
+ }
+
+ /*
+ * if no static match, driver_override will always match, send a dummy
+ * id.
+ */
if (!found_id && dev->driver_override)
found_id = &pci_device_id_any;
diff --git a/drivers/pci/pci-pf-stub.c b/drivers/pci/pci-pf-stub.c
index 45855a5e9fca..49544ba9a7af 100644
--- a/drivers/pci/pci-pf-stub.c
+++ b/drivers/pci/pci-pf-stub.c
@@ -19,6 +19,7 @@
*/
static const struct pci_device_id pci_pf_stub_whitelist[] = {
{ PCI_VDEVICE(AMAZON, 0x0053) },
+ { PCI_DEVICE_FLAGS(PCI_ANY_ID, PCI_ANY_ID, PCI_ID_F_STUB_DRIVER_OVERRIDE) }, /* match all by default (override) */
/* required last entry */
{ 0 }
};
diff --git a/drivers/pci/pci-stub.c b/drivers/pci/pci-stub.c
index e408099fea52..a67d136ad1e2 100644
--- a/drivers/pci/pci-stub.c
+++ b/drivers/pci/pci-stub.c
@@ -26,6 +26,13 @@ MODULE_PARM_DESC(ids, "Initial PCI IDs to add to the stub driver, format is "
"\"vendor:device[:subvendor[:subdevice[:class[:class_mask]]]]\""
" and multiple comma separated entries can be specified");
+static const struct pci_device_id stub_pci_table[] = {
+ { PCI_DEVICE_FLAGS(PCI_ANY_ID, PCI_ANY_ID, PCI_ID_F_STUB_DRIVER_OVERRIDE) }, /* match all by default */
+ { 0, }
+};
+
+MODULE_DEVICE_TABLE(pci, stub_pci_table);
+
static int pci_stub_probe(struct pci_dev *dev, const struct pci_device_id *id)
{
pci_info(dev, "claimed by stub\n");
@@ -34,7 +41,7 @@ static int pci_stub_probe(struct pci_dev *dev, const struct pci_device_id *id)
static struct pci_driver stub_driver = {
.name = "pci-stub",
- .id_table = NULL, /* only dynamic id's */
+ .id_table = stub_pci_table,
.probe = pci_stub_probe,
};
diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
index 850ea3a94e28..9beb4b841945 100644
--- a/drivers/vfio/pci/vfio_pci.c
+++ b/drivers/vfio/pci/vfio_pci.c
@@ -166,9 +166,16 @@ static int vfio_pci_sriov_configure(struct pci_dev *pdev, int nr_virtfn)
return vfio_pci_core_sriov_configure(pdev, nr_virtfn);
}
+static const struct pci_device_id vfio_pci_table[] = {
+ { PCI_DRIVER_OVERRIDE_DEVICE_VFIO(PCI_ANY_ID, PCI_ANY_ID) }, /* match all by default */
+ { 0, }
+};
+
+MODULE_DEVICE_TABLE(pci, vfio_pci_table);
+
static struct pci_driver vfio_pci_driver = {
.name = "vfio-pci",
- .id_table = NULL, /* only dynamic ids */
+ .id_table = vfio_pci_table,
.probe = vfio_pci_probe,
.remove = vfio_pci_remove,
.sriov_configure = vfio_pci_sriov_configure,
--
2.18.1
