Re: [PATCH v2] PCI: vmd: Enable Hotplug based on BIOS setting on VMD rootports

[Nirmal Patel <nirmal.patel@linux.intel.com>]

On Thu, 2023-12-14 at 15:22 -0700, Nirmal Patel wrote:
> On Thu, 2023-12-14 at 13:23 -0600, Bjorn Helgaas wrote:
> > On Wed, Dec 13, 2023 at 06:07:02PM -0700, Nirmal Patel wrote:
> > > On Tue, 2023-12-12 at 15:13 -0600, Bjorn Helgaas wrote:
> > > > On Mon, Dec 11, 2023 at 04:05:25PM -0700, Nirmal Patel wrote:
> > > > > On Tue, 2023-12-05 at 18:27 -0600, Bjorn Helgaas wrote:
> > > > > > On Tue, Dec 05, 2023 at 03:20:27PM -0700, Nirmal Patel
> > > > > > wrote:
> > > > > > ...
> > > > > > > Currently Hotplug is controlled by _OSC in both host and
> > > > > > > Guest.
> > > > > > > In case of guest, it seems guest BIOS hasn't implemented
> > > > > > > _OSC
> > > > > > > since all the flags are set to 0 which is not the case in
> > > > > > > host.
> > > > > > 
> > > > > > I think you want pciehp to work on the VMD Root Ports in
> > > > > > the
> > > > > > Guest, no matter what, on the assumption that any _OSC that
> > > > > > applies to host bridge A does not apply to the host bridge
> > > > > > created
> > > > > > by the vmd driver.
> > > > > > 
> > > > > > If so, we should just revert 04b12ef163d1 ("PCI: vmd: Honor
> > > > > > ACPI
> > > > > > _OSC on PCIe features").  Since host bridge B was not
> > > > > > enumerated
> > > > > > via ACPI, the OS owns all those features under B by
> > > > > > default,
> > > > > > so
> > > > > > reverting 04b12ef163d1 would leave that state.
> > > > > > 
> > > > > > Obviously we'd have to first figure out another solution
> > > > > > for
> > > > > > the
> > > > > > AER message flood that 04b12ef163d1 resolved.
> > > > > 
> > > > > If we revert 04b12ef163d1, then VMD driver will still enable
> > > > > AER
> > > > > blindly which is a bug. So we need to find a way to make VMD
> > > > > driver
> > > > > aware of AER platform setting and use that information to
> > > > > enable or
> > > > > disable AER for its child devices.
> > > > > 
> > > > > There is a setting in BIOS that allows us to enable OS native
> > > > > AER
> > > > > support on the platform. This setting is located in EDK Menu
> > > > > ->
> > > > > Platform configuration -> system event log -> IIO error
> > > > > enabling ->
> > > > > OS native AER support.
> > > > > 
> > > > > This setting is assigned to VMD bridge by
> > > > > vmd_copy_host_bridge_flags in patch 04b12ef163d1. Without the
> > > > > patch 04b12ef163d1, VMD driver will enable AER even if
> > > > > platform
> > > > > has disabled OS native AER support as mentioned earlier. This
> > > > > will result in an AER flood mentioned in 04b12ef163d1 since
> > > > > there is no AER handlers. 
> > 
> > I missed this before.  What does "there are no AER handlers"
> > mean?  Do
> > you mean there are no *firmware* AER handlers?  
> Sorry for confusing wordings. Your understanding is correct. The
> patch
> 04b12ef163d1 is used to disable AER on vmd and avoid the AER flooding
> by applying _OSC settings since vmd driver doesn't give a choice to
> toggle AER, DPC, LTR, etc.
> > The dmesg log at 
> > https://bugzilla.kernel.org/attachment.cgi?id=299571
> > (from https://bugzilla.kernel.org/show_bug.cgi?id=215027, the bug
> > fixed by 04b12ef163d1), shows this:
> > 
> >   acpi PNP0A08:00: _OSC: OS supports [ExtendedConfig ASPM ClockPM
> > Segments MSI HPX-Type3]
> >   acpi PNP0A08:00: _OSC: platform does not support [AER]
> >   acpi PNP0A08:00: _OSC: OS now controls [PCIeHotplug SHPCHotplug
> > PME
> > PCIeCapability LTR]
> > 
> > so the firmware did not grant AER control to the OS (I think
> > "platform
> > does not support" is a confusing description).
> > 
> > Prior to 04b12ef163d1, Linux applied _OSC above the VMD bridge but
> > not
> > below it, so Linux had native control below VMD and it did handle
> > AER
> > interrupts from the 10000:e0:06.0 Root Port below VMD:
> > 
> >   vmd 0000:00:0e.0: PCI host bridge to bus 10000:e0
> >   pcieport 10000:e0:06.0: AER: Corrected error received:
> > 10000:e1:00.0
> > 
> > After 04b12ef163d1, Linux applied _OSC below the VMD bridge as
> > well,
> > so it did not enable or handle any AER interrupts.  I suspect the
> > platform didn't handle AER interrupts below VMD either, so those
> > errors were probably just ignored.
> > 
> > > > > If VMD is aware of OS native AER support setting, then we
> > > > > will
> > > > > not
> > > > > see AER flooding issue.
> > > > > 
> > > > > Do you have any suggestion on how to make VMD driver aware of
> > > > > AER
> > > > > setting and keep it in sync with platform setting.
> > > > 
> > > > Well, this is the whole problem.  IIUC, you're saying we should
> > > > use
> > > > _OSC to negotiate for AER control below the VMD bridge, but
> > > > ignore
> > > > _OSC for hotplug control.
> > > 
> > > Because VMD has its own hotplug BIOS setting which allows vmd to
> > > enable or disable hotplug on its domain. However we don't have
> > > VMD
> > > specific AER, DPC, LTR settings. 
> > 
> > I don't quite follow.  The OS knows nothing about whether BIOS
> > settings exist, so they can't be used to determine where _OSC
> > applies.
> > 
> > > Is it okay if we include an additional check for hotplug? i.e.
> > > Hotplug capable bit in SltCap register which reflects VMD BIOS
> > > hotplug setting.
> > 
> > I don't know what check you have in mind, but the OS can definitely
> > use SltCap to decide what features to enable.
> Yes I agree. That is what I am also suggesting in this patch.
> > You suggest above that you want vmd to be aware of AER ownership
> > per
> > _OSC, but it sounds more like you just want AER disabled below VMD
> > regardless.  Or do you have platforms that can actually handle AER
> > interrupts from Root Ports below VMD?
> No I dont want AER disabled below VMD all the time. I am suggesting
> vmd
> should be in sync with all the _OSC flags since vmd doesn't give a
> choice to toggle. However, the issue arises in guest where _OSC
> setting
> for hotplug is always 0 eventhough hotplug is 1 in host and hotplug
> enable bit in SltCap register is 1 in both host and guest. So we can
> use that to enable hotplug in guest. Like you suggested in your above
> comment. 
> > > Another way is to overwrite _OSC setting for hotplug only in
> > > Guest
> > > OS.  If we make VMD driver aware of Host or Guest environment,
> > > only
> > > in case of Guest we overwrite _OSC hotplug using SltCap register
> > > and
> > > we don't revert the 04b12ef163d1.
> > 
> > Making vmd aware of being in host or guest sounds kind of ugly to
> > me.
> I agree.
> > Bjorn
Hi Bjorn,

Happy new year. sending a gentle reminder to help conclude the
discussion.

Thanks
nirmal