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

From: Lorenzo Pieralisi <lpieralisi@kernel.org>
To: Nirmal Patel <nirmal.patel@linux.intel.com>
Cc: Bjorn Helgaas <helgaas@kernel.org>, linux-pci@vger.kernel.org
Subject: Re: [PATCH v2] PCI: vmd: Enable Hotplug based on BIOS setting on VMD rootports
Date: Wed, 14 Feb 2024 14:43:38 +0100	[thread overview]
Message-ID: <ZczDiu84/n8qJgSp@lpieralisi> (raw)
In-Reply-To: <2728ad9d38442510c5e0c8174d0f7aae6ff247ac.camel@linux.intel.com>

On Tue, Dec 05, 2023 at 03:20:27PM -0700, Nirmal Patel wrote:

[...]

> > In the host OS, this overrides whatever was negotiated via _OSC, I
> > guess on the principle that _OSC doesn't apply because the host BIOS
> > doesn't know about the Root Ports below the VMD.  (I'm not sure it's
> > 100% resolved that _OSC doesn't apply to them, so we should mention
> > that assumption here.)
> _OSC still controls every flag including Hotplug. I have observed that
> slot capabilities register has hotplug enabled only when platform has
> enabled the hotplug. So technically not overriding it in the host.
> It overrides in guest because _OSC is passing wrong/different
> information than the _OSC information in Host.
> Also like I mentioned, slot capabilities registers are not changed in
> Guest because vmd is passthrough.
> > 
> > In the guest OS, this still overrides whatever was negotiated via
> > _OSC, but presumably the guest BIOS *does* know about the Root Ports,
> > so I don't think the same principle about overriding _OSC applies
> > there.
> > 
> > I think it's still a problem that we handle
> > host_bridge->native_pcie_hotplug differently than all the other
> > features negotiated via _OSC.  We should at least explain this
> > somehow.
> Right now this is the only way to know in Guest OS if platform has
> enabled Hotplug or not. We have many customers complaining about
> Hotplug being broken in Guest. So just trying to honor _OSC but also
> fixing its limitation in Guest.

The question is: should _OSC settings apply to the VMD hierarchy ?

Yes or no (not maybe) ?

If yes, the guest firmware is buggy (and I appreciate it is hard to
fix). If no this patch should also change vmd_copy_host_bridge_flags()
and remove the native_pcie_hotplug initialization from the root bridge.

As a matter of fact this patch overrides the _OSC settings in host and
guest and it is not acceptable because it is not based on any documented
policy (if there is a policy please describe it).

> > I suspect part of the reason for doing it differently is to avoid the
> > interrupt storm that we avoid via 04b12ef163d1 ("PCI: vmd: Honor ACPI
> > _OSC on PCIe features").  If so, I think 04b12ef163d1 should be
> > mentioned here because it's an important piece of the picture.
> I can add a note about this patch as well. Let me know if you want me
> to add something specific.

At the very least you need to explain the problem you are solving in the
commit log - sentences like:

"This patch will make sure that Hotplug is enabled properly in Host
as well as in VM while honoring _OSC settings as well as VMD hotplug
setting."

aren't useful to someone who will look into this in the future.

If _OSC does not grant the host kernel HP handling and *any* (that's
another choice that should be explained) slot
capability reports that the VMD root port is HP capable we do
override _OSC, we are not honouring it, AFAICS.

Then we can say that in your user experience the stars always align and
what I mention above is not a problem because it can't happen but it is hard
to grok by just reading the code and more importantly, it is not based
on any standard documentation.

> Thank you for taking the time. This is a very critical issue for us and
> we have many customers complaining about it, I would appreciate to get
> it resolved as soon as possible.

Sure but we need to solve it in a way that does not break tomorrow
otherwise we will keep applying plasters on top of plasters.

Ignoring _OSC hotplug settings for VMD bridges in *both* host and guests
may be an acceptable - though a tad controversial - policy (if based on
software/hardware guidelines).

A mixture thereof in my opinion is not acceptable.

Thanks,
Lorenzo