On 1/27/2023 8:28 AM, Michael S. Tsirkin wrote:
> On Sun, Jan 15, 2023 at 07:49:51PM -0500, Chuck Zmudzinski wrote:
> > The current reserved slot check in do_pci_register_device(), added with
> > commit 8b8849844fd6
>
> add ("subject here") please
>
> > ,is done even if the pci device being added is
> > configured manually for a particular slot. The new property, when set
> > to false, disables the check when the device is configured to request a
> > particular slot. This allows an administrator or management tool to
> > override slot_reserved_mask for a pci device by requesting a particular
> > slot for the device. The new property is initialized to true which
> > preserves the existing behavior of slot_reserved_mask by default.
> >
> > Signed-off-by: Chuck Zmudzinski <brchu...@aol.com>
>
> Thanks!
> I'm trying to think of the best default for this.
I think it would be better for the default value of
enforce_slot_reserved_mask_manual to be false, so that a
user-specified slot will by default override slot_reserved_mask.
But doing that would change the current behavior of
slot_reserved_mask.
Currently, this is the only place where slot_reserved_mask is used in all
of the Qemu source (code from hw/sparc64/sun4u.c):
------ snip -------
/* Only in-built Simba APBs can exist on the root bus, slot 0 on busA is
reserved (leaving no slots free after on-board devices) however slots
0-3 are free on busB */
pci_bus->slot_reserved_mask = 0xfffffffc;
pci_busA->slot_reserved_mask = 0xfffffff1;
pci_busB->slot_reserved_mask = 0xfffffff0;
------ snip -------
I think we could safely change the default value of
enforce_slot_reserved_mask_manual to false but set
it to true for the sparc64 sun4u board here to preserve
the current behavior of the only existing board in Qemu
that uses slot_reserved_mask.
What do you think?
> Users is trying to configure a specific device on a reserved
> slot. Should we
> CC a bunch more people for visibility. Input, anyone?
>
>
> > ---
> > hw/pci/pci.c | 9 ++++++++-
> > include/hw/pci/pci_bus.h | 1 +
> > 2 files changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > index c2fb88f9a3..5e15f08036 100644
> > --- a/hw/pci/pci.c
> > +++ b/hw/pci/pci.c
> > @@ -467,6 +467,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;
> > @@ -1074,6 +1075,12 @@ 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)));
> > +}
> > +
> > /* -1 for devfn means auto assign */
> > static PCIDevice *do_pci_register_device(PCIDevice *pci_dev,
> > const char *name, int devfn,
> > @@ -1107,7 +1114,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);
>
> Should this be a device property or a bus property?
> And maybe now mention the property name so users know how
> to override this?
If we implement the change to make the default value of
enforce_slot_reserved_mask_manual false, the only case
when the user will not be able to override it simply by specifying
a particular slot would be when it is a user of the sparc64 sun4u
board who wants to put a device in a slot that board does not
allow. In that case, it would be nice to report an error here that
explains why the slot is reserved even with a manual configuration
of the slot. I could add another property to the bus which will allow
for providing a reason the slot is reserved that can be used in the
error message instead of the current generic message that says the
slot is reserved without explaining why.
I will also think about overriding slot_reserved_mask using
a device property instead of a bus property, but I think keeping
it as a bus property, making it false by default, and adding a
reason for having reserved slots even with manually configured
slots (as in the case of the sparc64 sun4u board) in the form of
an error message string would improve the error message here
without changing the current behavior of the sparc64 sun4u board
and make the slot_reserved_mask useful for the case when it is
necessary to reserve a slot for a particular device when device slots
are auto-assigned but it is not necessary to reserve the slot when
the slot addresses are manually assigned. Since there is only the one
case in Qemu (the sparc64 sun4u board) where a specific error message
would be needed to be added, it is not too much work to implement this
change.
Does that make sense?
> > 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.0
>
