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

[Chuck Zmudzinski]

On 3/14/2023 9:41 AM, Mark Cave-Ayland wrote:
> On 14/03/2023 13:26, Chuck Zmudzinski wrote:
>
> > On 3/14/2023 9:17 AM, Michael S. Tsirkin wrote:
> >> On Tue, Mar 14, 2023 at 12:43:12PM +0000, Mark Cave-Ayland wrote:
> >>> On 14/03/2023 06:33, 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?
> >>>
> >>> My suggestion was to move the validation logic to a separate callback
> >>> function in PCIBus (see
> >>> https://lists.gnu.org/archive/html/qemu-devel/2023-03/msg03988.html) but
> >>> perhaps I wasn't clear enough in pointing out that I was thinking this 
> >>> could
> >>> *replace* the existing slot_reserved_mask mechanism, rather than 
> >>> providing a
> >>> hook to allow it to be manipulated.
> >>>
> >>> Here's a very rough patch put together over lunch that attempts this for
> >>> pci_bus_devfn_reserved(): the idea is that sun4u and Xen would call
> >>> pci_bus_set_slot_reserved_fn() with a suitable pci_slot_reserved_fn
> >>> implementation, and slot_reserved_mask gets removed completely i.e.:
> >>>
> >>>
> >>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> >>> index def5000e7b..30b856499a 100644
> >>> --- a/hw/pci/pci.c
> >>> +++ b/hw/pci/pci.c
> >>> @@ -493,6 +493,13 @@ bool pci_bus_bypass_iommu(PCIBus *bus)
> >>>       return host_bridge->bypass_iommu;
> >>>   }
> >>>
> >>> +static bool pci_bus_default_slot_reserved(PCISlotReservationType restype,
> >>> +                                          int devfn)
> >>> +{
> >>> +    /* All slots accessible by default */
> >>> +    return false;
> >>> +}
> >>> +
> >>>   static void pci_root_bus_internal_init(PCIBus *bus, DeviceState *parent,
> >>>                                          MemoryRegion *address_space_mem,
> >>>                                          MemoryRegion *address_space_io,
> >>> @@ -500,7 +507,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->slot_reserved_fn = pci_bus_default_slot_reserved;
> >>>       bus->address_space_mem = address_space_mem;
> >>>       bus->address_space_io = address_space_io;
> >>>       bus->flags |= PCI_BUS_IS_ROOT;
> >>> @@ -1111,9 +1118,15 @@ static bool pci_bus_devfn_available(PCIBus *bus, 
> >>> int devfn)
> >>>       return !(bus->devices[devfn]);
> >>>   }
> >>>
> >>> -static bool pci_bus_devfn_reserved(PCIBus *bus, int devfn)
> >>> +static bool pci_bus_devfn_reserved(PCIBus *bus, int devfn,
> >>> +                                   PCISlotReservationType restype)
> >>> +{
> >>> +    return bus->slot_reserved_fn(restype, devfn);
> >>> +}
> >>> +
> >>> +void pci_bus_set_slot_reserved_fn(PCIBus *bus, pci_slot_reserved_fn fn)
> >>>   {
> >>> -    return bus->slot_reserved_mask & (1UL << PCI_SLOT(devfn));
> >>> +    bus->slot_reserved_fn = fn;
> >>>   }
> >>>
> >>>   /* -1 for devfn means auto assign */
> >>> @@ -1141,7 +1154,7 @@ static PCIDevice *do_pci_register_device(PCIDevice 
> >>> *pci_dev,
> >>>           for(devfn = bus->devfn_min ; devfn < ARRAY_SIZE(bus->devices);
> >>>               devfn += PCI_FUNC_MAX) {
> >>>               if (pci_bus_devfn_available(bus, devfn) &&
> >>> -                   !pci_bus_devfn_reserved(bus, devfn)) {
> >>> +                   !pci_bus_devfn_reserved(bus, devfn, 
> >>> PCI_SLOT_RESERVATION_AUTO)) {
> >>>                   goto found;
> >>>               }
> >>>           }
> >>> @@ -1149,7 +1162,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(bus, devfn, 
> >>> PCI_SLOT_RESERVATION_MANUAL)) {
> >>>           error_setg(errp, "PCI: slot %d function %d not available for 
> >>> %s,"
> >>>                                          MemoryRegion *address_space_io,
> >>> @@ -500,7 +507,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->slot_reserved_fn = pci_bus_default_slot_reserved;
> >>>       bus->address_space_mem = address_space_mem;
> >>>       bus->address_space_io = address_space_io;
> >>>       bus->flags |= PCI_BUS_IS_ROOT;
> >>> @@ -1111,9 +1118,15 @@ static bool pci_bus_devfn_available(PCIBus *bus, 
> >>> int devfn)
> >>>       return !(bus->devices[devfn]);
> >>>   }
> >>>
> >>> -static bool pci_bus_devfn_reserved(PCIBus *bus, int devfn)
> >>> +static bool pci_bus_devfn_reserved(PCIBus *bus, int devfn,
> >>> +                                   PCISlotReservationType restype)
> >>> +{
> >>> +    return bus->slot_reserved_fn(restype, devfn);
> >>> +}
> >>> +
> >>> +void pci_bus_set_slot_reserved_fn(PCIBus *bus, pci_slot_reserved_fn fn)
> >>>   {
> >>> -    return bus->slot_reserved_mask & (1UL << PCI_SLOT(devfn));
> >>> +    bus->slot_reserved_fn = fn;
> >>>   }
> >>>
> >>>   /* -1 for devfn means auto assign */
> >>> @@ -1141,7 +1154,7 @@ static PCIDevice *do_pci_register_device(PCIDevice 
> >>> *pci_dev,
> >>>           for(devfn = bus->devfn_min ; devfn < ARRAY_SIZE(bus->devices);
> >>>               devfn += PCI_FUNC_MAX) {
> >>>               if (pci_bus_devfn_available(bus, devfn) &&
> >>> -                   !pci_bus_devfn_reserved(bus, devfn)) {
> >>> +                   !pci_bus_devfn_reserved(bus, devfn, 
> >>> PCI_SLOT_RESERVATION_AUTO)) {
> >>>                   goto found;
> >>>               }
> >>>           }
> >>> @@ -1149,7 +1162,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(bus, devfn, 
> >>> PCI_SLOT_RESERVATION_MANUAL)) {
> >>>           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 d5a40cd058..8a949f7ae1 100644
> >>> --- a/include/hw/pci/pci.h
> >>> +++ b/include/hw/pci/pci.h
> >>> @@ -257,10 +257,18 @@ MemoryRegion *pci_address_space_io(PCIDevice *dev);
> >>>    */
> >>>   int pci_bar(PCIDevice *d, int reg);
> >>>
> >>> +typedef enum PCISlotReservationType {
> >>> +    PCI_SLOT_RESERVATION_AUTO,
> >>> +    PCI_SLOT_RESERVATION_MANUAL
> >>> +} PCISlotReservationType;
> >>> +
> >>> +typedef bool (*pci_slot_reserved_fn)(PCISlotReservationType restype, int 
> >>> devfn);
> >>>   typedef void (*pci_set_irq_fn)(void *opaque, int irq_num, int level);
> >>>   typedef int (*pci_map_irq_fn)(PCIDevice *pci_dev, int irq_num);
> >>>   typedef PCIINTxRoute (*pci_route_irq_fn)(void *opaque, int pin);
> >>>
> >>> +void pci_bus_set_slot_reserved_fn(PCIBus *bus, pci_slot_reserved_fn fn);
> >>> +
> >>>   #define TYPE_PCI_BUS "PCI"
> >>>   OBJECT_DECLARE_TYPE(PCIBus, PCIBusClass, PCI_BUS)
> >>>   #define TYPE_PCIE_BUS "PCIE"
> >>> diff --git a/include/hw/pci/pci_bus.h b/include/hw/pci/pci_bus.h
> >>> index 5653175957..d68ea1418d 100644
> >>> --- a/include/hw/pci/pci_bus.h
> >>> +++ b/include/hw/pci/pci_bus.h
> >>> @@ -36,7 +36,7 @@ struct PCIBus {
> >>>       PCIIOMMUFunc iommu_fn;
> >>>       void *iommu_opaque;
> >>>       uint8_t devfn_min;
> >>> -    uint32_t slot_reserved_mask;
> >>> +    pci_slot_reserved_fn slot_reserved_fn;
> >>>       pci_set_irq_fn set_irq;
> >>>       pci_map_irq_fn map_irq;
> >>>       pci_route_irq_fn route_intx_to_irq;
> >>>
> >>>
> >>> If this approach seems reasonable, I'm happy for someone else to take this
> >>> over and turn it into a proper series.
> >>>
> >>>
> >>> ATB,
> >>>
> >>> Mark.
> >>
> >> It's ok too though I think I like chuck's proposal better:
> >> less callbacks to chase.
> >>
> > 
> > I would be willing to pursue this if there were more use cases for
> > slot_reserved_mask than just the two cases we have now: xen and sun4u.
> > Until there is a clear demand for a more general way to manipulate the
> > mask, I agree with Michael that the KISS principle should apply here.
>
> No worries. The thinking behind this idea was that it feel like the Xen case 
> is 
> special in that it has separate requirements for auto slot allocation and 
> manual slot 
> allocation: if slot reservation were used in more places, I'd expect the 
> sun4u case 
> to be more common, in which case it seems a bit more work for the common case 
> to have 
> to set both slot_reserved_mask_auto and slot_reserved_mask_manual separately. 
> Perhaps 
> a single accessor function can be used to set both mask values together for a 
> PCIBus?
>
> Regardless, I'll take step back and leave you and Michael to come up with a 
> solution 
> that you're both happy with. Let me know if you need me to test anything on 
> sun4u.

I will give this a little time to see if anyone else has any ideas here,
and to also more carefully consider Mark's proposal, before posting
v3.

Kind regards,

Chuck