[PATCH v3 0/2] pci: slot_reserved_mask improvements

[PATCH v3 0/2] pci: slot_reserved_mask improvements

[Chuck Zmudzinski]

This patch series consists of two patches. The first provides accessor
functions in pci.h to avoid direct access of slot_reserved_mask
according to the comment at the top of include/hw/pci/pci_bus.h. No
functional change is intended with this patch.

The second patch replaces slot_reserved_mask with two new masks,
slot_reserved_auto_mask and slot_reserved_manual_mask so the current
behavior of reserving slot 2 for the Intel IGD for the xenfv machine
will be ignored if an administrator manually configures a device to use
the reserved slot.

The current behavior of always reserving slots in the sun4u machine is
preserved by this patch series; the patch series only changes how
slot_reserved_mask works in the xenfv machine. Although the patch
series can affect xenfv machines configured for igd-passthru if an
administrator assigns some of the pci slot addresses manually, it
does not affect the libxl default configuration for igd-passthru because
libxl uses automatic slot assignment by default.

Testing:
   - Tested xenfv/igd with both manual and auto slot allocation - behaves as expected
   - Verified that qemu-system-sparc64 still compiles with the patches to sun4u.c
   - xen4u machine not tested -- Mark, can you do this?

Link: https://lore.kernel.org/qemu-devel/20230106064838-mutt-send-email-mst@kernel.org/

Chuck Zmudzinski (2):
  pci: avoid accessing slot_reserved_mask directly outside of pci.c
  pci: introduce slot_reserved_auto_mask and slot_reserved_manual_mask

Changelog

v3: Re-work second patch in response to comments/discussion of v2

v2: Add first patch and cover letter to make this a 2-patch series
    Make changes to the second patch (see second patch for changelog)

 hw/pci/pci.c             | 32 +++++++++++++++++++++++++++-----
 hw/sparc64/sun4u.c       |  7 +++----
 hw/xen/xen_pt.c          |  7 +++----
 include/hw/pci/pci.h     |  3 +++
 include/hw/pci/pci_bus.h |  3 ++-
 5 files changed, 38 insertions(+), 14 deletions(-)

-- 
2.39.2

[PATCH v3 1/2] pci: avoid accessing slot_reserved_mask directly outside of pci.c

[Chuck Zmudzinski]

This patch provides accessor functions as replacements for direct
access to slot_reserved_mask according to the comment at the top
of include/hw/pci/pci_bus.h which advises that data structures for
PCIBus should not be directly accessed but instead be accessed using
accessor functions in pci.h.

Three accessor functions can conveniently replace all direct accesses
of slot_reserved_mask. With this patch, the new accessor functions are
used in hw/sparc64/sun4u.c and hw/xen/xen_pt.c and pci_bus.h is removed
from the included header files of the same two files.

No functional change intended.

Signed-off-by: Chuck Zmudzinski <brchuckz@aol.com>
---
Changelog

v3: This patch is unchanged since v2.

v2: This is the first version of this patch, it did not exist in v1.

 hw/pci/pci.c         | 15 +++++++++++++++
 hw/sparc64/sun4u.c   |  7 +++----
 hw/xen/xen_pt.c      |  7 +++----
 include/hw/pci/pci.h |  3 +++
 4 files changed, 24 insertions(+), 8 deletions(-)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index def5000e7b..8a87ccc8b0 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -1116,6 +1116,21 @@ static bool pci_bus_devfn_reserved(PCIBus *bus, int devfn)
     return bus->slot_reserved_mask & (1UL << PCI_SLOT(devfn));
 }
 
+uint32_t pci_bus_get_slot_reserved_mask(PCIBus *bus)
+{
+    return bus->slot_reserved_mask;
+}
+
+void pci_bus_set_slot_reserved_mask(PCIBus *bus, uint32_t mask)
+{
+    bus->slot_reserved_mask |= mask;
+}
+
+void pci_bus_clear_slot_reserved_mask(PCIBus *bus, uint32_t mask)
+{
+    bus->slot_reserved_mask &= ~mask;
+}
+
 /* -1 for devfn means auto assign */
 static PCIDevice *do_pci_register_device(PCIDevice *pci_dev,
                                          const char *name, int devfn,
diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
index a25e951f9d..eae7589462 100644
--- a/hw/sparc64/sun4u.c
+++ b/hw/sparc64/sun4u.c
@@ -31,7 +31,6 @@
 #include "hw/irq.h"
 #include "hw/pci/pci.h"
 #include "hw/pci/pci_bridge.h"
-#include "hw/pci/pci_bus.h"
 #include "hw/pci/pci_host.h"
 #include "hw/qdev-properties.h"
 #include "hw/pci-host/sabre.h"
@@ -608,9 +607,9 @@ static void sun4uv_init(MemoryRegion *address_space_mem,
     /* Only in-built Simba APBs can exist on the root bus, slot 0 on busA is
        reserved (leaving no slots free after on-board devices) however slots
        0-3 are free on busB */
-    pci_bus->slot_reserved_mask = 0xfffffffc;
-    pci_busA->slot_reserved_mask = 0xfffffff1;
-    pci_busB->slot_reserved_mask = 0xfffffff0;
+    pci_bus_set_slot_reserved_mask(pci_bus, 0xfffffffc);
+    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);
     qdev_prop_set_uint64(DEVICE(ebus), "console-serial-base",
diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
index 2d33d178ad..a540149639 100644
--- a/hw/xen/xen_pt.c
+++ b/hw/xen/xen_pt.c
@@ -57,7 +57,6 @@
 #include <sys/ioctl.h>
 
 #include "hw/pci/pci.h"
-#include "hw/pci/pci_bus.h"
 #include "hw/qdev-properties.h"
 #include "hw/qdev-properties-system.h"
 #include "xen_pt.h"
@@ -951,7 +950,7 @@ void xen_igd_reserve_slot(PCIBus *pci_bus)
     }
 
     XEN_PT_LOG(0, "Reserving PCI slot 2 for IGD\n");
-    pci_bus->slot_reserved_mask |= XEN_PCI_IGD_SLOT_MASK;
+    pci_bus_set_slot_reserved_mask(pci_bus, XEN_PCI_IGD_SLOT_MASK);
 }
 
 static void xen_igd_clear_slot(DeviceState *qdev, Error **errp)
@@ -971,7 +970,7 @@ static void xen_igd_clear_slot(DeviceState *qdev, Error **errp)
         return;
     }
 
-    if (!(pci_bus->slot_reserved_mask & XEN_PCI_IGD_SLOT_MASK)) {
+    if (!(pci_bus_get_slot_reserved_mask(pci_bus) & XEN_PCI_IGD_SLOT_MASK)) {
         xpdc->pci_qdev_realize(qdev, errp);
         return;
     }
@@ -982,7 +981,7 @@ static void xen_igd_clear_slot(DeviceState *qdev, Error **errp)
         s->real_device.dev == XEN_PCI_IGD_DEV &&
         s->real_device.func == XEN_PCI_IGD_FN &&
         s->real_device.vendor_id == PCI_VENDOR_ID_INTEL) {
-        pci_bus->slot_reserved_mask &= ~XEN_PCI_IGD_SLOT_MASK;
+        pci_bus_clear_slot_reserved_mask(pci_bus, XEN_PCI_IGD_SLOT_MASK);
         XEN_PT_LOG(pci_dev, "Intel IGD found, using slot 2\n");
     }
     xpdc->pci_qdev_realize(qdev, errp);
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index d5a40cd058..935b4b91b4 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -287,6 +287,9 @@ void pci_bus_irqs(PCIBus *bus, pci_set_irq_fn set_irq,
 void pci_bus_map_irqs(PCIBus *bus, pci_map_irq_fn map_irq);
 void pci_bus_irqs_cleanup(PCIBus *bus);
 int pci_bus_get_irq_level(PCIBus *bus, int irq_num);
+uint32_t pci_bus_get_slot_reserved_mask(PCIBus *bus);
+void pci_bus_set_slot_reserved_mask(PCIBus *bus, uint32_t mask);
+void pci_bus_clear_slot_reserved_mask(PCIBus *bus, uint32_t mask);
 /* 0 <= pin <= 3 0 = INTA, 1 = INTB, 2 = INTC, 3 = INTD */
 static inline int pci_swizzle(int slot, int pin)
 {
-- 
2.39.2

[PATCH v3 2/2] pci: introduce slot_reserved_auto_mask and slot_reserved_manual_mask

[Chuck Zmudzinski]

Commit 4f67543bb8c5 ("xen/pt: reserve PCI slot 2 for Intel igd-passthru")
uses slot_reserved_mask to reserve slot 2 for the Intel igd for the
xenfv machine when the guest is configured for igd-passthru.

Prior to that commit, a single 32-bit mask was sufficient to meet the
needs of the only machine that used the 32-bit slot_reserved_mask, the
sun4u machine. However, the requirements of the xenfv machine with
igd-passthru is somewhat different from the requirements of the sun4u
machine.

First, the sun4u machine reserves slots in such a way that no device
can be assigned to a reserved slot, but the xenfv machine needs to
reserve a single slot that is reserved for a particular device, the
Intel igd. The necessary logic to ensure that the reserved slot is used
by the Intel igd was recently added by the aforementioned commit.

Second, it is useful to limit slot reservation in the case of the xenfv
machine with the Intel igd to devices configured for automatic slot
assignment so an administrator can assign a device to the reserved slot
by manually specifying the reserved slot address on the command line,
but the sun4u machine requires slot reservation for all devices, whether
or not the device is configured for automatic slot assignment or
configured manually by specifying a slot address on the command line. In
other words, for the sun4u machine, the required behavior is that an
attempt to assign a reserved slot to a device must always result in an
error, but it is useful to allow manual assignment of a reserved slot to
succeed for the xenfv machine with the Intel igd.

The necessary logic to implement the desired behavior of reserving one
or more slots only for the case of automatic slot allocation has not yet
been implemented, and that is the purpose of this patch.

The implementation is simple: the xenfv machine only sets
slot_reserved_auto_mask and the sun4u machine sets both
slot_reserved_manual_mask and slot_reserved_auto_mask. A single
"set" accessor function allows xenfv and sun4u machines to set the
value of the two masks appropriately for each use case.

Since the xenfv machine needs to implement additional logic to detect
the Intel igd and clear the bit in the mask to allow a particular device
to use the reserved slot, there is a need for a "get" and "clear" accessor
function for slot_reserved_auto_mask, but these accessor functions are
not needed for slot_reserved_manual_mask because the sun4u machine has
no need to get the value of the mask or clear bits in the mask.

Link: https://lore.kernel.org/qemu-devel/20230106064838-mutt-send-email-mst@kernel.org/
Signed-off-by: Chuck Zmudzinski <brchuckz@aol.com>
---
Changelog

v3: Change Subject of patch from
    "pci: allow slot_reserved_mask to be ignored with manual slot assignment" To
    "pci: introduce slot_reserved_auto_mask and slot_reserved_manual_mask"
    
    Substantially reword the commit message to clearly explain the reasons
    this patch is needed

    Apply changes in response to comments on v2:
       - slot_reserved_mask -> slot_reserved_auto_mask
       - remove enforce_slot_reserved_mask_manual
       - remove pci_bus_ignore_slot_reserved_mask_manual
       - add slot_reserved_manual_mask
       - pci_bus_devfn_reserved -> pci_bus_devfn_reserved_auto
       - change code in pci_bus_devfn_reserved_manual appropriately
       - pci_bus_set_slot_reserved_mask -> pci_bus_set_slot_reserved_masks
           - use renamed "set" function to set value of both masks for sun4u and xenfv cases
       - pci_bus_get_slot_reserved_mask -> pci_bus_get_slot_reserved_auto_mask
       - pci_bus_clear_slot_reserved_mask -> pci_bus_clear_slot_reserved_auto_mask

v2: Change Subject of patch from
    "pci: add enforce_slot_reserved_mask_manual property" To
    "pci: allow slot_reserved_mask to be ignored with manual slot assignment"

    Add pci_bus_ignore_slot_reserved_mask_manual function

    Call pci_bus_ignore_slot_reserved_mask_manual at appropriate place
    in hw/pci-host/i440fx.c

 hw/pci/pci.c             | 29 ++++++++++++++++++-----------
 hw/sparc64/sun4u.c       |  6 +++---
 hw/xen/xen_pt.c          |  6 +++---
 include/hw/pci/pci.h     |  6 +++---
 include/hw/pci/pci_bus.h |  3 ++-
 5 files changed, 29 insertions(+), 21 deletions(-)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 8a87ccc8b0..58a530a651 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -500,7 +500,8 @@ static void pci_root_bus_internal_init(PCIBus *bus, DeviceState *parent,
 {
     assert(PCI_FUNC(devfn_min) == 0);
     bus->devfn_min = devfn_min;
-    bus->slot_reserved_mask = 0x0;
+    bus->slot_reserved_auto_mask = 0x0;
+    bus->slot_reserved_manual_mask = 0x0;
     bus->address_space_mem = address_space_mem;
     bus->address_space_io = address_space_io;
     bus->flags |= PCI_BUS_IS_ROOT;
@@ -1111,24 +1112,30 @@ static bool pci_bus_devfn_available(PCIBus *bus, int devfn)
     return !(bus->devices[devfn]);
 }
 
-static bool pci_bus_devfn_reserved(PCIBus *bus, int devfn)
+static bool pci_bus_devfn_reserved_auto(PCIBus *bus, int devfn)
 {
-    return bus->slot_reserved_mask & (1UL << PCI_SLOT(devfn));
+    return bus->slot_reserved_auto_mask & (1UL << PCI_SLOT(devfn));
 }
 
-uint32_t pci_bus_get_slot_reserved_mask(PCIBus *bus)
+static bool pci_bus_devfn_reserved_manual(PCIBus *bus, int devfn)
 {
-    return bus->slot_reserved_mask;
+    return bus->slot_reserved_manual_mask & (1UL << PCI_SLOT(devfn));
 }
 
-void pci_bus_set_slot_reserved_mask(PCIBus *bus, uint32_t mask)
+void pci_bus_set_slot_reserved_masks(PCIBus *bus, uint32_t auto_mask, uint32_t manual_mask)
 {
-    bus->slot_reserved_mask |= mask;
+    bus->slot_reserved_auto_mask |= auto_mask;
+    bus->slot_reserved_manual_mask |= manual_mask;
 }
 
-void pci_bus_clear_slot_reserved_mask(PCIBus *bus, uint32_t mask)
+void pci_bus_clear_slot_reserved_auto_mask(PCIBus *bus, uint32_t mask)
 {
-    bus->slot_reserved_mask &= ~mask;
+    bus->slot_reserved_auto_mask &= ~mask;
+}
+
+uint32_t pci_bus_get_slot_reserved_auto_mask(PCIBus *bus)
+{
+    return bus->slot_reserved_auto_mask;
 }
 
 /* -1 for devfn means auto assign */
@@ -1156,7 +1163,7 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev,
         for(devfn = bus->devfn_min ; devfn < ARRAY_SIZE(bus->devices);
             devfn += PCI_FUNC_MAX) {
             if (pci_bus_devfn_available(bus, devfn) &&
-                   !pci_bus_devfn_reserved(bus, devfn)) {
+                   !pci_bus_devfn_reserved_auto(bus, devfn)) {
                 goto found;
             }
         }
@@ -1164,7 +1171,7 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev,
                    "or reserved", name);
         return NULL;
     found: ;
-    } else if (pci_bus_devfn_reserved(bus, devfn)) {
+    } else if (pci_bus_devfn_reserved_manual(bus, devfn)) {
         error_setg(errp, "PCI: slot %d function %d not available for %s,"
                    " reserved",
                    PCI_SLOT(devfn), PCI_FUNC(devfn), name);
diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
index eae7589462..21ab12f6f7 100644
--- a/hw/sparc64/sun4u.c
+++ b/hw/sparc64/sun4u.c
@@ -607,9 +607,9 @@ static void sun4uv_init(MemoryRegion *address_space_mem,
     /* Only in-built Simba APBs can exist on the root bus, slot 0 on busA is
        reserved (leaving no slots free after on-board devices) however slots
        0-3 are free on busB */
-    pci_bus_set_slot_reserved_mask(pci_bus, 0xfffffffc);
-    pci_bus_set_slot_reserved_mask(pci_busA, 0xfffffff1);
-    pci_bus_set_slot_reserved_mask(pci_busB, 0xfffffff0);
+    pci_bus_set_slot_reserved_masks(pci_bus, 0xfffffffc, 0xfffffffc);
+    pci_bus_set_slot_reserved_masks(pci_busA, 0xfffffff1, 0xfffffff1);
+    pci_bus_set_slot_reserved_masks(pci_busB, 0xfffffff0, 0xfffffff0);
 
     ebus = pci_new_multifunction(PCI_DEVFN(1, 0), true, TYPE_EBUS);
     qdev_prop_set_uint64(DEVICE(ebus), "console-serial-base",
diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
index a540149639..3d3290ec02 100644
--- a/hw/xen/xen_pt.c
+++ b/hw/xen/xen_pt.c
@@ -950,7 +950,7 @@ void xen_igd_reserve_slot(PCIBus *pci_bus)
     }
 
     XEN_PT_LOG(0, "Reserving PCI slot 2 for IGD\n");
-    pci_bus_set_slot_reserved_mask(pci_bus, XEN_PCI_IGD_SLOT_MASK);
+    pci_bus_set_slot_reserved_masks(pci_bus, XEN_PCI_IGD_SLOT_MASK, 0x0);
 }
 
 static void xen_igd_clear_slot(DeviceState *qdev, Error **errp)
@@ -970,7 +970,7 @@ static void xen_igd_clear_slot(DeviceState *qdev, Error **errp)
         return;
     }
 
-    if (!(pci_bus_get_slot_reserved_mask(pci_bus) & XEN_PCI_IGD_SLOT_MASK)) {
+    if (!(pci_bus_get_slot_reserved_auto_mask(pci_bus) & XEN_PCI_IGD_SLOT_MASK)) {
         xpdc->pci_qdev_realize(qdev, errp);
         return;
     }
@@ -981,7 +981,7 @@ static void xen_igd_clear_slot(DeviceState *qdev, Error **errp)
         s->real_device.dev == XEN_PCI_IGD_DEV &&
         s->real_device.func == XEN_PCI_IGD_FN &&
         s->real_device.vendor_id == PCI_VENDOR_ID_INTEL) {
-        pci_bus_clear_slot_reserved_mask(pci_bus, XEN_PCI_IGD_SLOT_MASK);
+        pci_bus_clear_slot_reserved_auto_mask(pci_bus, XEN_PCI_IGD_SLOT_MASK);
         XEN_PT_LOG(pci_dev, "Intel IGD found, using slot 2\n");
     }
     xpdc->pci_qdev_realize(qdev, errp);
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index 935b4b91b4..7e941e1c87 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -287,9 +287,9 @@ void pci_bus_irqs(PCIBus *bus, pci_set_irq_fn set_irq,
 void pci_bus_map_irqs(PCIBus *bus, pci_map_irq_fn map_irq);
 void pci_bus_irqs_cleanup(PCIBus *bus);
 int pci_bus_get_irq_level(PCIBus *bus, int irq_num);
-uint32_t pci_bus_get_slot_reserved_mask(PCIBus *bus);
-void pci_bus_set_slot_reserved_mask(PCIBus *bus, uint32_t mask);
-void pci_bus_clear_slot_reserved_mask(PCIBus *bus, uint32_t mask);
+uint32_t pci_bus_get_slot_reserved_auto_mask(PCIBus *bus);
+void pci_bus_set_slot_reserved_masks(PCIBus *bus, uint32_t auto_mask, uint32_t manual_mask);
+void pci_bus_clear_slot_reserved_auto_mask(PCIBus *bus, uint32_t mask);
 /* 0 <= pin <= 3 0 = INTA, 1 = INTB, 2 = INTC, 3 = INTD */
 static inline int pci_swizzle(int slot, int pin)
 {
diff --git a/include/hw/pci/pci_bus.h b/include/hw/pci/pci_bus.h
index 5653175957..3c2b3b8e32 100644
--- a/include/hw/pci/pci_bus.h
+++ b/include/hw/pci/pci_bus.h
@@ -36,7 +36,8 @@ struct PCIBus {
     PCIIOMMUFunc iommu_fn;
     void *iommu_opaque;
     uint8_t devfn_min;
-    uint32_t slot_reserved_mask;
+    uint32_t slot_reserved_auto_mask;
+    uint32_t slot_reserved_manual_mask;
     pci_set_irq_fn set_irq;
     pci_map_irq_fn map_irq;
     pci_route_irq_fn route_intx_to_irq;
-- 
2.39.2