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

[Michael S. Tsirkin]

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?

> ---
> 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