[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 2/2] pci: allow slot_reserved_mask to be ignored with manu
From: |
Michael S. Tsirkin |
Subject: |
Re: [PATCH v2 2/2] pci: allow slot_reserved_mask to be ignored with manual slot assignment |
Date: |
Tue, 14 Mar 2023 09:16:36 -0400 |
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
> >
- [PATCH v2 0/2] pci: slot_reserved_mask improvements, Chuck Zmudzinski, 2023/03/14
- [PATCH v2 2/2] pci: allow slot_reserved_mask to be ignored with manual slot assignment, Chuck Zmudzinski, 2023/03/14
- Re: [PATCH v2 2/2] pci: allow slot_reserved_mask to be ignored with manual slot assignment, Michael S. Tsirkin, 2023/03/14
- Re: [PATCH v2 2/2] pci: allow slot_reserved_mask to be ignored with manual slot assignment, Mark Cave-Ayland, 2023/03/14
- Re: [PATCH v2 2/2] pci: allow slot_reserved_mask to be ignored with manual slot assignment, Michael S. Tsirkin, 2023/03/14
- Re: [PATCH v2 2/2] pci: allow slot_reserved_mask to be ignored with manual slot assignment, Chuck Zmudzinski, 2023/03/14
- Re: [PATCH v2 2/2] pci: allow slot_reserved_mask to be ignored with manual slot assignment, Mark Cave-Ayland, 2023/03/14
- Re: [PATCH v2 2/2] pci: allow slot_reserved_mask to be ignored with manual slot assignment, Chuck Zmudzinski, 2023/03/14
- Re: [PATCH v2 2/2] pci: allow slot_reserved_mask to be ignored with manual slot assignment, Chuck Zmudzinski, 2023/03/14
- Re: [PATCH v2 2/2] pci: allow slot_reserved_mask to be ignored with manual slot assignment, Mark Cave-Ayland, 2023/03/14
- Re: [PATCH v2 2/2] pci: allow slot_reserved_mask to be ignored with manual slot assignment, Chuck Zmudzinski, 2023/03/14
[PATCH v2 1/2] pci: avoid accessing slot_reserved_mask directly outside of pci.c, Chuck Zmudzinski, 2023/03/14