From e052944a966ca4eb0131121646fa0ef047c25e52 Mon Sep 17 00:00:00 2001 From: Bernhard Beschow Date: Sat, 4 Mar 2023 12:40:42 +0100 Subject: hw/pci/pci: Remove multifunction parameter from pci_create_simple_multifunction() There is also pci_create_simple() which creates non-multifunction PCI devices. Accordingly the parameter is always set to true when a multi function PCI device is to be created. The reason for the parameter's existence seems to be that it is used in the internal PCI code as well which is the only location where it gets set to false. This one usage can be replaced by trivial code. Remove this redundant, error-prone parameter. Signed-off-by: Bernhard Beschow Message-Id: <20230304114043.121024-5-shentey@gmail.com> Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- hw/pci/pci.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) (limited to 'hw/pci/pci.c') diff --git a/hw/pci/pci.c b/hw/pci/pci.c index e2eb4c3b4a..5152989c10 100644 --- a/hw/pci/pci.c +++ b/hw/pci/pci.c @@ -2186,17 +2186,18 @@ bool pci_realize_and_unref(PCIDevice *dev, PCIBus *bus, Error **errp) } PCIDevice *pci_create_simple_multifunction(PCIBus *bus, int devfn, - bool multifunction, const char *name) { - PCIDevice *dev = pci_new_multifunction(devfn, multifunction, name); + PCIDevice *dev = pci_new_multifunction(devfn, true, name); pci_realize_and_unref(dev, bus, &error_fatal); return dev; } PCIDevice *pci_create_simple(PCIBus *bus, int devfn, const char *name) { - return pci_create_simple_multifunction(bus, devfn, false, name); + PCIDevice *dev = pci_new(devfn, name); + pci_realize_and_unref(dev, bus, &error_fatal); + return dev; } static uint8_t pci_find_space(PCIDevice *pdev, uint8_t size) -- cgit 1.4.1 From c925f40a29906355b516bad4c6ef80d30cf971b6 Mon Sep 17 00:00:00 2001 From: Bernhard Beschow Date: Sat, 4 Mar 2023 12:40:43 +0100 Subject: hw/pci/pci: Remove multifunction parameter from pci_new_multifunction() There is also pci_new() which creates non-multifunction PCI devices. Accordingly the parameter is always set to true when a multi function PCI device is to be created. The reason for the parameter's existence seems to be that it is used in the internal PCI code as well which is the only location where it gets set to false. This one usage can be resolved by factoring out an internal helper function. Remove this redundant, error-prone parameter. Signed-off-by: Bernhard Beschow Message-Id: <20230304114043.121024-6-shentey@gmail.com> Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- hw/i386/pc_q35.c | 6 +++--- hw/pci-host/sabre.c | 6 ++---- hw/pci/pci.c | 13 +++++++++---- hw/sparc64/sun4u.c | 5 ++--- include/hw/pci/pci.h | 3 +-- 5 files changed, 17 insertions(+), 16 deletions(-) (limited to 'hw/pci/pci.c') diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c index fdab1f6a56..dc27a9e223 100644 --- a/hw/i386/pc_q35.c +++ b/hw/i386/pc_q35.c @@ -100,12 +100,12 @@ static int ehci_create_ich9_with_companions(PCIBus *bus, int slot) return -1; } - ehci = pci_new_multifunction(PCI_DEVFN(slot, 7), true, name); + ehci = pci_new_multifunction(PCI_DEVFN(slot, 7), name); pci_realize_and_unref(ehci, bus, &error_fatal); usbbus = QLIST_FIRST(&ehci->qdev.child_bus); for (i = 0; i < 3; i++) { - uhci = pci_new_multifunction(PCI_DEVFN(slot, comp[i].func), true, + uhci = pci_new_multifunction(PCI_DEVFN(slot, comp[i].func), comp[i].name); qdev_prop_set_string(&uhci->qdev, "masterbus", usbbus->name); qdev_prop_set_uint32(&uhci->qdev, "firstport", comp[i].port); @@ -239,7 +239,7 @@ static void pc_q35_init(MachineState *machine) pcms->bus = host_bus; /* create ISA bus */ - lpc = pci_new_multifunction(PCI_DEVFN(ICH9_LPC_DEV, ICH9_LPC_FUNC), true, + lpc = pci_new_multifunction(PCI_DEVFN(ICH9_LPC_DEV, ICH9_LPC_FUNC), TYPE_ICH9_LPC_DEVICE); qdev_prop_set_bit(DEVICE(lpc), "smm-enabled", x86_machine_is_smm_enabled(x86ms)); diff --git a/hw/pci-host/sabre.c b/hw/pci-host/sabre.c index 949ecc21f2..dcb2e230b6 100644 --- a/hw/pci-host/sabre.c +++ b/hw/pci-host/sabre.c @@ -387,14 +387,12 @@ static void sabre_realize(DeviceState *dev, Error **errp) pci_setup_iommu(phb->bus, sabre_pci_dma_iommu, s->iommu); /* APB secondary busses */ - pci_dev = pci_new_multifunction(PCI_DEVFN(1, 0), true, - TYPE_SIMBA_PCI_BRIDGE); + pci_dev = pci_new_multifunction(PCI_DEVFN(1, 0), TYPE_SIMBA_PCI_BRIDGE); s->bridgeB = PCI_BRIDGE(pci_dev); pci_bridge_map_irq(s->bridgeB, "pciB", pci_simbaB_map_irq); pci_realize_and_unref(pci_dev, phb->bus, &error_fatal); - pci_dev = pci_new_multifunction(PCI_DEVFN(1, 1), true, - TYPE_SIMBA_PCI_BRIDGE); + pci_dev = pci_new_multifunction(PCI_DEVFN(1, 1), TYPE_SIMBA_PCI_BRIDGE); s->bridgeA = PCI_BRIDGE(pci_dev); pci_bridge_map_irq(s->bridgeA, "pciA", pci_simbaA_map_irq); pci_realize_and_unref(pci_dev, phb->bus, &error_fatal); diff --git a/hw/pci/pci.c b/hw/pci/pci.c index 5152989c10..fb17138f7d 100644 --- a/hw/pci/pci.c +++ b/hw/pci/pci.c @@ -2164,8 +2164,8 @@ static void pci_qdev_realize(DeviceState *qdev, Error **errp) pci_dev->msi_trigger = pci_msi_trigger; } -PCIDevice *pci_new_multifunction(int devfn, bool multifunction, - const char *name) +static PCIDevice *pci_new_internal(int devfn, bool multifunction, + const char *name) { DeviceState *dev; @@ -2175,9 +2175,14 @@ PCIDevice *pci_new_multifunction(int devfn, bool multifunction, return PCI_DEVICE(dev); } +PCIDevice *pci_new_multifunction(int devfn, const char *name) +{ + return pci_new_internal(devfn, true, name); +} + PCIDevice *pci_new(int devfn, const char *name) { - return pci_new_multifunction(devfn, false, name); + return pci_new_internal(devfn, false, name); } bool pci_realize_and_unref(PCIDevice *dev, PCIBus *bus, Error **errp) @@ -2188,7 +2193,7 @@ bool pci_realize_and_unref(PCIDevice *dev, PCIBus *bus, Error **errp) PCIDevice *pci_create_simple_multifunction(PCIBus *bus, int devfn, const char *name) { - PCIDevice *dev = pci_new_multifunction(devfn, true, name); + PCIDevice *dev = pci_new_multifunction(devfn, name); pci_realize_and_unref(dev, bus, &error_fatal); return dev; } diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c index 29e9b6cc26..d908a38f73 100644 --- a/hw/sparc64/sun4u.c +++ b/hw/sparc64/sun4u.c @@ -612,7 +612,7 @@ static void sun4uv_init(MemoryRegion *address_space_mem, pci_bus_set_slot_reserved_mask(pci_busA, 0xfffffff1); pci_bus_set_slot_reserved_mask(pci_busB, 0xfffffff0); - ebus = pci_new_multifunction(PCI_DEVFN(1, 0), true, TYPE_EBUS); + ebus = pci_new_multifunction(PCI_DEVFN(1, 0), TYPE_EBUS); qdev_prop_set_uint64(DEVICE(ebus), "console-serial-base", hwdef->console_serial_base); pci_realize_and_unref(ebus, pci_busA, &error_fatal); @@ -648,8 +648,7 @@ static void sun4uv_init(MemoryRegion *address_space_mem, if (!nd->model || strcmp(nd->model, mc->default_nic) == 0) { if (!onboard_nic) { - pci_dev = pci_new_multifunction(PCI_DEVFN(1, 1), - true, mc->default_nic); + pci_dev = pci_new_multifunction(PCI_DEVFN(1, 1), mc->default_nic); bus = pci_busA; memcpy(&macaddr, &nd->macaddr.a, sizeof(MACAddr)); onboard_nic = true; diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h index 18c35b64db..ab2bd65a3a 100644 --- a/include/hw/pci/pci.h +++ b/include/hw/pci/pci.h @@ -577,8 +577,7 @@ pci_set_quad_by_mask(uint8_t *config, uint64_t mask, uint64_t reg) pci_set_quad(config, (~mask & val) | (mask & rval)); } -PCIDevice *pci_new_multifunction(int devfn, bool multifunction, - const char *name); +PCIDevice *pci_new_multifunction(int devfn, const char *name); PCIDevice *pci_new(int devfn, const char *name); bool pci_realize_and_unref(PCIDevice *dev, PCIBus *bus, Error **errp); -- cgit 1.4.1 From ca92eb5defcf9d1c2106341744a73a03cf26e824 Mon Sep 17 00:00:00 2001 From: Ani Sinha Date: Wed, 5 Jul 2023 17:29:23 +0530 Subject: hw/pci: warn when PCIe device is plugged into non-zero slot of downstream port PCIe downstream ports only have a single device 0, so PCI Express devices can only be plugged into slot 0 on a PCIe port. Add a warning to let users know when the invalid configuration is used. We may enforce this more strongly later once we get more clarity on whether we are introducing a bad regression for users currently using the wrong configuration. The change has been tested to not break or alter behaviors of ARI capable devices by instantiating seven vfs on an emulated igb device (the maximum number of vfs the igb device supports). The vfs are instantiated correctly and are seen to have non-zero device/slot numbers in the conventional PCI BDF representation. CC: jusual@redhat.com CC: imammedo@redhat.com CC: mst@redhat.com CC: akihiko.odaki@daynix.com Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2128929 Signed-off-by: Ani Sinha Reviewed-by: Julia Suvorova Message-Id: <20230705115925.5339-6-anisinha@redhat.com> Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin Reviewed-by: Akihiko Odaki --- hw/pci/pci.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) (limited to 'hw/pci/pci.c') diff --git a/hw/pci/pci.c b/hw/pci/pci.c index fb17138f7d..4b14f31859 100644 --- a/hw/pci/pci.c +++ b/hw/pci/pci.c @@ -65,6 +65,7 @@ bool pci_available = true; static char *pcibus_get_dev_path(DeviceState *dev); static char *pcibus_get_fw_dev_path(DeviceState *dev); static void pcibus_reset(BusState *qbus); +static bool pcie_has_upstream_port(PCIDevice *dev); static Property pci_props[] = { DEFINE_PROP_PCI_DEVFN("addr", PCIDevice, devfn, -1), @@ -2121,6 +2122,25 @@ static void pci_qdev_realize(DeviceState *qdev, Error **errp) } } + /* + * A PCIe Downstream Port that do not have ARI Forwarding enabled must + * associate only Device 0 with the device attached to the bus + * representing the Link from the Port (PCIe base spec rev 4.0 ver 0.3, + * sec 7.3.1). + * With ARI, PCI_SLOT() can return non-zero value as the traditional + * 5-bit Device Number and 3-bit Function Number fields in its associated + * Routing IDs, Requester IDs and Completer IDs are interpreted as a + * single 8-bit Function Number. Hence, ignore ARI capable devices. + */ + if (pci_is_express(pci_dev) && + !pcie_find_capability(pci_dev, PCI_EXT_CAP_ID_ARI) && + pcie_has_upstream_port(pci_dev) && + PCI_SLOT(pci_dev->devfn)) { + warn_report("PCI: slot %d is not valid for %s," + " parent device only allows plugging into slot 0.", + PCI_SLOT(pci_dev->devfn), pci_dev->name); + } + if (pci_dev->failover_pair_id) { if (!pci_bus_is_express(pci_get_bus(pci_dev))) { error_setg(errp, "failover primary device must be on " -- cgit 1.4.1 From 7c228c5f3301113ffd7cfee7f982e7ae04c8ffda Mon Sep 17 00:00:00 2001 From: Akihiko Odaki Date: Tue, 11 Jul 2023 00:38:36 +0900 Subject: pcie: Specify 0 for ARI next function numbers The current implementers of ARI are all SR-IOV devices. The ARI next function number field is undefined for VF according to PCI Express Base Specification Revision 5.0 Version 1.0 section 9.3.7.7. The PF still requires some defined value so end the linked list formed with the field by specifying 0 as required for any ARI implementation according to section 7.8.7.2. For migration, the field will keep having 1 as its value on the old QEMU machine versions. Fixes: 2503461691 ("pcie: Add some SR/IOV API documentation in docs/pcie_sriov.txt") Fixes: 44c2c09488 ("hw/nvme: Add support for SR-IOV") Fixes: 3a977deebe ("Intrdocue igb device emulation") Signed-off-by: Akihiko Odaki Reviewed-by: Ani Sinha Message-Id: <20230710153838.33917-3-akihiko.odaki@daynix.com> Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- hw/core/machine.c | 1 + hw/pci/pci.c | 2 ++ hw/pci/pcie.c | 2 +- include/hw/pci/pci.h | 2 ++ 4 files changed, 6 insertions(+), 1 deletion(-) (limited to 'hw/pci/pci.c') diff --git a/hw/core/machine.c b/hw/core/machine.c index 46f8f9a2b0..f0d35c6401 100644 --- a/hw/core/machine.c +++ b/hw/core/machine.c @@ -41,6 +41,7 @@ GlobalProperty hw_compat_8_0[] = { { "migration", "multifd-flush-after-each-section", "on"}, + { TYPE_PCI_DEVICE, "x-pcie-ari-nextfn-1", "on" }, }; const size_t hw_compat_8_0_len = G_N_ELEMENTS(hw_compat_8_0); diff --git a/hw/pci/pci.c b/hw/pci/pci.c index 4b14f31859..784c02a182 100644 --- a/hw/pci/pci.c +++ b/hw/pci/pci.c @@ -83,6 +83,8 @@ static Property pci_props[] = { DEFINE_PROP_UINT32("acpi-index", PCIDevice, acpi_index, 0), DEFINE_PROP_BIT("x-pcie-err-unc-mask", PCIDevice, cap_present, QEMU_PCIE_ERR_UNC_MASK_BITNR, true), + DEFINE_PROP_BIT("x-pcie-ari-nextfn-1", PCIDevice, cap_present, + QEMU_PCIE_ARI_NEXTFN_1_BITNR, false), DEFINE_PROP_END_OF_LIST() }; diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c index 6075ff5556..6db0cf69cd 100644 --- a/hw/pci/pcie.c +++ b/hw/pci/pcie.c @@ -1041,7 +1041,7 @@ void pcie_sync_bridge_lnk(PCIDevice *bridge_dev) /* ARI */ void pcie_ari_init(PCIDevice *dev, uint16_t offset) { - uint16_t nextfn = 1; + uint16_t nextfn = dev->cap_present & QEMU_PCIE_ARI_NEXTFN_1 ? 1 : 0; pcie_add_capability(dev, PCI_EXT_CAP_ID_ARI, PCI_ARI_VER, offset, PCI_ARI_SIZEOF); diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h index ab2bd65a3a..abdc1ef103 100644 --- a/include/hw/pci/pci.h +++ b/include/hw/pci/pci.h @@ -209,6 +209,8 @@ enum { QEMU_PCIE_CAP_CXL = (1 << QEMU_PCIE_CXL_BITNR), #define QEMU_PCIE_ERR_UNC_MASK_BITNR 11 QEMU_PCIE_ERR_UNC_MASK = (1 << QEMU_PCIE_ERR_UNC_MASK_BITNR), +#define QEMU_PCIE_ARI_NEXTFN_1_BITNR 12 + QEMU_PCIE_ARI_NEXTFN_1 = (1 << QEMU_PCIE_ARI_NEXTFN_1_BITNR), }; typedef struct PCIINTxRoute { -- cgit 1.4.1