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

From: Nirmal Patel <nirmal.patel@linux.intel.com>
To: Kai-Heng Feng <kai.heng.feng@canonical.com>
Cc: Bjorn Helgaas <helgaas@kernel.org>,
	Lorenzo Pieralisi <lpieralisi@kernel.org>,
	linux-pci@vger.kernel.org,
	"Rafael J. Wysocki" <rjw@rjwysocki.net>,
	paul.m.stillwell.jr@intel.com
Subject: Re: [PATCH v2] PCI: vmd: Enable Hotplug based on BIOS setting on VMD rootports
Date: Thu, 7 Mar 2024 17:09:04 -0700	[thread overview]
Message-ID: <20240307170904.00001cd4@linux.intel.com> (raw)
In-Reply-To: <CAAd53p4j3MwigsFXpftuDdSfhn7EH_-cOOjP2DqnqeAuD+Fb=Q@mail.gmail.com>

On Thu, 7 Mar 2024 14:44:23 +0800
Kai-Heng Feng <kai.heng.feng@canonical.com> wrote:

> Hi Nirmal,
> 
> On Thu, Mar 7, 2024 at 6:20 AM Nirmal Patel
> <nirmal.patel@linux.intel.com> wrote:
> >
> > On Tue, 2024-02-13 at 10:47 -0700, Nirmal Patel wrote:  
> > > On Wed, 2024-02-07 at 12:55 -0600, Bjorn Helgaas wrote:  
> > > > On Tue, Feb 06, 2024 at 05:27:29PM -0700, Nirmal Patel wrote:  
> > > > > ...
> > > > > Did you have a chance to look at my response on January 16th
> > > > > to your
> > > > > questions? I tried to summarize all of the potential problems
> > > > > and issues with different fixes. Please let me know if it is
> > > > > easier if
> > > > > I
> > > > > resend the explanation. Thanks.  
> > > >
> > > > I did see your Jan 16 response, thanks.
> > > >
> > > > I had more questions after reading it, but they're more about
> > > > understanding the topology seen by the host and the guest:
> > > >   Jan 16:
> > > > https://lore.kernel.org/r/20240117004933.GA108810@bhelgaas
> > > >   Feb  1:
> > > > https://lore.kernel.org/r/20240201211620.GA650432@bhelgaas
> > > >
> > > > As I mentioned in my second Feb 1 response
> > > > (https://lore.kernel.org/r/20240201222245.GA650725@bhelgaas),
> > > > the usual plan envisioned by the PCI Firmware spec is that an
> > > > OS can use
> > > > a
> > > > PCIe feature if the platform has granted the OS ownership via
> > > > _OSC and
> > > > a device advertises the feature via a Capability in config
> > > > space.
> > > >
> > > > My main concern with the v2 patch
> > > > (
> > > > https://lore.kernel.org/r/20231127211729.42668-1-nirmal.patel@linux.intel.com
> > > > )
> > > > is that it overrides _OSC for native_pcie_hotplug, but not for
> > > > any of
> > > > the other features (AER, PME, LTR, DPC, etc.)
> > > >
> > > > I think it's hard to read the specs and conclude that PCIe
> > > > hotplug is
> > > > a special case, and I think we're likely to have similar issues
> > > > with
> > > > other features in the future.
> > > >
> > > > But if you think this is the best solution, I'm OK with merging
> > > > it.  
> > Hi Bjorn,
> >
> > We tried some other ways to pass the information about all of the
> > flags but I couldn't retrieve it in guest OS and VMD hardware
> > doesn't have empty registers to write this information. So as of
> > now we have this solution which only overwrites Hotplug flag. If
> > you can accept it that would be great.  
> 
> My original commit "PCI: vmd: Honor ACPI _OSC on PCIe features" was a
> logically conclusion based on VMD ports are just apertures.
> Apparently there are more nuances than that, and people outside of
> Intel can't possibly know. And yes VMD creates lots of headaches
> during hardware enablement.
> 
> So is it possible to document the right way to use _OSC, as Bjorn
> suggested?
> 
> Kai-Heng
Well we are stuck here with two issues which are kind of chicken and egg
problem.
1) VMD respects _OSC; which works excellent in Host but _OSC gives
wrong information in Guest OS which ends up disabling Hotplug, AER,
DPC, etc. We can live with AER, DPC disabled in VM but not the Hotplug.

2) VMD takes ownership of all the flags. For this to work VMD needs to
know these settings from BIOS. VMD hardware doesn't have empty register
where VMD UEFI driver can write this information. So honoring user
settings for AER, Hotplug, etc from BIOS is not possible.

Where do you want me to add documentation? Will more information in
the comment section of the Sltcap code change help?

Architecture wise VMD must own all of these flags but we have a
hardware limitation to successfully pass the necessary information to
the Host OS and the Guest OS.

Thanks
nirmal
> 
> > > 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.
> >
> > Thanks
> > nirmal.
> >  
> > > I sincerely apologize for late responses. I just found out that my
> > > evolution mail client is automatically sending linux-pci emails to
> > > junk
> > > since January 2024.
> > >
> > > At the moment Hotplug is an exception for us, but I do share your
> > > concern about other flags. We have done lot of testing and so far
> > > patch
> > > v2 is the best solution we have.
> > >
> > > Thanks
> > > nirmal  
> > > > Bjorn  
> >  