|
[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 manual slot assignment
On 3/14/2023 10:39 AM, Mark Cave-Ayland wrote:
> On 14/03/2023 14:21, Chuck Zmudzinski wrote:
>
> > 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@xxxxxxxxxx/
> >>>>>>> Signed-off-by: Chuck Zmudzinski <brchuckz@xxxxxxx>
> >>>>>>
> >>>>>> 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?
> >
> > My immediate thought as a way to avoid needing to set both masks in the
> > sun4u machine would be to add an initialization function that defines the
> > relationship between the two masks. The sun4u machine would use a default
> > function that simply sets the manual mask equal to the auto mask. In the
> > special case of xen, the manual mask would be set to 0.
>
> I must admit I wasn't too concerned about having two separate masks, more
> that in the
> common case you would have to remember to call two functions. I was thinking
> that
> instead of having separate pci_bus_set_slot_reserved_mask_auto() and
> pci_bus_set_slot_reserved_mask_manual() accessors, you could just have:
>
> pci_bus_set_slot_reserved_mask(uint32_t auto_mask, uint32_t manual_mask);
>
> since that gives a single function call for all cases, and it seems fairly
> intuitive
> what pci_bus_set_slot_reserved_mask(foo, foo) is doing vs.
> pci_bus_set_slot_reserved_mask(foo, bar).
That looks good to me. I can implement it with the value for each mask
passed as separate arguments of a single function.
I think ultimately, both the sun4u machine and the xenfv machine with
the Intel igd are unique cases, and I don't think one is necessarily more
common that the other. In fact, I agree with Michael that the requirements
of the xenfv machine with Intel igd are different enough from the requirements
of the sun4u machine that it makes sense to use two independent mechanisms
to accommodate each case. For example, in the case of the Intel igd, slot
reservation is not to prevent all pci devices from using the slot but only to
reserve the slot for a particular device: the Intel igd. So, from the point of
view of what is needed for the Intel igd, slot_reserved_mask for
the case of the sun4u machine really means slot_unavailable_mask, not
slot_reserved_mask. Ideally, the kind of mask that the Intel igd needs is
one that releases for the slot for use when the particular device that
is requesting the slot is the actual device the slot is reserved for, which
is more complicated to implement than the single 32-bit mask that works
well for the sun4u machine. Specifically, it needs more information than just
the devfn value to decide whether or not to release the slot for use.
I think that in the long term, it might be best to try to collect all the unique
quirks of the Intel igd into a special pci-host device that both kvm (with
legacy
passthrough mode) and xen can use, but that could not be accomplished in
time for the 8.0 release.
I will have v3 ready later today or tomorrow.
Kind regards,
Chuck
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |