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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAAd53p4cyOvhkorHBkt227_KKcCoKZJ+SM13n_97fmTTq_HLuQ@mail.gmail.com>
Date: Wed, 4 Sep 2024 09:57:08 +0800
From: Kai-Heng Feng <kai.heng.feng@...onical.com>
To: Keith Busch <kbusch@...nel.org>
Cc: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.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 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?

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).