[PATCH 1/2] PCI: populate struct pci_device subsystem_device, subsystem_vendor

[PATCH 1/2] PCI: populate struct pci_device subsystem_device, subsystem_vendor

[Ahmad Fatoum]

pci_device::subsystem_device and pci_device::subsystem_vendor have been
there since the beginning, but they were never populated. This went
unnoticed so far, because they are compared against PCI_ANY_ID
everywhere, except for some NS16550-over-PCI quirks.

We should either drop the members or populate them unconditionally.
Let's do as Linux does and populate them for devices[1].

[1]: https://elixir.bootlin.com/linux/v6.5-rc7/source/drivers/pci/probe.c#L1915

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 drivers/pci/pci.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index d1b7549d7100..638af9722efc 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -434,6 +434,9 @@ static unsigned int pci_scan_bus(struct pci_bus *bus)
 				goto bad;
 
 			setup_device(dev, 6);
+
+			pci_read_config_word(dev, PCI_SUBSYSTEM_ID, &dev->subsystem_device);
+			pci_read_config_word(dev, PCI_SUBSYSTEM_VENDOR_ID, &dev->subsystem_vendor);
 			break;
 		case PCI_HEADER_TYPE_BRIDGE:
 			child_bus = pci_alloc_bus();
-- 
2.39.2

[PATCH 2/2] virtio: pci: add support for transitional devices

[Ahmad Fatoum]

So far, barebox required that Virt I/O devices it consumes are
parameterized with pci,disable-legacy=on. Otherwise, it bails out with:

  Legacy and transitional devices unsupported

because only modern (specification >= v1.0) devices were supported.

By default, Qemu provides its guests with transitional virtio-pci
devices: These enumerate like legacy devices, but implement the bars of
both legacy and modern devices. Add support for transitional device, so
pci,disable-legacy=on is not required any longer. we still leave
pci,disable-modern=off in the scripts, but it should not be needed with
any recent Qemu version.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 Documentation/user/virtio.rst      | 14 ++++++++------
 drivers/virtio/virtio_pci_common.c |  2 +-
 drivers/virtio/virtio_pci_modern.c | 18 +++++++++++-------
 test/conftest.py                   |  2 +-
 4 files changed, 21 insertions(+), 15 deletions(-)

diff --git a/Documentation/user/virtio.rst b/Documentation/user/virtio.rst
index a8624649f473..046e5dac8225 100644
--- a/Documentation/user/virtio.rst
+++ b/Documentation/user/virtio.rst
@@ -72,15 +72,17 @@ malta VM with one HWRNG and 2 block VirtIO PCI devices::
 
   $ qemu-system-mips -m 256M -M malta -serial stdio         \
     	-bios ./images/barebox-qemu-malta.img -monitor null \
-  	-device virtio-rng-pci,disable-legacy=on            \
+  	-device virtio-rng-pci                              \
   	-drive if=none,file=image1.hdimg,format=raw,id=hd0  \
-  	-device virtio-blk-pci,drive=hd0,disable-legacy=on  \
+  	-device virtio-blk-pci,drive=hd0                    \
   	-drive if=none,file=image2.hdimg,format=raw,id=hd1  \
-  	-device virtio-blk-pci,drive=hd1,disable-legacy=on
+  	-device virtio-blk-pci,drive=hd1
 
-Note the use of ``disable-legacy=on``. barebox doesn't support legacy
-or transitional VirtIO devices. Some versions of QEMU may need to
-have ``,disable-modern=off`` specfied as well.
+Note that barebox does not support non-transitional legacy Virt I/O devices.
+Depending on QEMU version used, it may be required to add
+``disable-legacy=on``, ``disable-modern=off`` or both, e.g.::
+
+  	-device virtio-blk-pci,drive=hd1,disable-legacy=on,disable-modern=off
 
 .. _VirtIO: http://docs.oasis-open.org/virtio/virtio/v1.0/virtio-v1.0.pdf
 .. _qemu: https://www.qemu.org
diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
index b0ac8befd49b..c4644834c797 100644
--- a/drivers/virtio/virtio_pci_common.c
+++ b/drivers/virtio/virtio_pci_common.c
@@ -46,7 +46,7 @@ static int virtio_pci_probe(struct pci_dev *pci_dev,
 
 	rc = virtio_pci_modern_probe(vp_dev);
 	if (rc == -ENODEV)
-		dev_err(&pci_dev->dev, "Legacy and transitional devices unsupported\n");
+		dev_err(&pci_dev->dev, "Legacy devices unsupported\n");
 	if (rc)
 		goto err_enable_device;
 
diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
index 5cdea79fbc13..26eefba85bea 100644
--- a/drivers/virtio/virtio_pci_modern.c
+++ b/drivers/virtio/virtio_pci_modern.c
@@ -359,15 +359,19 @@ int virtio_pci_modern_probe(struct virtio_pci_device *vp_dev)
 	int common, notify, device;
 	int offset;
 
-	/*
-	 * We only own devices >= 0x1000 and <= 0x107f. We don't support
-	 * transitional devices, so start at 0x1040 and leave the rest.
-	 */
-	if (pci_dev->device < 0x1040 || pci_dev->device > 0x107f)
+	/* We only own devices >= 0x1000 and <= 0x107f: leave the rest. */
+	if (pci_dev->device < 0x1000 || pci_dev->device > 0x107f)
 		return -ENODEV;
 
-	/* Modern devices: simply use PCI device id, but start from 0x1040. */
-	vp_dev->vdev.id.device = pci_dev->device - 0x1040;
+	if (pci_dev->device < 0x1040) {
+		/* Transitional devices: use the PCI subsystem device id as
+		 * virtio device id, same as legacy driver always did.
+		 */
+		vp_dev->vdev.id.device = pci_dev->subsystem_device;
+	} else {
+		/* Modern devices: simply use PCI device id, but start from 0x1040. */
+		vp_dev->vdev.id.device = pci_dev->device - 0x1040;
+	}
 	vp_dev->vdev.id.vendor = pci_dev->subsystem_vendor;
 
 	/* Check for a common config: if not, driver could fall back to legacy mode (bar 0) */
diff --git a/test/conftest.py b/test/conftest.py
index 2bca3c37e5fb..d2eef5156423 100644
--- a/test/conftest.py
+++ b/test/conftest.py
@@ -61,7 +61,7 @@ def strategy(request, target, pytestconfig):
     if "virtio-mmio" in features:
         virtio = "device"
     if "virtio-pci" in features:
-        virtio = "pci,disable-legacy=on,disable-modern=off"
+        virtio = "pci,disable-modern=off"
         features.append("pci")
 
     if virtio and pytestconfig.option.qemu_rng:
-- 
2.39.2

Re: [PATCH 1/2] PCI: populate struct pci_device subsystem_device, subsystem_vendor

[Sascha Hauer]

On Tue, Aug 22, 2023 at 09:48:31AM +0200, Ahmad Fatoum wrote:
> pci_device::subsystem_device and pci_device::subsystem_vendor have been
> there since the beginning, but they were never populated. This went
> unnoticed so far, because they are compared against PCI_ANY_ID
> everywhere, except for some NS16550-over-PCI quirks.
> 
> We should either drop the members or populate them unconditionally.
> Let's do as Linux does and populate them for devices[1].
> 
> [1]: https://elixir.bootlin.com/linux/v6.5-rc7/source/drivers/pci/probe.c#L1915
> 
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
>  drivers/pci/pci.c | 3 +++
>  1 file changed, 3 insertions(+)

Applied, thanks

Sascha

> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index d1b7549d7100..638af9722efc 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -434,6 +434,9 @@ static unsigned int pci_scan_bus(struct pci_bus *bus)
>  				goto bad;
>  
>  			setup_device(dev, 6);
> +
> +			pci_read_config_word(dev, PCI_SUBSYSTEM_ID, &dev->subsystem_device);
> +			pci_read_config_word(dev, PCI_SUBSYSTEM_VENDOR_ID, &dev->subsystem_vendor);
>  			break;
>  		case PCI_HEADER_TYPE_BRIDGE:
>  			child_bus = pci_alloc_bus();
> -- 
> 2.39.2
> 
> 
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |