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
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAPpJ_efs9nHrTx=N2NwtmN=py3CFg62izcgJrUkcMDd_ErR5VA@mail.gmail.com>
Date: Tue, 25 Jun 2024 18:31:54 +0800
From: Jian-Hong Pan <jhp@...lessos.org>
To: Paul M Stillwell Jr <paul.m.stillwell.jr@...el.com>
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

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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ