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

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

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?

Thanks.

>
> 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.

>
>> 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
>>