[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 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).
ATB,
Mark.
|
