| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 25 Jun 2024 13:32:06 -0700
From: Paul M Stillwell Jr <paul.m.stillwell.jr@...el.com>
To: Jian-Hong Pan <jhp@...lessos.org>
CC: Nirmal Patel <nirmal.patel@...ux.intel.com>, Bjorn Helgaas
<helgaas@...nel.org>, Francisco Munoz <francisco.munoz.ruiz@...ux.intel.com>,
Johan Hovold <johan@...nel.org>, David Box <david.e.box@...ux.intel.com>,
Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>, "Kuppuswamy
Sathyanarayanan" <sathyanarayanan.kuppuswamy@...ux.intel.com>, "Mika
Westerberg" <mika.westerberg@...ux.intel.com>, Damien Le Moal
<dlemoal@...nel.org>, Jonathan Derrick <jonathan.derrick@...ux.dev>,
<linux-pci@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
<linux@...lessos.org>
Subject: Re: [PATCH v6 3/3] PCI: vmd: Drop resetting PCI bus action after scan
mapped PCI child bus
On 6/25/2024 3:31 AM, Jian-Hong Pan wrote:
> Paul M Stillwell Jr <paul.m.stillwell.jr@...el.com> 於 2024年6月24日 週一 下午11:39寫道：
>>
>> On 6/24/2024 8:24 AM, Nirmal Patel wrote:
>>> On Mon, 24 Jun 2024 16:21:45 +0800
>>> Jian-Hong Pan <jhp@...lessos.org> wrote:
>>>
>>>> According to "PCIe r6.0, sec 5.5.4", before enabling ASPM L1.2 on the
>>>> PCIe Root Port and the child device, they should be programmed with
>>>> the same LTR1.2_Threshold value. However, they have different values
>>>> on VMD mapped PCI child bus. For example, Asus B1400CEAE's VMD mapped
>>>> PCI bridge and NVMe SSD controller have different LTR1.2_Threshold
>>>> values:
>>>>
>>>> 10000:e0:06.0 PCI bridge: Intel Corporation 11th Gen Core Processor
>>>> PCIe Controller (rev 01) (prog-if 00 [Normal decode]) ...
>>>> Capabilities: [200 v1] L1 PM Substates
>>>> L1SubCap: PCI-PM_L1.2+ PCI-PM_L1.1+ ASPM_L1.2+ ASPM_L1.1+
>>>> L1_PM_Substates+ PortCommonModeRestoreTime=45us PortTPowerOnTime=50us
>>>> L1SubCtl1: PCI-PM_L1.2- PCI-PM_L1.1- ASPM_L1.2+ ASPM_L1.1-
>>>> T_CommonMode=45us LTR1.2_Threshold=101376ns
>>>> L1SubCtl2: T_PwrOn=50us
>>>>
>>>> 10000:e1:00.0 Non-Volatile memory controller: Sandisk Corp WD Blue
>>>> SN550 NVMe SSD (rev 01) (prog-if 02 [NVM Express]) ...
>>>> Capabilities: [900 v1] L1 PM Substates
>>>> L1SubCap: PCI-PM_L1.2+ PCI-PM_L1.1- ASPM_L1.2+ ASPM_L1.1-
>>>> L1_PM_Substates+ PortCommonModeRestoreTime=32us PortTPowerOnTime=10us
>>>> L1SubCtl1: PCI-PM_L1.2- PCI-PM_L1.1- ASPM_L1.2+ ASPM_L1.1-
>>>> T_CommonMode=0us LTR1.2_Threshold=0ns
>>>> L1SubCtl2: T_PwrOn=10us
>>>>
>>>> After debug in detail, both of the VMD mapped PCI bridge and the NVMe
>>>> SSD controller have been configured properly with the same
>>>> LTR1.2_Threshold value. But, become misconfigured after reset the VMD
>>>> mapped PCI bus which is introduced from commit 0a584655ef89 ("PCI:
>>>> vmd: Fix secondary bus reset for Intel bridges") and commit
>>>> 6aab5622296b ("PCI: vmd: Clean up domain before enumeration"). So,
>>>> drop the resetting PCI bus action after scan VMD mapped PCI child bus.
>>>>
>>>> Signed-off-by: Jian-Hong Pan <jhp@...lessos.org>
>>>> ---
>>>> v6:
>>>> - Introduced based on the discussion
>>>> https://lore.kernel.org/linux-pci/CAPpJ_efYWWxGBopbSQHB=Y2+1RrXFR2XWeqEhGTgdiw3XX0Jmw@mail.gmail.com/
>>>>
>>>> drivers/pci/controller/vmd.c | 20 --------------------
>>>> 1 file changed, 20 deletions(-)
>>>>
>>>> diff --git a/drivers/pci/controller/vmd.c
>>>> b/drivers/pci/controller/vmd.c index 5309afbe31f9..af413cdb4f4e 100644
>>>> --- a/drivers/pci/controller/vmd.c
>>>> +++ b/drivers/pci/controller/vmd.c
>>>> @@ -793,7 +793,6 @@ static int vmd_enable_domain(struct vmd_dev *vmd,
>>>> unsigned long features) resource_size_t offset[2] = {0};
>>>> resource_size_t membar2_offset = 0x2000;
>>>> struct pci_bus *child;
>>>> - struct pci_dev *dev;
>>>> int ret;
>>>>
>>>> /*
>>>> @@ -935,25 +934,6 @@ static int vmd_enable_domain(struct vmd_dev
>>>> *vmd, unsigned long features) pci_scan_child_bus(vmd->bus);
>>>> vmd_domain_reset(vmd);
>>>>
>>>> - /* When Intel VMD is enabled, the OS does not discover the
>>>> Root Ports
>>>> - * owned by Intel VMD within the MMCFG space.
>>>> pci_reset_bus() applies
>>>> - * a reset to the parent of the PCI device supplied as
>>>> argument. This
>>>> - * is why we pass a child device, so the reset can be
>>>> triggered at
>>>> - * the Intel bridge level and propagated to all the children
>>>> in the
>>>> - * hierarchy.
>>>> - */
>>>> - list_for_each_entry(child, &vmd->bus->children, node) {
>>>> - if (!list_empty(&child->devices)) {
>>>> - dev = list_first_entry(&child->devices,
>>>> - struct pci_dev,
>>>> bus_list);
>>>> - ret = pci_reset_bus(dev);
>>>> - if (ret)
>>>> - pci_warn(dev, "can't reset device:
>>>> %d\n", ret); -
>>>> - break;
>>>> - }
>>>> - }
>>>> -
>>>> pci_assign_unassigned_bus_resources(vmd->bus);
>>>>
>>>> pci_walk_bus(vmd->bus, vmd_pm_enable_quirk, &features);
>>>
>>> Thanks for the patch.
>>>
>>> pci_reset_bus is required to avoid failure in vmd domain creation
>>> during multiple soft reboots test. So I believe we can't just remove
>>> it without proper testing. vmd_pm_enable_quirk happens after
>>> pci_reset_bus, then how is it resetting LTR1.2_Threshold value?
>>>
>>> Thanks
>>> -nirmal
>>
>> To follow up on what Nirmal said: why can't you set the threshold value
>> in vmd_pm_enable_quirk() since we look at LTR there?
>
> Looks like setting the threshold value again in vmd_pm_enable_quirk()
> is the preferred direction?
> If so, I can prepare for that in the next version patch.
>
> Jian-Hong Pan
>
Yes, I think keeping all the LTR code together is the best thing.
Paul
Powered by blists - more mailing lists