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

Commit Message

Chuck Zmudzinski March 14, 2023, 4:01 a.m. UTC

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/[e-mail]/
Signed-off-by: Chuck Zmudzinski <[e-mail]>
---
Changelog

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-host/i440fx.c     |  1 +
 hw/pci/pci.c             | 14 +++++++++++++-
 include/hw/pci/pci.h     |  1 +
 include/hw/pci/pci_bus.h |  1 +
 4 files changed, 16 insertions(+), 1 deletion(-)

Comments

Michael S. Tsirkin March 14, 2023, 6:33 a.m. UTC | #1

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/[e-mail]/
> Signed-off-by: Chuck Zmudzinski <[e-mail]>

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?

> ---
> Changelog
> 
> 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-host/i440fx.c     |  1 +
>  hw/pci/pci.c             | 14 +++++++++++++-
>  include/hw/pci/pci.h     |  1 +
>  include/hw/pci/pci_bus.h |  1 +
>  4 files changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/pci-host/i440fx.c b/hw/pci-host/i440fx.c
> index 262f82c303..8e00b88926 100644
> --- a/hw/pci-host/i440fx.c
> +++ b/hw/pci-host/i440fx.c
> @@ -257,6 +257,7 @@ PCIBus *i440fx_init(const char *pci_type,
>      s = PCI_HOST_BRIDGE(dev);
>      b = pci_root_bus_new(dev, NULL, pci_address_space,
>                           address_space_io, 0, TYPE_PCI_BUS);
> +    pci_bus_ignore_slot_reserved_mask_manual(b);
>      s->bus = b;
>      object_property_add_child(qdev_get_machine(), "i440fx", OBJECT(dev));
>      sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 8a87ccc8b0..670ecc6986 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -501,6 +501,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->enforce_slot_reserved_mask_manual = true;
>      bus->address_space_mem = address_space_mem;
>      bus->address_space_io = address_space_io;
>      bus->flags |= PCI_BUS_IS_ROOT;
> @@ -1116,6 +1117,17 @@ static bool pci_bus_devfn_reserved(PCIBus *bus, int devfn)
>      return bus->slot_reserved_mask & (1UL << PCI_SLOT(devfn));
>  }
>  
> +static bool pci_bus_devfn_reserved_manual(PCIBus *bus, int devfn)
> +{
> +    return bus->enforce_slot_reserved_mask_manual &&
> +            (bus->slot_reserved_mask & (1UL << PCI_SLOT(devfn)));
> +}
> +
> +void pci_bus_ignore_slot_reserved_mask_manual(PCIBus *bus)
> +{
> +    bus->enforce_slot_reserved_mask_manual = false;
> +}
> +
>  uint32_t pci_bus_get_slot_reserved_mask(PCIBus *bus)
>  {
>      return bus->slot_reserved_mask;
> @@ -1164,7 +1176,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/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index 935b4b91b4..48d29ec234 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -287,6 +287,7 @@ 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);
> +void pci_bus_ignore_slot_reserved_mask_manual(PCIBus *bus);
>  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);
> diff --git a/include/hw/pci/pci_bus.h b/include/hw/pci/pci_bus.h
> index 5653175957..e0f15ee9be 100644
> --- a/include/hw/pci/pci_bus.h
> +++ b/include/hw/pci/pci_bus.h
> @@ -37,6 +37,7 @@ struct PCIBus {
>      void *iommu_opaque;
>      uint8_t devfn_min;
>      uint32_t slot_reserved_mask;
> +    bool enforce_slot_reserved_mask_manual;
>      pci_set_irq_fn set_irq;
>      pci_map_irq_fn map_irq;
>      pci_route_irq_fn route_intx_to_irq;
> -- 
> 2.39.2

Chuck Zmudzinski March 14, 2023, 12:33 p.m. UTC | #2

On 3/14/2023 2:33 AM, 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/[e-mail]/
> > Signed-off-by: Chuck Zmudzinski <[e-mail]>
>
> 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?

Actually, xen would use something like slot_auto_mask, and
sun4u would use both slot_auto_mask and slot_manual_mask.

Is it just that this patch touches hw/pci-host/i440fx.c that you
don't like or is it that you don't like adding slot_reserved_mask_manual
and pci_bus_ignore_slot_reserved_mask_manual, or is it both
that you don't like?

If it's the former that you don't like, the call to
pci_bus_ignore_slot_reserved_mask_manual can be moved to
xen_igd_reserve_slot in hw/xen/xen_pt.c and this would
avoid touching hw/pci-host/i440fx.c.

If it's the latter that you don't like, both slot_reserved_mask_manual
and pci_bus_ignore_slot_reserved_mask_manual can be removed
and this can be implemented with two independent slot masks:

rename slot_reserved_mask as slot_auto_mask - used by both xen and sun4u
slot_manual_mask - new mask, used only by sun4u.

We would also need to have two sets of accessor functions in this case, one
set to access slot_auto_mask, and the other to access slot_manual_mask.
Since the sun4u machine does not need to either get the value of
slot_manual_mask or clear the slot_manual_mask, slot_manual_mask
would only need to have one accessor function to set the value of the
mask. slot_auto_mask would have all three accessor functions that xen
needs to use.

Would that be OK?

>
> > ---
> > Changelog
> > 
> > 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-host/i440fx.c     |  1 +
> >  hw/pci/pci.c             | 14 +++++++++++++-
> >  include/hw/pci/pci.h     |  1 +
> >  include/hw/pci/pci_bus.h |  1 +
> >  4 files changed, 16 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/pci-host/i440fx.c b/hw/pci-host/i440fx.c
> > index 262f82c303..8e00b88926 100644
> > --- a/hw/pci-host/i440fx.c
> > +++ b/hw/pci-host/i440fx.c
> > @@ -257,6 +257,7 @@ PCIBus *i440fx_init(const char *pci_type,
> >      s = PCI_HOST_BRIDGE(dev);
> >      b = pci_root_bus_new(dev, NULL, pci_address_space,
> >                           address_space_io, 0, TYPE_PCI_BUS);
> > +    pci_bus_ignore_slot_reserved_mask_manual(b);
> >      s->bus = b;
> >      object_property_add_child(qdev_get_machine(), "i440fx", OBJECT(dev));
> >      sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
> > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > index 8a87ccc8b0..670ecc6986 100644
> > --- a/hw/pci/pci.c
> > +++ b/hw/pci/pci.c
> > @@ -501,6 +501,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->enforce_slot_reserved_mask_manual = true;
> >      bus->address_space_mem = address_space_mem;
> >      bus->address_space_io = address_space_io;
> >      bus->flags |= PCI_BUS_IS_ROOT;
> > @@ -1116,6 +1117,17 @@ static bool pci_bus_devfn_reserved(PCIBus *bus, int devfn)
> >      return bus->slot_reserved_mask & (1UL << PCI_SLOT(devfn));
> >  }
> >  
> > +static bool pci_bus_devfn_reserved_manual(PCIBus *bus, int devfn)
> > +{
> > +    return bus->enforce_slot_reserved_mask_manual &&
> > +            (bus->slot_reserved_mask & (1UL << PCI_SLOT(devfn)));
> > +}
> > +
> > +void pci_bus_ignore_slot_reserved_mask_manual(PCIBus *bus)
> > +{
> > +    bus->enforce_slot_reserved_mask_manual = false;
> > +}
> > +
> >  uint32_t pci_bus_get_slot_reserved_mask(PCIBus *bus)
> >  {
> >      return bus->slot_reserved_mask;
> > @@ -1164,7 +1176,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/include/hw/pci/pci.h b/include/hw/pci/pci.h
> > index 935b4b91b4..48d29ec234 100644
> > --- a/include/hw/pci/pci.h
> > +++ b/include/hw/pci/pci.h
> > @@ -287,6 +287,7 @@ 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);
> > +void pci_bus_ignore_slot_reserved_mask_manual(PCIBus *bus);
> >  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);
> > diff --git a/include/hw/pci/pci_bus.h b/include/hw/pci/pci_bus.h
> > index 5653175957..e0f15ee9be 100644
> > --- a/include/hw/pci/pci_bus.h
> > +++ b/include/hw/pci/pci_bus.h
> > @@ -37,6 +37,7 @@ struct PCIBus {
> >      void *iommu_opaque;
> >      uint8_t devfn_min;
> >      uint32_t slot_reserved_mask;
> > +    bool enforce_slot_reserved_mask_manual;
> >      pci_set_irq_fn set_irq;
> >      pci_map_irq_fn map_irq;
> >      pci_route_irq_fn route_intx_to_irq;
> > -- 
> > 2.39.2
>

Mark Cave-Ayland March 14, 2023, 12:43 p.m. UTC | #3

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/[e-mail]/
>> Signed-off-by: Chuck Zmudzinski <[e-mail]>
> 
> 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.

Michael S. Tsirkin March 14, 2023, 1:16 p.m. UTC | #4

On Tue, Mar 14, 2023 at 08:33:02AM -0400, Chuck Zmudzinski wrote:
> On 3/14/2023 2:33 AM, 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/[e-mail]/
> > > Signed-off-by: Chuck Zmudzinski <[e-mail]>
> >
> > 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?
> 
> Actually, xen would use something like slot_auto_mask, and
> sun4u would use both slot_auto_mask and slot_manual_mask.
> 
> Is it just that this patch touches hw/pci-host/i440fx.c that you
> don't like or is it that you don't like adding slot_reserved_mask_manual
> and pci_bus_ignore_slot_reserved_mask_manual, or is it both
> that you don't like?

I don't like the enforce_slot_reserved_mask_manual flag -
I prefer straight forward logic with no branches in
the common code.

> If it's the former that you don't like, the call to
> pci_bus_ignore_slot_reserved_mask_manual can be moved to
> xen_igd_reserve_slot in hw/xen/xen_pt.c and this would
> avoid touching hw/pci-host/i440fx.c.
> 
> If it's the latter that you don't like, both slot_reserved_mask_manual
> and pci_bus_ignore_slot_reserved_mask_manual can be removed
> and this can be implemented with two independent slot masks:
> 
> rename slot_reserved_mask as slot_auto_mask - used by both xen and sun4u
> slot_manual_mask - new mask, used only by sun4u.

Sounds good to me, except let's add "reserved" in here.
slot_reserved_mask_auto, slot_reserved_mask_manual ?

> We would also need to have two sets of accessor functions in this case, one
> set to access slot_auto_mask, and the other to access slot_manual_mask.
> Since the sun4u machine does not need to either get the value of
> slot_manual_mask or clear the slot_manual_mask, slot_manual_mask
> would only need to have one accessor function to set the value of the
> mask. slot_auto_mask would have all three accessor functions that xen
> needs to use.
> 
> Would that be OK?


Sounds good to me.

> >
> > > ---
> > > Changelog
> > > 
> > > 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-host/i440fx.c     |  1 +
> > >  hw/pci/pci.c             | 14 +++++++++++++-
> > >  include/hw/pci/pci.h     |  1 +
> > >  include/hw/pci/pci_bus.h |  1 +
> > >  4 files changed, 16 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/hw/pci-host/i440fx.c b/hw/pci-host/i440fx.c
> > > index 262f82c303..8e00b88926 100644
> > > --- a/hw/pci-host/i440fx.c
> > > +++ b/hw/pci-host/i440fx.c
> > > @@ -257,6 +257,7 @@ PCIBus *i440fx_init(const char *pci_type,
> > >      s = PCI_HOST_BRIDGE(dev);
> > >      b = pci_root_bus_new(dev, NULL, pci_address_space,
> > >                           address_space_io, 0, TYPE_PCI_BUS);
> > > +    pci_bus_ignore_slot_reserved_mask_manual(b);
> > >      s->bus = b;
> > >      object_property_add_child(qdev_get_machine(), "i440fx", OBJECT(dev));
> > >      sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
> > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > > index 8a87ccc8b0..670ecc6986 100644
> > > --- a/hw/pci/pci.c
> > > +++ b/hw/pci/pci.c
> > > @@ -501,6 +501,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->enforce_slot_reserved_mask_manual = true;
> > >      bus->address_space_mem = address_space_mem;
> > >      bus->address_space_io = address_space_io;
> > >      bus->flags |= PCI_BUS_IS_ROOT;
> > > @@ -1116,6 +1117,17 @@ static bool pci_bus_devfn_reserved(PCIBus *bus, int devfn)
> > >      return bus->slot_reserved_mask & (1UL << PCI_SLOT(devfn));
> > >  }
> > >  
> > > +static bool pci_bus_devfn_reserved_manual(PCIBus *bus, int devfn)
> > > +{
> > > +    return bus->enforce_slot_reserved_mask_manual &&
> > > +            (bus->slot_reserved_mask & (1UL << PCI_SLOT(devfn)));
> > > +}
> > > +
> > > +void pci_bus_ignore_slot_reserved_mask_manual(PCIBus *bus)
> > > +{
> > > +    bus->enforce_slot_reserved_mask_manual = false;
> > > +}
> > > +
> > >  uint32_t pci_bus_get_slot_reserved_mask(PCIBus *bus)
> > >  {
> > >      return bus->slot_reserved_mask;
> > > @@ -1164,7 +1176,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/include/hw/pci/pci.h b/include/hw/pci/pci.h
> > > index 935b4b91b4..48d29ec234 100644
> > > --- a/include/hw/pci/pci.h
> > > +++ b/include/hw/pci/pci.h
> > > @@ -287,6 +287,7 @@ 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);
> > > +void pci_bus_ignore_slot_reserved_mask_manual(PCIBus *bus);
> > >  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);
> > > diff --git a/include/hw/pci/pci_bus.h b/include/hw/pci/pci_bus.h
> > > index 5653175957..e0f15ee9be 100644
> > > --- a/include/hw/pci/pci_bus.h
> > > +++ b/include/hw/pci/pci_bus.h
> > > @@ -37,6 +37,7 @@ struct PCIBus {
> > >      void *iommu_opaque;
> > >      uint8_t devfn_min;
> > >      uint32_t slot_reserved_mask;
> > > +    bool enforce_slot_reserved_mask_manual;
> > >      pci_set_irq_fn set_irq;
> > >      pci_map_irq_fn map_irq;
> > >      pci_route_irq_fn route_intx_to_irq;
> > > -- 
> > > 2.39.2
> >

Michael S. Tsirkin March 14, 2023, 1:17 p.m. UTC | #5

On Tue, Mar 14, 2023 at 12:43:12PM +0000, Mark Cave-Ayland wrote:
> 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/[e-mail]/
> > > Signed-off-by: Chuck Zmudzinski <[e-mail]>
> > 
> > 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.

It's ok too though I think I like chuck's proposal better:
less callbacks to chase.

Chuck Zmudzinski March 14, 2023, 1:26 p.m. UTC | #6

On 3/14/2023 9:17 AM, Michael S. Tsirkin wrote:
> On Tue, Mar 14, 2023 at 12:43:12PM +0000, Mark Cave-Ayland wrote:
> > 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/[e-mail]/
> > > > Signed-off-by: Chuck Zmudzinski <[e-mail]>
> > > 
> > > 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.
>
> It's ok too though I think I like chuck's proposal better:
> less callbacks to chase.
>

I would be willing to pursue this if there were more use cases for
slot_reserved_mask than just the two cases we have now: xen and sun4u.
Until there is a clear demand for a more general way to manipulate the
mask, I agree with Michael that the KISS principle should apply here.

Kind regards,

Chuck

Mark Cave-Ayland March 14, 2023, 1:41 p.m. UTC | #7

On 14/03/2023 13:26, Chuck Zmudzinski wrote:

> On 3/14/2023 9:17 AM, Michael S. Tsirkin wrote:
>> On Tue, Mar 14, 2023 at 12:43:12PM +0000, Mark Cave-Ayland wrote:
>>> 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/[e-mail]/
>>>>> Signed-off-by: Chuck Zmudzinski <[e-mail]>
>>>>
>>>> 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.
>>
>> It's ok too though I think I like chuck's proposal better:
>> less callbacks to chase.
>>
> 
> I would be willing to pursue this if there were more use cases for
> slot_reserved_mask than just the two cases we have now: xen and sun4u.
> Until there is a clear demand for a more general way to manipulate the
> mask, I agree with Michael that the KISS principle should apply here.

No worries. The thinking behind this idea was that it feel like the Xen case is 
special in that it has separate requirements for auto slot allocation and manual slot 
allocation: if slot reservation were used in more places, I'd expect the sun4u case 
to be more common, in which case it seems a bit more work for the common case to have 
to set both slot_reserved_mask_auto and slot_reserved_mask_manual separately. Perhaps 
a single accessor function can be used to set both mask values together for a PCIBus?

Regardless, I'll take step back and leave you and Michael to come up with a solution 
that you're both happy with. Let me know if you need me to test anything on sun4u.


ATB,

Mark.

Chuck Zmudzinski March 14, 2023, 2:08 p.m. UTC | #8

On 3/14/2023 9:41 AM, Mark Cave-Ayland wrote:
> On 14/03/2023 13:26, Chuck Zmudzinski wrote:
>
> > On 3/14/2023 9:17 AM, Michael S. Tsirkin wrote:
> >> On Tue, Mar 14, 2023 at 12:43:12PM +0000, Mark Cave-Ayland wrote:
> >>> 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/[e-mail]/
> >>>>> Signed-off-by: Chuck Zmudzinski <[e-mail]>
> >>>>
> >>>> 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.
> >>
> >> It's ok too though I think I like chuck's proposal better:
> >> less callbacks to chase.
> >>
> > 
> > I would be willing to pursue this if there were more use cases for
> > slot_reserved_mask than just the two cases we have now: xen and sun4u.
> > Until there is a clear demand for a more general way to manipulate the
> > mask, I agree with Michael that the KISS principle should apply here.
>
> No worries. The thinking behind this idea was that it feel like the Xen case is 
> special in that it has separate requirements for auto slot allocation and manual slot 
> allocation: if slot reservation were used in more places, I'd expect the sun4u case 
> to be more common, in which case it seems a bit more work for the common case to have 
> to set both slot_reserved_mask_auto and slot_reserved_mask_manual separately. Perhaps 
> a single accessor function can be used to set both mask values together for a PCIBus?
>
> Regardless, I'll take step back and leave you and Michael to come up with a solution 
> that you're both happy with. Let me know if you need me to test anything on sun4u.

I will give this a little time to see if anyone else has any ideas here,
and to also more carefully consider Mark's proposal, before posting
v3.

Kind regards,

Chuck

Chuck Zmudzinski March 14, 2023, 2:21 p.m. UTC | #9

On 3/14/2023 9:41 AM, Mark Cave-Ayland wrote:
> On 14/03/2023 13:26, Chuck Zmudzinski wrote:
>
> > On 3/14/2023 9:17 AM, Michael S. Tsirkin wrote:
> >> On Tue, Mar 14, 2023 at 12:43:12PM +0000, Mark Cave-Ayland wrote:
> >>> 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/[e-mail]/
> >>>>> Signed-off-by: Chuck Zmudzinski <[e-mail]>
> >>>>
> >>>> 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.
> >>
> >> It's ok too though I think I like chuck's proposal better:
> >> less callbacks to chase.
> >>
> > 
> > I would be willing to pursue this if there were more use cases for
> > slot_reserved_mask than just the two cases we have now: xen and sun4u.
> > Until there is a clear demand for a more general way to manipulate the
> > mask, I agree with Michael that the KISS principle should apply here.
>
> No worries. The thinking behind this idea was that it feel like the Xen case is 
> special in that it has separate requirements for auto slot allocation and manual slot 
> allocation: if slot reservation were used in more places, I'd expect the sun4u case 
> to be more common, in which case it seems a bit more work for the common case to have 
> to set both slot_reserved_mask_auto and slot_reserved_mask_manual separately. Perhaps 
> a single accessor function can be used to set both mask values together for a PCIBus?

My immediate thought as a way to avoid needing to set both masks in the
sun4u machine would be to add an initialization function that defines the
relationship between the two masks. The sun4u machine would use a default
function that simply sets the manual mask equal to the auto mask. In the
special case of xen, the manual mask would be set to 0.

>
> Regardless, I'll take step back and leave you and Michael to come up with a solution 
> that you're both happy with. Let me know if you need me to test anything on sun4u.
>
>
> ATB,
>
> Mark.

Mark Cave-Ayland March 14, 2023, 2:39 p.m. UTC | #10

On 14/03/2023 14:21, Chuck Zmudzinski wrote:

> On 3/14/2023 9:41 AM, Mark Cave-Ayland wrote:
>> On 14/03/2023 13:26, Chuck Zmudzinski wrote:
>>
>>> On 3/14/2023 9:17 AM, Michael S. Tsirkin wrote:
>>>> On Tue, Mar 14, 2023 at 12:43:12PM +0000, Mark Cave-Ayland wrote:
>>>>> 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/[e-mail]/
>>>>>>> Signed-off-by: Chuck Zmudzinski <[e-mail]>
>>>>>>
>>>>>> 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.
>>>>
>>>> It's ok too though I think I like chuck's proposal better:
>>>> less callbacks to chase.
>>>>
>>>
>>> I would be willing to pursue this if there were more use cases for
>>> slot_reserved_mask than just the two cases we have now: xen and sun4u.
>>> Until there is a clear demand for a more general way to manipulate the
>>> mask, I agree with Michael that the KISS principle should apply here.
>>
>> No worries. The thinking behind this idea was that it feel like the Xen case is
>> special in that it has separate requirements for auto slot allocation and manual slot
>> allocation: if slot reservation were used in more places, I'd expect the sun4u case
>> to be more common, in which case it seems a bit more work for the common case to have
>> to set both slot_reserved_mask_auto and slot_reserved_mask_manual separately. Perhaps
>> a single accessor function can be used to set both mask values together for a PCIBus?
> 
> My immediate thought as a way to avoid needing to set both masks in the
> sun4u machine would be to add an initialization function that defines the
> relationship between the two masks. The sun4u machine would use a default
> function that simply sets the manual mask equal to the auto mask. In the
> special case of xen, the manual mask would be set to 0.

I must admit I wasn't too concerned about having two separate masks, more that in the 
common case you would have to remember to call two functions. I was thinking that 
instead of having separate pci_bus_set_slot_reserved_mask_auto() and 
pci_bus_set_slot_reserved_mask_manual() accessors, you could just have:

   pci_bus_set_slot_reserved_mask(uint32_t auto_mask, uint32_t manual_mask);

since that gives a single function call for all cases, and it seems fairly intuitive 
what pci_bus_set_slot_reserved_mask(foo, foo) is doing vs. 
pci_bus_set_slot_reserved_mask(foo, bar).


ATB,

Mark.

Chuck Zmudzinski March 14, 2023, 5:02 p.m. UTC | #11

On 3/14/2023 10:39 AM, Mark Cave-Ayland wrote:
> On 14/03/2023 14:21, Chuck Zmudzinski wrote:
>
> > On 3/14/2023 9:41 AM, Mark Cave-Ayland wrote:
> >> On 14/03/2023 13:26, Chuck Zmudzinski wrote:
> >>
> >>> On 3/14/2023 9:17 AM, Michael S. Tsirkin wrote:
> >>>> On Tue, Mar 14, 2023 at 12:43:12PM +0000, Mark Cave-Ayland wrote:
> >>>>> 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/[e-mail]/
> >>>>>>> Signed-off-by: Chuck Zmudzinski <[e-mail]>
> >>>>>>
> >>>>>> 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.
> >>>>
> >>>> It's ok too though I think I like chuck's proposal better:
> >>>> less callbacks to chase.
> >>>>
> >>>
> >>> I would be willing to pursue this if there were more use cases for
> >>> slot_reserved_mask than just the two cases we have now: xen and sun4u.
> >>> Until there is a clear demand for a more general way to manipulate the
> >>> mask, I agree with Michael that the KISS principle should apply here.
> >>
> >> No worries. The thinking behind this idea was that it feel like the Xen case is
> >> special in that it has separate requirements for auto slot allocation and manual slot
> >> allocation: if slot reservation were used in more places, I'd expect the sun4u case
> >> to be more common, in which case it seems a bit more work for the common case to have
> >> to set both slot_reserved_mask_auto and slot_reserved_mask_manual separately. Perhaps
> >> a single accessor function can be used to set both mask values together for a PCIBus?
> > 
> > My immediate thought as a way to avoid needing to set both masks in the
> > sun4u machine would be to add an initialization function that defines the
> > relationship between the two masks. The sun4u machine would use a default
> > function that simply sets the manual mask equal to the auto mask. In the
> > special case of xen, the manual mask would be set to 0.
>
> I must admit I wasn't too concerned about having two separate masks, more that in the 
> common case you would have to remember to call two functions. I was thinking that 
> instead of having separate pci_bus_set_slot_reserved_mask_auto() and 
> pci_bus_set_slot_reserved_mask_manual() accessors, you could just have:
>
>    pci_bus_set_slot_reserved_mask(uint32_t auto_mask, uint32_t manual_mask);
>
> since that gives a single function call for all cases, and it seems fairly intuitive 
> what pci_bus_set_slot_reserved_mask(foo, foo) is doing vs. 
> pci_bus_set_slot_reserved_mask(foo, bar).

That looks good to me. I can implement it with the value for each mask
passed as separate arguments of a single function.

I think ultimately, both the sun4u machine and the xenfv machine with
the Intel igd are unique cases, and I don't think one is necessarily more
common that the other. In fact, I agree with Michael that the requirements
of the xenfv machine with Intel igd are different enough from the requirements
of the sun4u machine that it makes sense to use two independent mechanisms
to accommodate each case. For example, in the case of the Intel igd, slot
reservation is not to prevent all pci devices from using the slot but only to
reserve the slot for a particular device: the Intel igd. So, from the point of
view of what is needed for the Intel igd, slot_reserved_mask for
the case of the sun4u machine really means slot_unavailable_mask, not
slot_reserved_mask. Ideally, the kind of mask that the Intel igd needs is
one that releases for the slot for use when the particular device that
is requesting the slot is the actual device the slot is reserved for, which
is more complicated to implement than the single 32-bit mask that works
well for the sun4u machine. Specifically, it needs more information than just
the devfn value to decide whether or not to release the slot for use.

I think that in the long term, it might be best to try to collect all the unique
quirks of the Intel igd into a special pci-host device that both kvm (with legacy
passthrough mode) and xen can use, but that could not be accomplished in
time for the 8.0 release.

I will have v3 ready later today or tomorrow.

Kind regards,

Chuck

Patch

diff --git a/hw/pci-host/i440fx.c b/hw/pci-host/i440fx.c
index 262f82c303..8e00b88926 100644
--- a/hw/pci-host/i440fx.c
+++ b/hw/pci-host/i440fx.c
@@ -257,6 +257,7 @@  PCIBus *i440fx_init(const char *pci_type,
     s = PCI_HOST_BRIDGE(dev);
     b = pci_root_bus_new(dev, NULL, pci_address_space,
                          address_space_io, 0, TYPE_PCI_BUS);
+    pci_bus_ignore_slot_reserved_mask_manual(b);
     s->bus = b;
     object_property_add_child(qdev_get_machine(), "i440fx", OBJECT(dev));
     sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 8a87ccc8b0..670ecc6986 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -501,6 +501,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->enforce_slot_reserved_mask_manual = true;
     bus->address_space_mem = address_space_mem;
     bus->address_space_io = address_space_io;
     bus->flags |= PCI_BUS_IS_ROOT;
@@ -1116,6 +1117,17 @@  static bool pci_bus_devfn_reserved(PCIBus *bus, int devfn)
     return bus->slot_reserved_mask & (1UL << PCI_SLOT(devfn));
 }
 
+static bool pci_bus_devfn_reserved_manual(PCIBus *bus, int devfn)
+{
+    return bus->enforce_slot_reserved_mask_manual &&
+            (bus->slot_reserved_mask & (1UL << PCI_SLOT(devfn)));
+}
+
+void pci_bus_ignore_slot_reserved_mask_manual(PCIBus *bus)
+{
+    bus->enforce_slot_reserved_mask_manual = false;
+}
+
 uint32_t pci_bus_get_slot_reserved_mask(PCIBus *bus)
 {
     return bus->slot_reserved_mask;
@@ -1164,7 +1176,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/include/hw/pci/pci.h b/include/hw/pci/pci.h
index 935b4b91b4..48d29ec234 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -287,6 +287,7 @@  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);
+void pci_bus_ignore_slot_reserved_mask_manual(PCIBus *bus);
 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);
diff --git a/include/hw/pci/pci_bus.h b/include/hw/pci/pci_bus.h
index 5653175957..e0f15ee9be 100644
--- a/include/hw/pci/pci_bus.h
+++ b/include/hw/pci/pci_bus.h
@@ -37,6 +37,7 @@  struct PCIBus {
     void *iommu_opaque;
     uint8_t devfn_min;
     uint32_t slot_reserved_mask;
+    bool enforce_slot_reserved_mask_manual;
     pci_set_irq_fn set_irq;
     pci_map_irq_fn map_irq;
     pci_route_irq_fn route_intx_to_irq;