linux-kernel - Re: [PATCH] PCI: vmd: Delay interrupt handling on MTL VMD controller

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: <20240904062219.x7kft2l3gq4qsojc@thinkpad>
Date: Wed, 4 Sep 2024 11:52:19 +0530
From: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
To: Kai-Heng Feng <kai.heng.feng@...onical.com>
Cc: Keith Busch <kbusch@...nel.org>, nirmal.patel@...ux.intel.com,
	jonathan.derrick@...ux.dev, acelan.kao@...onical.com,
	lpieralisi@...nel.org, kw@...ux.com, robh@...nel.org,
	bhelgaas@...gle.com, linux-pci@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] PCI: vmd: Delay interrupt handling on MTL VMD controller

On Wed, Sep 04, 2024 at 09:57:08AM +0800, Kai-Heng Feng wrote:
> On Tue, Sep 3, 2024 at 10:51 PM Keith Busch <kbusch@...nel.org> wrote:
> >
> > On Tue, Sep 03, 2024 at 03:07:45PM +0800, Kai-Heng Feng wrote:
> > > On Tue, Sep 3, 2024 at 12:29 PM Manivannan Sadhasivam
> > > <manivannan.sadhasivam@...aro.org> wrote:
> > > >
> > > > On Tue, Sep 03, 2024 at 10:55:44AM +0800, Kai-Heng Feng wrote:
> > > > > Meteor Lake VMD has a bug that the IRQ raises before the DMA region is
> > > > > ready, so the requested IO is considered never completed:
> > > > > [   97.343423] nvme nvme0: I/O 259 QID 2 timeout, completion polled
> > > > > [   97.343446] nvme nvme0: I/O 384 QID 3 timeout, completion polled
> > > > > [   97.343459] nvme nvme0: I/O 320 QID 4 timeout, completion polled
> > > > > [   97.343470] nvme nvme0: I/O 707 QID 5 timeout, completion polled
> > > > >
> > > > > The is documented as erratum MTL016 [0]. The suggested workaround is to
> > > > > "The VMD MSI interrupt-handler should initially perform a dummy register
> > > > > read to the MSI initiator device prior to any writes to ensure proper
> > > > > PCIe ordering." which essentially is adding a delay before the interrupt
> > > > > handling.
> > > > >
> > > >
> > > > Why can't you add a dummy register read instead? Adding a delay for PCIe
> > > > ordering is not going to work always.
> > >
> > > This can be done too. But it can take longer than 4us delay, so I'd
> > > like to keep it a bit faster here.
> >
> > An added delay is just a side effect of the read. The read flushes
> > pending device-to-host writes, which is most likely what the errata
> > really requires. I think Mani is right, you need to pay that register
> > read penalty to truly fix this.
> 
> OK, will change the quirk to perform dummy register read.
> 
> But I am not sure which is the "MSI initiator device", is it VMD
> controller or NVMe devices?
> 

'MSI initiator' should be the NVMe device. My understanding is that the
workaround suggests reading the NVMe register from the MSI handler before doing
any write to the device to ensures that the previous writes from the device are
flushed.

And this sounds like the workaround should be done in the NVMe driver as it has
the knowledge of the NVMe registers. But isn't the NVMe driver already reading
CQE status first up in the ISR?

- Mani

> Kai-Heng
> 
> >
> > > > > +     /* Erratum MTL016 */
> > > > > +     VMD_FEAT_INTERRUPT_QUIRK        = (1 << 6),
> > > > >  };
> > > > >
> > > > >  #define VMD_BIOS_PM_QUIRK_LTR        0x1003  /* 3145728 ns */
> > > > > @@ -90,6 +94,8 @@ static DEFINE_IDA(vmd_instance_ida);
> > > > >   */
> > > > >  static DEFINE_RAW_SPINLOCK(list_lock);
> > > > >
> > > > > +static bool interrupt_delay;
> > > > > +
> > > > >  /**
> > > > >   * struct vmd_irq - private data to map driver IRQ to the VMD shared vector
> > > > >   * @node:    list item for parent traversal.
> > > > > @@ -105,6 +111,7 @@ struct vmd_irq {
> > > > >       struct vmd_irq_list     *irq;
> > > > >       bool                    enabled;
> > > > >       unsigned int            virq;
> > > > > +     bool                    delay_irq;
> > > >
> > > > This is unused. Perhaps you wanted to use this instead of interrupt_delay?
> > >
> > > This is leftover, will scratch this.
> >
> > Maybe you should actually use it instead of making a global? The quirk
> > says it is device specific, so no need to punish every device if it
> > doesn't need it (unlikely as it is to see such a thing).

-- 
மணிவண்ணன் சதாசிவம்