Re: [PATCH] pci: add enforce_slot_reserved_mask_manual property

[Chuck Zmudzinski]

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.