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

[Chuck Zmudzinski <brchuckz@xxxxxxx>]

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/20230106064838-mutt-send-email-mst@xxxxxxxxxx/
> > Signed-off-by: Chuck Zmudzinski <brchuckz@xxxxxxx>
>
> I really dislike this. 
> It seems that xen should not have used slot_reserved_mask,
> and instead needs something new like slot_manual_mask.
> No?

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
>