[+cc Rafael for RESUME_EARLY quirk question] On Mon, Mar 20, 2023 at 01:32:16AM +0000, Limonciello, Mario wrote: > > -----Original Message----- > > From: Bjorn Helgaas <helgaas@xxxxxxxxxx> > > Sent: Friday, March 10, 2023 16:14 > > To: Limonciello, Mario <Mario.Limonciello@xxxxxxx> > > Cc: Natikar, Basavaraj <Basavaraj.Natikar@xxxxxxx>; Natikar, Basavaraj > > <Basavaraj.Natikar@xxxxxxx>; bhelgaas@xxxxxxxxxx; linux- > > pci@xxxxxxxxxxxxxxx; thomas@xxxxxxxxxxxx > > Subject: Re: [PATCH] PCI: Add quirk to clear MSI-X > > > > On Thu, Mar 09, 2023 at 06:57:38PM -0600, Mario Limonciello wrote: > > > On 3/9/23 16:30, Bjorn Helgaas wrote: > > > > On Thu, Mar 09, 2023 at 12:32:41PM -0600, Limonciello, Mario wrote: > > > > > On 3/9/2023 12:25, Bjorn Helgaas wrote: > > > > > ... > > > > > > > > > > > > https://gitlab.freedesktop.org/agd5f/linux/- > > /commit/07494a25fc8881e122c242a46b5c53e0e4403139 > > > > > > > > > > > > That nbio_v7.2.c patch and this patch don't look anything > > > > > > alike. It looks like the nbio_v7.2.c patch might run > > > > > > once? Could *this* be done once at enumeration-time, too? > > > > > > > > > > They don't look anything alike because they're attacking the > > > > > problem from different angles. > > > > > > > > Why do we need different angles? > > > > > > The GPU driver approach only works if the GPU is enabled. If > > > the GPU could never be disabled then it alone would be > > > sufficient. > > > > > > > > The NBIO patch fixes the initialization value for the > > > > > internal registers. This is what the BIOS "should" have > > > > > done. When the internal registers are configured properly > > > > > then the behavior the kernel expects works as well. > > > > > > > > > > The NBIO patch will run both at amdgpu startup as well as > > > > > when resuming from suspend. > > > > > > > > If initializing something as BIOS should have done makes the > > > > hardware work correctly, isn't once enough? Why does the NBIO > > > > patch need to run at resume-time? > > > > > > During suspend some internal registers are in a power domain > > > that the state will be lost. These are typically restored by > > > the BIOS to the values defined in initialization tables before > > > handing control back to the OS. > > > > I don't quite get this. I thought I read that if BIOS had > > initialized the hardware correctly, a D0->D3hot->D0 transition > > would work without any issues. Linux can do this with PMCSR > > writes and BIOS isn't involved at all. > > During a suspend transition not all registers are powered. Firmware > will capture some during the suspend transition and restore some of > them for the resume transition, but it's up to the firmware whether > this one is included. > > Furthermore most IP blocks in amdgpu typically initialize the same > during both startup and resume to ensure that firmware couldn't have > mucked with the expected golden state. We're spending way more time on this than makes sense, but I do think it's important that the commit log is accurate and makes sense even to people who don't know the internals of the device. It *sounds* like what's happening is: - OS writes PMCSR to put device in D3hot - BIOS traps D0->D3hot transition via something like SMI and captures MSI-X state - Device enters D3hot - Device internal MSI-X state is lost - BIOS traps D3hot->D0 transition via SMI - Device enters D0 - BIOS restores MSI-X state - OS resumes use of device If that's what's happening, the fact that the device loses the internal state in D3hot sounds like a *hardware* defect -- if you put the device in a system without a BIOS, the D0->D3hot->D0 transitions would not work as required by the PCIe spec. We can call the fact that BIOS lacks the MSI-X save/restore a BIOS defect, but the only reason BIOS would *need* that save/restore is because of the underlying *hardware* defect. If that's the case, I would expect a commit log something like this: The AMD [1022:15b8] USB controller loses some internal functional MSI-X context when transitioning from D0 to D3hot. BIOS normally traps D0->D3hot and D3hot->D0 transitions so it can save and restore that internal context, but some firmware in the field lacks this workaround. If MSI-X is enabled, toggle the PCI_MSIX_FLAGS_ENABLE bit when resuming to D0, which resynchronizes the internal state that was lost in D3hot. Rafael, do we run the DECLARE_PCI_FIXUP_RESUME_EARLY quirks for *all* D3hot->D0 transitions? I'm concerned about places like pci_pm_reset(), where we do D0->D3hot->D0 to do the reset. Or vfio_pm_config_write(), where it looks like a guest could do that without running the quirk. Current proposed patch is: https://lore.kernel.org/r/ddbbfb50-24b6-202f-7452-c8959901c739@xxxxxxx Bjorn
