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

["Michael S. Tsirkin" <mst@redhat.com>]

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/20230106064838-mutt-send-email-mst@kernel.org/
> > > Signed-off-by: Chuck Zmudzinski <brchuckz@aol.com>
> >
> > 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
> >