Re: [PATCH] pci: add enforce_slot_reserved_mask_manual property

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

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 <brchuckz@aol.com>

Thanks!
I'm trying to think of the best default for this.
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?

> 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