Re: [PATCH v2 2/2] pci: allow slot_reserved_mask to be ignored with manual slot assignment

[Mark Cave-Ayland <mark.cave-ayland@xxxxxxxxxxxx>]

On 14/03/2023 06:33, Michael S. Tsirkin wrote:

> On Tue, Mar 14, 2023 at 12:01:09AM -0400, Chuck Zmudzinski wrote:
> > 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.
> >
> > A desired extension to that commit is to allow use of the reserved slot
> > if the administrator manually configures a device to use the reserved
> > slot. Currently, slot_reserved_mask is enforced unconditionally. With
> > this patch, the pci bus can be configured so the slot is only reserved
> > if the pci device to be added to the bus is configured for automatic
> > slot assignment.
> >
> > To enable the desired behavior of slot_reserved_mask machine, add a
> > boolean member enforce_slot_reserved_mask_manual to struct PCIBus and
> > add a function pci_bus_ignore_slot_reserved_mask_manual which can be
> > called to change the default behavior of always enforcing
> > slot_reserved_mask so, in that case, slot_reserved_mask is only enforced
> > when the pci device being added is configured for automatic slot
> > assignment.
> >
> > Call the new pci_bus_ignore_slot_reserved_mask_manual function after
> > creating the pci bus for the pc/i440fx/xenfv machine type to implement
> > the desired behavior of causing slot_reserved_mask to only apply when
> > the pci device to be added to a pc/i440fx/xenfv machine is configured
> > for automatic slot assignment.
> >
> > Link: 
> > https://lore.kernel.org/qemu-devel/20230106064838-mutt-send-email-mst@xxxxxxxxxx/
> > Signed-off-by: Chuck Zmudzinski <brchuckz@xxxxxxx>
> I really dislike this.
> It seems that xen should not have used slot_reserved_mask,
> and instead needs something new like slot_manual_mask.
> No?
My suggestion was to move the validation logic to a separate callback function in 
PCIBus (see https://lists.gnu.org/archive/html/qemu-devel/2023-03/msg03988.html) but 
perhaps I wasn't clear enough in pointing out that I was thinking this could 
*replace* the existing slot_reserved_mask mechanism, rather than providing a hook to 
allow it to be manipulated.
Here's a very rough patch put together over lunch that attempts this for 
pci_bus_devfn_reserved(): the idea is that sun4u and Xen would call 
pci_bus_set_slot_reserved_fn() with a suitable pci_slot_reserved_fn implementation, 
and slot_reserved_mask gets removed completely i.e.:

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index def5000e7b..30b856499a 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -493,6 +493,13 @@ bool pci_bus_bypass_iommu(PCIBus *bus)
     return host_bridge->bypass_iommu;
 }

+static bool pci_bus_default_slot_reserved(PCISlotReservationType restype,
+                                          int devfn)
+{
+    /* All slots accessible by default */
+    return false;
+}
+
 static void pci_root_bus_internal_init(PCIBus *bus, DeviceState *parent,
                                        MemoryRegion *address_space_mem,
                                        MemoryRegion *address_space_io,
@@ -500,7 +507,7 @@ 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_fn = pci_bus_default_slot_reserved;
     bus->address_space_mem = address_space_mem;
     bus->address_space_io = address_space_io;
     bus->flags |= PCI_BUS_IS_ROOT;
@@ -1111,9 +1118,15 @@ 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(PCIBus *bus, int devfn,
+                                   PCISlotReservationType restype)
+{
+    return bus->slot_reserved_fn(restype, devfn);
+}
+
+void pci_bus_set_slot_reserved_fn(PCIBus *bus, pci_slot_reserved_fn fn)
 {
-    return bus->slot_reserved_mask & (1UL << PCI_SLOT(devfn));
+    bus->slot_reserved_fn = fn;
 }

 /* -1 for devfn means auto assign */
@@ -1141,7 +1154,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(bus, devfn, 
PCI_SLOT_RESERVATION_AUTO)) {
                 goto found;
             }
         }
@@ -1149,7 +1162,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(bus, devfn, 
PCI_SLOT_RESERVATION_MANUAL)) {
         error_setg(errp, "PCI: slot %d function %d not available for %s,"
                                        MemoryRegion *address_space_io,
@@ -500,7 +507,7 @@ 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_fn = pci_bus_default_slot_reserved;
     bus->address_space_mem = address_space_mem;
     bus->address_space_io = address_space_io;
     bus->flags |= PCI_BUS_IS_ROOT;
@@ -1111,9 +1118,15 @@ 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(PCIBus *bus, int devfn,
+                                   PCISlotReservationType restype)
+{
+    return bus->slot_reserved_fn(restype, devfn);
+}
+
+void pci_bus_set_slot_reserved_fn(PCIBus *bus, pci_slot_reserved_fn fn)
 {
-    return bus->slot_reserved_mask & (1UL << PCI_SLOT(devfn));
+    bus->slot_reserved_fn = fn;
 }

 /* -1 for devfn means auto assign */
@@ -1141,7 +1154,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(bus, devfn, 
PCI_SLOT_RESERVATION_AUTO)) {
                 goto found;
             }
         }
@@ -1149,7 +1162,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(bus, devfn, 
PCI_SLOT_RESERVATION_MANUAL)) {
         error_setg(errp, "PCI: slot %d function %d not available for %s,"
                    " reserved",
                    PCI_SLOT(devfn), PCI_FUNC(devfn), name);
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index d5a40cd058..8a949f7ae1 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -257,10 +257,18 @@ MemoryRegion *pci_address_space_io(PCIDevice *dev);
  */
 int pci_bar(PCIDevice *d, int reg);

+typedef enum PCISlotReservationType {
+    PCI_SLOT_RESERVATION_AUTO,
+    PCI_SLOT_RESERVATION_MANUAL
+} PCISlotReservationType;
+
+typedef bool (*pci_slot_reserved_fn)(PCISlotReservationType restype, int 
devfn);
 typedef void (*pci_set_irq_fn)(void *opaque, int irq_num, int level);
 typedef int (*pci_map_irq_fn)(PCIDevice *pci_dev, int irq_num);
 typedef PCIINTxRoute (*pci_route_irq_fn)(void *opaque, int pin);

+void pci_bus_set_slot_reserved_fn(PCIBus *bus, pci_slot_reserved_fn fn);
+
 #define TYPE_PCI_BUS "PCI"
 OBJECT_DECLARE_TYPE(PCIBus, PCIBusClass, PCI_BUS)
 #define TYPE_PCIE_BUS "PCIE"
diff --git a/include/hw/pci/pci_bus.h b/include/hw/pci/pci_bus.h
index 5653175957..d68ea1418d 100644
--- a/include/hw/pci/pci_bus.h
+++ b/include/hw/pci/pci_bus.h
@@ -36,7 +36,7 @@ struct PCIBus {
     PCIIOMMUFunc iommu_fn;
     void *iommu_opaque;
     uint8_t devfn_min;
-    uint32_t slot_reserved_mask;
+    pci_slot_reserved_fn slot_reserved_fn;
     pci_set_irq_fn set_irq;
     pci_map_irq_fn map_irq;
     pci_route_irq_fn route_intx_to_irq;


If this approach seems reasonable, I'm happy for someone else to take this over and 
turn it into a proper series.

ATB,

Mark.