Re: [PATCH v4] PCI: vmd: Do not change the BIOS Hotplug setting on VMD rootports

[Bjorn Helgaas <helgaas@kernel.org>]

On Tue, Sep 12, 2023 at 02:35:39PM -0700, Patel, Nirmal wrote:
> On 8/30/2023 9:55 AM, Bjorn Helgaas wrote:
> > On Tue, Aug 29, 2023 at 02:35:36PM -0700, Patel, Nirmal wrote:
> >> On 8/29/2023 11:00 AM, Bjorn Helgaas wrote:
> >>> On Tue, Aug 29, 2023 at 01:10:22AM -0400, Nirmal Patel wrote:
> >>>> Currently during Host boot up, VMD UEFI driver loads and configures
> >>>> all the VMD endpoints devices and devices behind VMD. Then during
> >>>> VMD rootport creation, VMD driver honors ACPI settings for Hotplug,
> >>>> AER, DPC, PM and enables these features based on BIOS settings.
> >>>>
> >>>> During the Guest boot up, ACPI settings along with VMD UEFI driver are
> >>>> not present in Guest BIOS which results in assigning default values to
> >>>> Hotplug, AER, DPC, etc. As a result hotplug is disabled on the VMD
> >>>> rootports in the Guest OS.
> >>>>
> >>>> VMD driver in Guest should be able to see the same settings as seen
> >>>> by Host VMD driver. Because of the missing implementation of VMD UEFI
> >>>> driver in Guest BIOS, the Hotplug is disabled on VMD rootport in
> >>>> Guest OS. Hot inserted drives don't show up and hot removed drives
> >>>> do not disappear even if VMD supports Hotplug in Guest. This
> >>>> behavior is observed in various combinations of guest OSes i.e. RHEL,
> >>>> SLES and hypervisors i.e. KVM and ESXI.
> >>>>
> >>>> This change will make the VMD Host and Guest Driver to keep the settings
> >>>> implemented by the UEFI VMD DXE driver and thus honoring the user
> >>>> selections for hotplug in the BIOS.
> >>> These settings are negotiated between the OS and the BIOS.  The guest
> >>> runs a different BIOS than the host, so why should the guest setting
> >>> be related to the host setting?
> >>>
> >>> I'm not a virtualization whiz, and I don't understand all of what's
> >>> going on here, so please correct me when I go wrong:
> >>>
> >>> IIUC you need to change the guest behavior.  The guest currently sees
> >>> vmd_bridge->native_pcie_hotplug as FALSE, and you need it to be TRUE?
> >> Correct.
> >>
> >>> Currently this is copied from the guest's
> >>> root_bridge->native_pcie_hotplug, so that must also be FALSE.
> >>>
> >>> I guess the guest sees a fabricated host bridge, which would start
> >>> with native_pcie_hotplug as TRUE (from pci_init_host_bridge()), and
> >>> then it must be set to FALSE because the guest _OSC didn't grant
> >>> ownership to the OS?  (The guest dmesg should show this, right?)
> >> This is my understanding too. I don't know much in detail about Guest
> >> expectation.
> >>
> >>> In the guest, vmd_enable_domain() allocates a host bridge via
> >>> pci_create_root_bus(), and that would again start with
> >>> native_pcie_hotplug as TRUE.  It's not an ACPI host bridge, so I don't
> >>> think we do _OSC negotiation for it.  After this patch removes the
> >>> copy from the fabricated host bridge, it would be left as TRUE.
> >> VMD was not dependent on _OSC settings and is not ACPI Host bridge. It
> >> became _OSC dependent after the patch 04b12ef163d1.
> >> https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/commit/drivers/pci/controller/vmd.c?id=04b12ef163d10e348db664900ae7f611b83c7a0e
> >>
> >> This patch was added as a quick fix for AER flooding but it could
> >> have been avoided by using rate limit for AER.
> >>
> >> I don't know all the history of VMD driver but does it have to be
> >> dependent on root_bridge flags from _OSC? Is reverting 04b12ef163d1
> >> a better idea than not allowing just hotplug flags to be copied from
> >> root_bridge?
> > It seems like the question is who owns AER, hotplug, etc for devices
> > below VMD.  AER rate limiting sounds itself like a quick fix without
> > addressing the underlying problem.
> >
> >>> If this is on track, it seems like if we want the guest to own PCIe
> >>> hotplug, the guest BIOS _OSC for the fabricated host bridge should
> >>> grant ownership of it.
> >> I will try to check this option.
> > On second thought, this doesn't seem right to me.  An _OSC method
> > clearly applies to the hierarchy under that device, e.g., if we have a
> > PNP0A03 host bridge with _SEG 0000 and _CRS that includes [bus 00-ff],
> > its _OSC clearly applies to any devices in domain 0000, which in this
> > case would include the VMD bridge.
> >
> > But I don't think it should apply to the devices *below* the VMD
> > bridge.  Those are in a different domain, and if the platform wants to
> > manage AER, hotplug, etc., for those devices, it would have to know
> > some alternate config access method in order to read the AER and
> > hotplug registers.  I think that config access depends on the setup
> > done by the VMD driver, so the platform doesn't know that.
> >
> > By this argument, I would think the guest _OSC would apply to the VMD
> > device itself, but not to the children of the VMD, and the guest could
> > retain the default native control of all these services inside the VMD
> > domain.
> >
> > But prior to 04b12ef163d1 ("PCI: vmd: Honor ACPI _OSC on PCIe
> > features"), the guest *did* retain native control, so we would have to 
> > resolve the issue solved by 04b12ef163d1 in some other way.
>
> Yes, _OSC settings should applied to devices on 0000 domain.

To be careful here, I think a PNP0A03 _OSC applies to the devices in
the PNP0A03 hierarchy, which could be any domain, not just 0000.

But I think we agree that devices behind a VMD bridge are in some
hierarchy separate from the PNP0A03 one because the PNP0A03 device
doesn't tell OSPM how to find things behind the VMD.

> VMD creates its own domain to manage the child devices. So it is
> against the VMD design to force _OSC settings and overwrite VMD
> settings.
> 
> The patch 04b12ef163d1 disables AER on VMD rootports by using BIOS system
> settings for AER, Hotplug, etc.
> The patch 04b12ef163d1 *assumes VMD is a bridge device* and copies
> and *imposes system settings* for AER, DPC, Hotplug, PM, etc on VMD.
> Borrowing and applying system settings on VMD rootports is
> not correct.

Yes, agreed, and I think this suggests that we really should remove
04b12ef163d1 ("PCI: vmd: Honor ACPI _OSC on PCIe features")
completely.

> VMD is *type 0 PCI endpoint device* and all the PCI devices
> under VMD are *privately* owned by VMD not by the OS. Also VMD has
> its *own Hotplug settings* for its rootports and child devices in BIOS
> under VMD settings that are different from BIOS system settings.
> It is these settings that give VMD its own unique functionality.
> 
> For the above reason, VMD driver should disable AER generation by
> devices it owns. There are two possible solutions.
>
> Possible options to fix: There are two possible solutions.
> 
> Options 1: VMD driver disables AER by copying AER BIOS system settings
> which the patch 04b12ef163d1 does but do not change other settings
> including Hotplug. The proposed patch does that.

This doesn't seem right because I don't think we should be applying
any _OSC settings to devices below VMD unless there's a PNP0A03 device
that describes the domain below VMD.

> Option 2: Either disable AER by adding an extra BIOS settings under
> VMD settings or disable AER by Linux VMD driver by adding a boot
> parameter and remove the patch 04b12ef163d1.

I think we should remove 04b12ef163d1 and figure out some other way
of handling devices below VMD.  Maybe you just hard-code those items
to be what you want.

> >>>> Signed-off-by: Nirmal Patel <nirmal.patel@linux.intel.com>
> >>>> ---
> >>>> v3->v4: Rewrite the commit log.
> >>>> v2->v3: Update the commit log.
> >>>> v1->v2: Update the commit log.
> >>>> ---
> >>>>  drivers/pci/controller/vmd.c | 2 --
> >>>>  1 file changed, 2 deletions(-)
> >>>>
> >>>> diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
> >>>> index 769eedeb8802..52c2461b4761 100644
> >>>> --- a/drivers/pci/controller/vmd.c
> >>>> +++ b/drivers/pci/controller/vmd.c
> >>>> @@ -701,8 +701,6 @@ static int vmd_alloc_irqs(struct vmd_dev *vmd)
> >>>>  static void vmd_copy_host_bridge_flags(struct pci_host_bridge *root_bridge,
> >>>>  				       struct pci_host_bridge *vmd_bridge)
> >>>>  {
> >>>> -	vmd_bridge->native_pcie_hotplug = root_bridge->native_pcie_hotplug;
> >>>> -	vmd_bridge->native_shpc_hotplug = root_bridge->native_shpc_hotplug;
> >>>>  	vmd_bridge->native_aer = root_bridge->native_aer;
> >>>>  	vmd_bridge->native_pme = root_bridge->native_pme;
> >>>>  	vmd_bridge->native_ltr = root_bridge->native_ltr;
> >>>> -- 
> >>>> 2.31.1
> >>>>
>