hw/pci/pci.c | 9 ++++++++- include/hw/pci/pci_bus.h | 1 + 2 files changed, 9 insertions(+), 1 deletion(-)
The current reserved slot check in do_pci_register_device(), added with
commit 8b8849844fd6, 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>
---
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);
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
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
On 1/27/2023 8:28 AM, Michael S. Tsirkin wrote: > 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. I think it would be better for the default value of enforce_slot_reserved_mask_manual to be false, so that a user-specified slot will by default override slot_reserved_mask. But doing that would change the current behavior of slot_reserved_mask. Currently, this is the only place where slot_reserved_mask is used in all of the Qemu source (code from hw/sparc64/sun4u.c): ------ snip ------- /* Only in-built Simba APBs can exist on the root bus, slot 0 on busA is reserved (leaving no slots free after on-board devices) however slots 0-3 are free on busB */ pci_bus->slot_reserved_mask = 0xfffffffc; pci_busA->slot_reserved_mask = 0xfffffff1; pci_busB->slot_reserved_mask = 0xfffffff0; ------ snip ------- I think we could safely change the default value of enforce_slot_reserved_mask_manual to false but set it to true for the sparc64 sun4u board here to preserve the current behavior of the only existing board in Qemu that uses slot_reserved_mask. What do you think? > 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? If we implement the change to make the default value of enforce_slot_reserved_mask_manual false, the only case when the user will not be able to override it simply by specifying a particular slot would be when it is a user of the sparc64 sun4u board who wants to put a device in a slot that board does not allow. In that case, it would be nice to report an error here that explains why the slot is reserved even with a manual configuration of the slot. I could add another property to the bus which will allow for providing a reason the slot is reserved that can be used in the error message instead of the current generic message that says the slot is reserved without explaining why. I will also think about overriding slot_reserved_mask using a device property instead of a bus property, but I think keeping it as a bus property, making it false by default, and adding a reason for having reserved slots even with manually configured slots (as in the case of the sparc64 sun4u board) in the form of an error message string would improve the error message here without changing the current behavior of the sparc64 sun4u board and make the slot_reserved_mask useful for the case when it is necessary to reserve a slot for a particular device when device slots are auto-assigned but it is not necessary to reserve the slot when the slot addresses are manually assigned. Since there is only the one case in Qemu (the sparc64 sun4u board) where a specific error message would be needed to be added, it is not too much work to implement this change. Does that make sense? > > 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 >
On 28/01/2023 03:39, Chuck Zmudzinski wrote: > On 1/27/2023 8:28 AM, Michael S. Tsirkin wrote: >> 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. > > I think it would be better for the default value of > enforce_slot_reserved_mask_manual to be false, so that a > user-specified slot will by default override slot_reserved_mask. > But doing that would change the current behavior of > slot_reserved_mask. > > Currently, this is the only place where slot_reserved_mask is used in all > of the Qemu source (code from hw/sparc64/sun4u.c): > > ------ snip ------- > /* Only in-built Simba APBs can exist on the root bus, slot 0 on busA is > reserved (leaving no slots free after on-board devices) however slots > 0-3 are free on busB */ > pci_bus->slot_reserved_mask = 0xfffffffc; > pci_busA->slot_reserved_mask = 0xfffffff1; > pci_busB->slot_reserved_mask = 0xfffffff0; > ------ snip ------- > > I think we could safely change the default value of > enforce_slot_reserved_mask_manual to false but set > it to true for the sparc64 sun4u board here to preserve > the current behavior of the only existing board in Qemu > that uses slot_reserved_mask. > > What do you think? > >> Users is trying to configure a specific device on a reserved >> slot. Should we >> CC a bunch more people for visibility. Input, anyone? For a bit of background, slot_reserved_mask was added by me to solve a problem with the sun4u machine: on a real Ultra-5, the pci "A" bus has 2 free slots and the pci "B" bus has 4 free slots. Whilst it is possible to plug a PCI device into any slot in QEMU, the PCI bridges only have IRQ mapping registers for those 6 slots, so you can easily end up with an auto-allocated slot where it is impossible for the OS to map the IRQ. Hence slot_reserved_mask was originally intended to mark slots as being unavailable for both manual and automatic allocation to ensure that devices plugged into both PCI buses would always work. If there is a need to change/refactor the logic then I can test the sun4u machine to ensure the original test case still works. ATB, Mark.
On 1/28/2023 4:58 PM, Mark Cave-Ayland wrote: > On 28/01/2023 03:39, Chuck Zmudzinski wrote: > > > On 1/27/2023 8:28 AM, Michael S. Tsirkin wrote: > >> 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. Hi Michael and Mark, The Xen maintainers pulled up the commit that reserves slot 2 for the Intel IGD with the xenfv machine so we need to discuss how slot_reserved_mask should work with this change. The original version of the patch keeps the current default of always enforcing slot_reserved_mask, even if the administrator tries to configure the slots manually. So if we keep that behavior, we will need to patch the xenfv machine again to implement the desired behavior that slot_reserved_mask will only apply with automatic slot assignment for the xenfv machine. That would be a fairly trivial one-line patch to add on top of the patch the Xen maintainers just pulled up. Another option is to change the default value of enforce_slot_reserved_mask_manual to false so slot_reserved_mask only affects automatic slot address assignment by default and then preserve the current behavior for the sun4u machine by patching the sun4u machine so the value of enforce_slot_reserved_mask_manual is true for the sun4u machine. > > > > I think it would be better for the default value of > > enforce_slot_reserved_mask_manual to be false, so that a > > user-specified slot will by default override slot_reserved_mask. > > But doing that would change the current behavior of > > slot_reserved_mask. > > > > Currently, this is the only place where slot_reserved_mask is used in all > > of the Qemu source (code from hw/sparc64/sun4u.c): > > > > ------ snip ------- > > /* Only in-built Simba APBs can exist on the root bus, slot 0 on busA is > > reserved (leaving no slots free after on-board devices) however slots > > 0-3 are free on busB */ > > pci_bus->slot_reserved_mask = 0xfffffffc; > > pci_busA->slot_reserved_mask = 0xfffffff1; > > pci_busB->slot_reserved_mask = 0xfffffff0; > > ------ snip ------- > > > > I think we could safely change the default value of > > enforce_slot_reserved_mask_manual to false but set > > it to true for the sparc64 sun4u board here to preserve > > the current behavior of the only existing board in Qemu > > that uses slot_reserved_mask. > > > > What do you think? > > > >> Users is trying to configure a specific device on a reserved > >> slot. Should we > >> CC a bunch more people for visibility. Input, anyone? > > For a bit of background, slot_reserved_mask was added by me to solve a problem with > the sun4u machine: on a real Ultra-5, the pci "A" bus has 2 free slots and the pci > "B" bus has 4 free slots. Whilst it is possible to plug a PCI device into any slot in > QEMU, the PCI bridges only have IRQ mapping registers for those 6 slots, so you can > easily end up with an auto-allocated slot where it is impossible for the OS to map > the IRQ. > > Hence slot_reserved_mask was originally intended to mark slots as being unavailable > for both manual and automatic allocation to ensure that devices plugged into both PCI > buses would always work. Here is a third option: Mark, would it be acceptable to replace the error in the case of manual allocation with a warning, so manual device assignment to a reserved slot in the sun4u machine would succeed (automatic assignment would still prevent the slot from being used) but there would be a warning message to provide the administrator/user with a clue that the current manual configuration of the device is not correct and could cause problems? That would be a less aggressive change that could be implemented without needing to add the enforce_slot_reserved_mask_manual property to PCIBus. Also, Michael, I notice that the current usage of slot_reserved_mask violates the comment at the beginning of pci_bus.h that asks that the properties of PCIBus such as slot_reserved_mask be accessed via accessor functions in pci.h instead of directly accessing them. Should I also write v2 of the patch to add accessor functions for slot_reserved_mask so the accessor functions can be used instead of accessing slot_reserved_mask directly? I will wait a little while for some feedback before posting v2 of this patch. > If there is a need to change/refactor the logic then I can test the sun4u machine to > ensure the original test case still works. Sounds good... Best regards, Chuck > ATB, > > Mark.
On 06/03/2023 16:37, Chuck Zmudzinski wrote: > On 1/28/2023 4:58 PM, Mark Cave-Ayland wrote: >> On 28/01/2023 03:39, Chuck Zmudzinski wrote: >> >>> On 1/27/2023 8:28 AM, Michael S. Tsirkin wrote: >>>> 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. > > Hi Michael and Mark, > > The Xen maintainers pulled up the commit that reserves slot 2 > for the Intel IGD with the xenfv machine so we need to discuss > how slot_reserved_mask should work with this change. The original > version of the patch keeps the current default of always enforcing > slot_reserved_mask, even if the administrator tries to configure the > slots manually. So if we keep that behavior, we will need to patch > the xenfv machine again to implement the desired behavior that > slot_reserved_mask will only apply with automatic slot assignment > for the xenfv machine. That would be a fairly trivial one-line patch > to add on top of the patch the Xen maintainers just pulled up. > > Another option is to change the default value of > enforce_slot_reserved_mask_manual to false so slot_reserved_mask > only affects automatic slot address assignment by default and then > preserve the current behavior for the sun4u machine by patching the > sun4u machine so the value of enforce_slot_reserved_mask_manual > is true for the sun4u machine. > >>> >>> I think it would be better for the default value of >>> enforce_slot_reserved_mask_manual to be false, so that a >>> user-specified slot will by default override slot_reserved_mask. >>> But doing that would change the current behavior of >>> slot_reserved_mask. >>> >>> Currently, this is the only place where slot_reserved_mask is used in all >>> of the Qemu source (code from hw/sparc64/sun4u.c): >>> >>> ------ snip ------- >>> /* Only in-built Simba APBs can exist on the root bus, slot 0 on busA is >>> reserved (leaving no slots free after on-board devices) however slots >>> 0-3 are free on busB */ >>> pci_bus->slot_reserved_mask = 0xfffffffc; >>> pci_busA->slot_reserved_mask = 0xfffffff1; >>> pci_busB->slot_reserved_mask = 0xfffffff0; >>> ------ snip ------- >>> >>> I think we could safely change the default value of >>> enforce_slot_reserved_mask_manual to false but set >>> it to true for the sparc64 sun4u board here to preserve >>> the current behavior of the only existing board in Qemu >>> that uses slot_reserved_mask. >>> >>> What do you think? >>> >>>> Users is trying to configure a specific device on a reserved >>>> slot. Should we >>>> CC a bunch more people for visibility. Input, anyone? >> >> For a bit of background, slot_reserved_mask was added by me to solve a problem with >> the sun4u machine: on a real Ultra-5, the pci "A" bus has 2 free slots and the pci >> "B" bus has 4 free slots. Whilst it is possible to plug a PCI device into any slot in >> QEMU, the PCI bridges only have IRQ mapping registers for those 6 slots, so you can >> easily end up with an auto-allocated slot where it is impossible for the OS to map >> the IRQ. >> >> Hence slot_reserved_mask was originally intended to mark slots as being unavailable >> for both manual and automatic allocation to ensure that devices plugged into both PCI >> buses would always work. > > Here is a third option: > > Mark, would it be acceptable to replace the error in the case of manual > allocation with a warning, so manual device assignment to a reserved slot > in the sun4u machine would succeed (automatic assignment would still > prevent the slot from being used) but there would be a warning message > to provide the administrator/user with a clue that the current manual > configuration of the device is not correct and could cause problems? > > That would be a less aggressive change that could be implemented without > needing to add the enforce_slot_reserved_mask_manual property to PCIBus. Unfortunately in the sun4u case that doesn't quite work, since the PCI host bridge doesn't have IRQ routing registers for the reserved slots and so the card would still fail with a manual allocation. In effect the reserved bit in its current implementation means "this slot is unavailable" which is the intended result for the original implementation. Certainly this logic would reduce the changes of creating an unusable setup from the command line, but then some management tools which manually allocates slots would still allow an unusable configuration. > Also, Michael, I notice that the current usage of slot_reserved_mask violates > the comment at the beginning of pci_bus.h that asks that the properties of > PCIBus such as slot_reserved_mask be accessed via accessor functions in pci.h > instead of directly accessing them. Should I also write v2 of the patch to add > accessor functions for slot_reserved_mask so the accessor functions can be used > instead of accessing slot_reserved_mask directly? > > I will wait a little while for some feedback before posting v2 of this patch. Another possibility could be to move the slot validation logic in do_pci_register_device() to a separate function, and add a new slot validation callback to PCIBusClass to be used by do_pci_register_device() instead. By default this would call the existing slot validation logic function. It would then be possible to override the default slot validation function in the Xen case, perhaps even calling the existing logic first before adding your additional logic requirement on top. ATB, Mark.
On 3/12/2023 10:13 AM, Mark Cave-Ayland wrote: > On 06/03/2023 16:37, Chuck Zmudzinski wrote: > > > On 1/28/2023 4:58 PM, Mark Cave-Ayland wrote: > >> On 28/01/2023 03:39, Chuck Zmudzinski wrote: > >> > >>> On 1/27/2023 8:28 AM, Michael S. Tsirkin wrote: > >>>> 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. > > > > Hi Michael and Mark, > > > > The Xen maintainers pulled up the commit that reserves slot 2 > > for the Intel IGD with the xenfv machine so we need to discuss > > how slot_reserved_mask should work with this change. The original > > version of the patch keeps the current default of always enforcing > > slot_reserved_mask, even if the administrator tries to configure the > > slots manually. So if we keep that behavior, we will need to patch > > the xenfv machine again to implement the desired behavior that > > slot_reserved_mask will only apply with automatic slot assignment > > for the xenfv machine. That would be a fairly trivial one-line patch > > to add on top of the patch the Xen maintainers just pulled up. > > > > Another option is to change the default value of > > enforce_slot_reserved_mask_manual to false so slot_reserved_mask > > only affects automatic slot address assignment by default and then > > preserve the current behavior for the sun4u machine by patching the > > sun4u machine so the value of enforce_slot_reserved_mask_manual > > is true for the sun4u machine. > > > >>> > >>> I think it would be better for the default value of > >>> enforce_slot_reserved_mask_manual to be false, so that a > >>> user-specified slot will by default override slot_reserved_mask. > >>> But doing that would change the current behavior of > >>> slot_reserved_mask. > >>> > >>> Currently, this is the only place where slot_reserved_mask is used in all > >>> of the Qemu source (code from hw/sparc64/sun4u.c): > >>> > >>> ------ snip ------- > >>> /* Only in-built Simba APBs can exist on the root bus, slot 0 on busA is > >>> reserved (leaving no slots free after on-board devices) however slots > >>> 0-3 are free on busB */ > >>> pci_bus->slot_reserved_mask = 0xfffffffc; > >>> pci_busA->slot_reserved_mask = 0xfffffff1; > >>> pci_busB->slot_reserved_mask = 0xfffffff0; > >>> ------ snip ------- > >>> > >>> I think we could safely change the default value of > >>> enforce_slot_reserved_mask_manual to false but set > >>> it to true for the sparc64 sun4u board here to preserve > >>> the current behavior of the only existing board in Qemu > >>> that uses slot_reserved_mask. > >>> > >>> What do you think? > >>> > >>>> Users is trying to configure a specific device on a reserved > >>>> slot. Should we > >>>> CC a bunch more people for visibility. Input, anyone? > >> > >> For a bit of background, slot_reserved_mask was added by me to solve a problem with > >> the sun4u machine: on a real Ultra-5, the pci "A" bus has 2 free slots and the pci > >> "B" bus has 4 free slots. Whilst it is possible to plug a PCI device into any slot in > >> QEMU, the PCI bridges only have IRQ mapping registers for those 6 slots, so you can > >> easily end up with an auto-allocated slot where it is impossible for the OS to map > >> the IRQ. > >> > >> Hence slot_reserved_mask was originally intended to mark slots as being unavailable > >> for both manual and automatic allocation to ensure that devices plugged into both PCI > >> buses would always work. > > > > Here is a third option: > > > > Mark, would it be acceptable to replace the error in the case of manual > > allocation with a warning, so manual device assignment to a reserved slot > > in the sun4u machine would succeed (automatic assignment would still > > prevent the slot from being used) but there would be a warning message > > to provide the administrator/user with a clue that the current manual > > configuration of the device is not correct and could cause problems? > > > > That would be a less aggressive change that could be implemented without > > needing to add the enforce_slot_reserved_mask_manual property to PCIBus. > > Unfortunately in the sun4u case that doesn't quite work, since the PCI host bridge > doesn't have IRQ routing registers for the reserved slots and so the card would still > fail with a manual allocation. > > In effect the reserved bit in its current implementation means "this slot is > unavailable" which is the intended result for the original implementation. Certainly > this logic would reduce the changes of creating an unusable setup from the command > line, but then some management tools which manually allocates slots would still allow > an unusable configuration. OK, I will not change the current behavior for the sun4u machine. > > > Also, Michael, I notice that the current usage of slot_reserved_mask violates > > the comment at the beginning of pci_bus.h that asks that the properties of > > PCIBus such as slot_reserved_mask be accessed via accessor functions in pci.h > > instead of directly accessing them. Should I also write v2 of the patch to add > > accessor functions for slot_reserved_mask so the accessor functions can be used > > instead of accessing slot_reserved_mask directly? > > > > I will wait a little while for some feedback before posting v2 of this patch. > > Another possibility could be to move the slot validation logic in > do_pci_register_device() to a separate function, and add a new slot validation > callback to PCIBusClass to be used by do_pci_register_device() instead. By default > this would call the existing slot validation logic function. > > It would then be possible to override the default slot validation function in the Xen > case, perhaps even calling the existing logic first before adding your additional > logic requirement on top. I agree we can keep the default to be the logic that the sun4u machine currently uses and override it to achieve the desired behavior of the xenfv machine. Kind regards, Chuck > > > ATB, > > Mark.
On 1/28/2023 4:58 PM, Mark Cave-Ayland wrote: > On 28/01/2023 03:39, Chuck Zmudzinski wrote: > > > On 1/27/2023 8:28 AM, Michael S. Tsirkin wrote: > >> 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. > > > > I think it would be better for the default value of > > enforce_slot_reserved_mask_manual to be false, so that a > > user-specified slot will by default override slot_reserved_mask. > > But doing that would change the current behavior of > > slot_reserved_mask. > > > > Currently, this is the only place where slot_reserved_mask is used in all > > of the Qemu source (code from hw/sparc64/sun4u.c): > > > > ------ snip ------- > > /* Only in-built Simba APBs can exist on the root bus, slot 0 on busA is > > reserved (leaving no slots free after on-board devices) however slots > > 0-3 are free on busB */ > > pci_bus->slot_reserved_mask = 0xfffffffc; > > pci_busA->slot_reserved_mask = 0xfffffff1; > > pci_busB->slot_reserved_mask = 0xfffffff0; > > ------ snip ------- > > > > I think we could safely change the default value of > > enforce_slot_reserved_mask_manual to false but set > > it to true for the sparc64 sun4u board here to preserve > > the current behavior of the only existing board in Qemu > > that uses slot_reserved_mask. > > > > What do you think? > > > >> Users is trying to configure a specific device on a reserved > >> slot. Should we > >> CC a bunch more people for visibility. Input, anyone? > > For a bit of background, slot_reserved_mask was added by me to solve a problem with > the sun4u machine: on a real Ultra-5, the pci "A" bus has 2 free slots and the pci > "B" bus has 4 free slots. Whilst it is possible to plug a PCI device into any slot in > QEMU, the PCI bridges only have IRQ mapping registers for those 6 slots, so you can > easily end up with an auto-allocated slot where it is impossible for the OS to map > the IRQ. > > Hence slot_reserved_mask was originally intended to mark slots as being unavailable > for both manual and automatic allocation to ensure that devices plugged into both PCI > buses would always work. > > If there is a need to change/refactor the logic then I can test the sun4u machine to > ensure the original test case still works. > > > ATB, > > Mark. Thanks, I will let you know if there is a patch to test on the sun4u machine. For now, we are waiting to see if the xen maintainers will accept a patch that uses slot_reserved_mask to prevent other devices from using the slot that is required by the Intel igd in the xenfv machine. That patch does not change the way slot_reserved_mask works, but if that patch is added some users might want to add a capability for a user to override slot_reserved_mask, and that is what this patch attempts to implement. Kind regards, Chuck
On Fri, Jan 27, 2023 at 10:39:28PM -0500, Chuck Zmudzinski wrote: > On 1/27/2023 8:28 AM, Michael S. Tsirkin wrote: > > 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. > > I think it would be better for the default value of > enforce_slot_reserved_mask_manual to be false, so that a > user-specified slot will by default override slot_reserved_mask. > But doing that would change the current behavior of > slot_reserved_mask. > > Currently, this is the only place where slot_reserved_mask is used in all > of the Qemu source (code from hw/sparc64/sun4u.c): > > ------ snip ------- > /* Only in-built Simba APBs can exist on the root bus, slot 0 on busA is > reserved (leaving no slots free after on-board devices) however slots > 0-3 are free on busB */ > pci_bus->slot_reserved_mask = 0xfffffffc; > pci_busA->slot_reserved_mask = 0xfffffff1; > pci_busB->slot_reserved_mask = 0xfffffff0; > ------ snip ------- > > I think we could safely change the default value of > enforce_slot_reserved_mask_manual to false but set > it to true for the sparc64 sun4u board here to preserve > the current behavior of the only existing board in Qemu > that uses slot_reserved_mask. > > What do you think? I guess first can you answer whether this is still needed with the latest Xen patches? -- MST
On 1/28/23 5:26 AM, Michael S. Tsirkin wrote: > On Fri, Jan 27, 2023 at 10:39:28PM -0500, Chuck Zmudzinski wrote: >> On 1/27/2023 8:28 AM, Michael S. Tsirkin wrote: >> > 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. >> >> I think it would be better for the default value of >> enforce_slot_reserved_mask_manual to be false, so that a >> user-specified slot will by default override slot_reserved_mask. >> But doing that would change the current behavior of >> slot_reserved_mask. >> >> Currently, this is the only place where slot_reserved_mask is used in all >> of the Qemu source (code from hw/sparc64/sun4u.c): >> >> ------ snip ------- >> /* Only in-built Simba APBs can exist on the root bus, slot 0 on busA is >> reserved (leaving no slots free after on-board devices) however slots >> 0-3 are free on busB */ >> pci_bus->slot_reserved_mask = 0xfffffffc; >> pci_busA->slot_reserved_mask = 0xfffffff1; >> pci_busB->slot_reserved_mask = 0xfffffff0; >> ------ snip ------- >> >> I think we could safely change the default value of >> enforce_slot_reserved_mask_manual to false but set >> it to true for the sparc64 sun4u board here to preserve >> the current behavior of the only existing board in Qemu >> that uses slot_reserved_mask. >> >> What do you think? > > I guess first can you answer whether this is still needed > with the latest Xen patches? > It's not really needed except for experimental purposes to allow an administrator to test experimental configurations with a device other than the igd at slot 2. That might be useful in some cases, but it is not really necessary unless someone asks for that capability. If libvirt users who ordinarily like to manually specify all the settings will be OK with the proposed patch to xen that prevents an administrator from being able to override a new setting that reserves slot 2 for the igd for type "xenfv" machines configured for igd passthrough, then there is no need for this patch. I don't think many users need the capability to insert a different device in slot 2 for the "xenfv" machine type configured with igd-passthru=on, so I would be OK if this patch is not included in qemu. Chuck
On Sat, Jan 28, 2023 at 08:20:55AM -0500, Chuck Zmudzinski wrote: > On 1/28/23 5:26 AM, Michael S. Tsirkin wrote: > > On Fri, Jan 27, 2023 at 10:39:28PM -0500, Chuck Zmudzinski wrote: > >> On 1/27/2023 8:28 AM, Michael S. Tsirkin wrote: > >> > 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. > >> > >> I think it would be better for the default value of > >> enforce_slot_reserved_mask_manual to be false, so that a > >> user-specified slot will by default override slot_reserved_mask. > >> But doing that would change the current behavior of > >> slot_reserved_mask. > >> > >> Currently, this is the only place where slot_reserved_mask is used in all > >> of the Qemu source (code from hw/sparc64/sun4u.c): > >> > >> ------ snip ------- > >> /* Only in-built Simba APBs can exist on the root bus, slot 0 on busA is > >> reserved (leaving no slots free after on-board devices) however slots > >> 0-3 are free on busB */ > >> pci_bus->slot_reserved_mask = 0xfffffffc; > >> pci_busA->slot_reserved_mask = 0xfffffff1; > >> pci_busB->slot_reserved_mask = 0xfffffff0; > >> ------ snip ------- > >> > >> I think we could safely change the default value of > >> enforce_slot_reserved_mask_manual to false but set > >> it to true for the sparc64 sun4u board here to preserve > >> the current behavior of the only existing board in Qemu > >> that uses slot_reserved_mask. > >> > >> What do you think? > > > > I guess first can you answer whether this is still needed > > with the latest Xen patches? > > > > It's not really needed except for experimental purposes to allow > an administrator to test experimental configurations with a device > other than the igd at slot 2. That might be useful in some cases, > but it is not really necessary unless someone asks for that capability. > If libvirt users who ordinarily like to manually specify all the > settings will be OK with the proposed patch to xen that prevents > an administrator from being able to override a new setting that > reserves slot 2 for the igd for type "xenfv" machines configured for > igd passthrough, then there is no need for this patch. I don't think > many users need the capability to insert a different device in slot 2 for > the "xenfv" machine type configured with igd-passthru=on, so I would be > OK if this patch is not included in qemu. > > Chuck Pls wait and see if that patch gets picked up. Let me know.
On 1/28/23 2:14 PM, Michael S. Tsirkin wrote: > On Sat, Jan 28, 2023 at 08:20:55AM -0500, Chuck Zmudzinski wrote: > > On 1/28/23 5:26 AM, Michael S. Tsirkin wrote: > > > On Fri, Jan 27, 2023 at 10:39:28PM -0500, Chuck Zmudzinski wrote: > > >> On 1/27/2023 8:28 AM, Michael S. Tsirkin wrote: > > >> > 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. > > >> > > >> I think it would be better for the default value of > > >> enforce_slot_reserved_mask_manual to be false, so that a > > >> user-specified slot will by default override slot_reserved_mask. > > >> But doing that would change the current behavior of > > >> slot_reserved_mask. > > >> > > >> Currently, this is the only place where slot_reserved_mask is used in all > > >> of the Qemu source (code from hw/sparc64/sun4u.c): > > >> > > >> ------ snip ------- > > >> /* Only in-built Simba APBs can exist on the root bus, slot 0 on busA is > > >> reserved (leaving no slots free after on-board devices) however slots > > >> 0-3 are free on busB */ > > >> pci_bus->slot_reserved_mask = 0xfffffffc; > > >> pci_busA->slot_reserved_mask = 0xfffffff1; > > >> pci_busB->slot_reserved_mask = 0xfffffff0; > > >> ------ snip ------- > > >> > > >> I think we could safely change the default value of > > >> enforce_slot_reserved_mask_manual to false but set > > >> it to true for the sparc64 sun4u board here to preserve > > >> the current behavior of the only existing board in Qemu > > >> that uses slot_reserved_mask. > > >> > > >> What do you think? > > > > > > I guess first can you answer whether this is still needed > > > with the latest Xen patches? > > > > > > > It's not really needed except for experimental purposes to allow > > an administrator to test experimental configurations with a device > > other than the igd at slot 2. That might be useful in some cases, > > but it is not really necessary unless someone asks for that capability. > > If libvirt users who ordinarily like to manually specify all the > > settings will be OK with the proposed patch to xen that prevents > > an administrator from being able to override a new setting that > > reserves slot 2 for the igd for type "xenfv" machines configured for > > igd passthrough, then there is no need for this patch. I don't think > > many users need the capability to insert a different device in slot 2 for > > the "xenfv" machine type configured with igd-passthru=on, so I would be > > OK if this patch is not included in qemu. > > > > Chuck > > Pls wait and see if that patch gets picked up. Let me know. > Hi Anthony, I didn't Cc you when I proposed a patch to core pci to answer Michael's complaint that the patch Qemu to reserve slot 2 for the Intel IGD unconditionally reserves slot 2 for the IGD for the "xenfv" machine when the guest is configured with igd-passthru=on. I probably should have Cc'd this patch to you even though you are not a maintainer of core pci because the discussion of this patch to core pci is relevant to your decision about which patch to use (if any) to improve support for the intel IGD with Xen and upstream Qemu. Michael, who is a maintainer of core pci, shows interest in this patch if you decide to pull up the patch to Qemu that reserves slot 2 for the Intel IGD when using the "xenfv" machine configured with igd-passthru=on. As you see from this message, we are waiting on a decision of the Xen maintainers about what patch, if any, will be applied to fix the support for the Intel IGD on Xen in Qemu upstream. I know you have quite a bit of information from me to process before you make a decision about which patch to apply to fix the Intel IGD support in upstream Qemu and Xen, so please take enough time to make the best decision. If you need any more information from me, please let me know by replying to one of my proposed patches to fix support for the Intel IGD. If you are subscribed to qemu-devel, you should have the full discussion of this patch to Qemu core pci in your inbox. You can also access the proposed patch and the discussion of the patch here: https://lore.kernel.org/qemu-devel/ad5f5cf8bc4bd4a720724ed41e47565a7f27adf5.1673829387.git.brchuckz@aol.com/ Kind regards, Chuck
On 1/28/23 2:14 PM, Michael S. Tsirkin wrote: > On Sat, Jan 28, 2023 at 08:20:55AM -0500, Chuck Zmudzinski wrote: >> On 1/28/23 5:26 AM, Michael S. Tsirkin wrote: >> > On Fri, Jan 27, 2023 at 10:39:28PM -0500, Chuck Zmudzinski wrote: >> >> On 1/27/2023 8:28 AM, Michael S. Tsirkin wrote: >> >> > 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. >> >> >> >> I think it would be better for the default value of >> >> enforce_slot_reserved_mask_manual to be false, so that a >> >> user-specified slot will by default override slot_reserved_mask. >> >> But doing that would change the current behavior of >> >> slot_reserved_mask. >> >> >> >> Currently, this is the only place where slot_reserved_mask is used in all >> >> of the Qemu source (code from hw/sparc64/sun4u.c): >> >> >> >> ------ snip ------- >> >> /* Only in-built Simba APBs can exist on the root bus, slot 0 on busA is >> >> reserved (leaving no slots free after on-board devices) however slots >> >> 0-3 are free on busB */ >> >> pci_bus->slot_reserved_mask = 0xfffffffc; >> >> pci_busA->slot_reserved_mask = 0xfffffff1; >> >> pci_busB->slot_reserved_mask = 0xfffffff0; >> >> ------ snip ------- >> >> >> >> I think we could safely change the default value of >> >> enforce_slot_reserved_mask_manual to false but set >> >> it to true for the sparc64 sun4u board here to preserve >> >> the current behavior of the only existing board in Qemu >> >> that uses slot_reserved_mask. >> >> >> >> What do you think? >> > >> > I guess first can you answer whether this is still needed >> > with the latest Xen patches? >> > >> >> It's not really needed except for experimental purposes to allow >> an administrator to test experimental configurations with a device >> other than the igd at slot 2. That might be useful in some cases, >> but it is not really necessary unless someone asks for that capability. >> If libvirt users who ordinarily like to manually specify all the >> settings will be OK with the proposed patch to xen that prevents >> an administrator from being able to override a new setting that >> reserves slot 2 for the igd for type "xenfv" machines configured for >> igd passthrough, then there is no need for this patch. I don't think >> many users need the capability to insert a different device in slot 2 for >> the "xenfv" machine type configured with igd-passthru=on, so I would be >> OK if this patch is not included in qemu. >> >> Chuck > > Pls wait and see if that patch gets picked up. Let me know. > A day or two ago Anthony said he would look at the xen patch soon. So we'll just wait for him, and I'll let you know if he is going to pull it up.
© 2016 - 2024 Red Hat, Inc.