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
