| 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
| ||
|
Message-ID: <a2dfd120-2305-4793-9b9f-c978ee692ecb@amd.com>
Date: Thu, 19 Jun 2025 19:11:32 +1000
From: Alexey Kardashevskiy <aik@....com>
To: Mario Limonciello <superm1@...nel.org>,
"Cabiddu, Giovanni" <giovanni.cabiddu@...el.com>,
Alex Williamson <alex.williamson@...hat.com>
Cc: bhelgaas@...gle.com, mario.limonciello@....com,
rafael.j.wysocki@...el.com, huang.ying.caritas@...il.com,
stern@...land.harvard.edu, linux-pci@...r.kernel.org,
mike.ximing.chen@...el.com, ahsan.atta@...el.com,
suman.kumar.chakraborty@...el.com, kvm@...r.kernel.org,
linux-crypto@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] PCI: Explicitly put devices into D0 when initializing
- Bug report
On 12/6/25 06:45, Mario Limonciello wrote:
> On 6/11/2025 9:13 AM, Cabiddu, Giovanni wrote:
>> On Wed, Jun 11, 2025 at 10:00:02AM -0600, Alex Williamson wrote:
>>> On Wed, 11 Jun 2025 06:50:59 -0700
>>> Mario Limonciello <superm1@...nel.org> wrote:
>>>
>>>> On 6/11/2025 5:52 AM, Cabiddu, Giovanni wrote:
>>>>> Hi Mario, Bjorn and Alex,
>>>>>
>>>>> On Wed, Apr 23, 2025 at 11:31:32PM -0500, Mario Limonciello wrote:
>>>>>> From: Mario Limonciello <mario.limonciello@....com>
>>>>>>
>>>>>> AMD BIOS team has root caused an issue that NVME storage failed to come
>>>>>> back from suspend to a lack of a call to _REG when NVME device was probed.
>>>>>>
>>>>>> commit 112a7f9c8edbf ("PCI/ACPI: Call _REG when transitioning D-states")
>>>>>> added support for calling _REG when transitioning D-states, but this only
>>>>>> works if the device actually "transitions" D-states.
>>>>>>
>>>>>> commit 967577b062417 ("PCI/PM: Keep runtime PM enabled for unbound PCI
>>>>>> devices") added support for runtime PM on PCI devices, but never actually
>>>>>> 'explicitly' sets the device to D0.
>>>>>>
>>>>>> To make sure that devices are in D0 and that platform methods such as
>>>>>> _REG are called, explicitly set all devices into D0 during initialization.
>>>>>>
>>>>>> Fixes: 967577b062417 ("PCI/PM: Keep runtime PM enabled for unbound PCI devices")
>>>>>> Signed-off-by: Mario Limonciello <mario.limonciello@....com>
>>>>>> ---
>>>>> Through a bisect, we identified that this patch, in v6.16-rc1,
>>>>> introduces a regression on vfio-pci across all Intel QuickAssist (QAT)
>>>>> devices. Specifically, the ioctl VFIO_GROUP_GET_DEVICE_FD call fails
>>>>> with -EACCES.
>>>>>
>>>>> Upon further investigation, the -EACCES appears to originate from the
>>>>> rpm_resume() function, which is called by pm_runtime_resume_and_get()
>>>>> within vfio_pci_core_enable(). Here is the exact call trace:
>>>>>
>>>>> drivers/base/power/runtime.c: rpm_resume()
>>>>> drivers/base/power/runtime.c: __pm_runtime_resume()
>>>>> include/linux/pm_runtime.h: pm_runtime_resume_and_get()
>>>>> drivers/vfio/pci/vfio_pci_core.c: vfio_pci_core_enable()
>>>>> drivers/vfio/pci/vfio_pci.c: vfio_pci_open_device()
>>>>> drivers/vfio/vfio_main.c: device->ops->open_device()
>>>>> drivers/vfio/vfio_main.c: vfio_df_device_first_open()
>>>>> drivers/vfio/vfio_main.c: vfio_df_open()
>>>>> drivers/vfio/group.c: vfio_df_group_open()
>>>>> drivers/vfio/group.c: vfio_device_open_file()
>>>>> drivers/vfio/group.c: vfio_group_ioctl_get_device_fd()
>>>>> drivers/vfio/group.c: vfio_group_fops_unl_ioctl(..., VFIO_GROUP_GET_DEVICE_FD, ...)
>>>>>
>>>>> Is this a known issue that affects other devices? Is there any ongoing
>>>>> discussion or fix in progress?
>>>>>
>>>>> Thanks,
>>>>
>>>> This is the first I've heard about an issue with that patch.
>>>>
>>>> Does setting the VFIO parameter disable_idle_d3 help?
>>>>
>>>> If so; this feels like an imbalance of runtime PM calls in the VFIO
>>>> stack that this patch exposed.
>>>>
>>>> Alex, any ideas?
>>>
>>> Does the device in question have a PM capability? I note that
>>> 4d4c10f763d7 makes the sequence:
>>>
>>> pm_runtime_forbid(&dev->dev);
>>> pm_runtime_set_active(&dev->dev);
>>> pm_runtime_enable(&dev->dev);
>>>
>>> Dependent on the presence of a PM capability. The PM capability is
>>> optional on SR-IOV VFs. This feels like a bug in the original patch,
>>> we should be able to use pm_runtime ops on a device without
>>> specifically checking if the device supports PCI PM.
>>>
>>> vfio-pci also has a somewhat unique sequence versus other drivers, we
>>> don't call pci_enable_device() until the user opens the device, but we
>>> want to put the device into low power before that occurs. Historically
>>> PCI-core left device in an unknown power state between driver uses, so
>>> we've needed to manually move the device to D0 before calling
>>> pm_runtime_allow() and pm_runtime_put() (see
>>> vfio_pci_core_register_device()). Possibly this is redundant now but
>>> we're using pci_set_power_state() which shouldn't interact with
>>> pm_runtime, so my initial guess is that we might be unbalanced because
>>> this is a VF w/o a PM capability and we've missed the expected
>>> pm_runtime initialization sequence. Thanks,
>>
>> Yes, for Intel QAT, the issue occurs with a VF without the PM capability.
>>
>> Thanks,
>>
>
> Got it, thanks Alex! I think this should help return it to previous behavior for devices without runtime PM and still fix the problem it needed to.
Seems working for me too, thanks,
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 3dd44d1ad829..c495c3c692f5 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -3221,15 +3221,17 @@ void pci_pm_init(struct pci_dev *dev)
>
> /* find PCI PM capability in list */
> pm = pci_find_capability(dev, PCI_CAP_ID_PM);
> - if (!pm)
> + if (!pm) {
> + goto poweron;
> return;
> + }
> /* Check device's ability to generate PME# */
> pci_read_config_word(dev, pm + PCI_PM_PMC, &pmc);
>
> if ((pmc & PCI_PM_CAP_VER_MASK) > 3) {
> pci_err(dev, "unsupported PM cap regs version (%u)\n",
> pmc & PCI_PM_CAP_VER_MASK);
> - return;
> + goto poweron;
> }
>
> dev->pm_cap = pm;
> @@ -3274,6 +3276,7 @@ void pci_pm_init(struct pci_dev *dev)
> pci_read_config_word(dev, PCI_STATUS, &status);
> if (status & PCI_STATUS_IMM_READY)
> dev->imm_ready = 1;
> +poweron:
> pci_pm_power_up_and_verify_state(dev);
> pm_runtime_forbid(&dev->dev);
> pm_runtime_set_active(&dev->dev);
--
Alexey
Powered by blists - more mailing lists