Commit d7d8ab87e3e7 ("PCI: vmd: Switch to msi_create_parent_irq_domain()")
added a WARN_ON sanity check that child devices support MSI-X, because VMD
document says [1]:

    "Intel VMD only supports MSIx Interrupts from child devices and
    therefore the BIOS must enable PCIe Hot Plug and MSIx interrups [sic]."

However, on Ammar's machine, a PCIe port below VMD does not support MSI-X,
triggering this WARN_ON.

This inconsistency between the document and reality should be investigated
further. For now, remove the MSI-X check.

Allowing child devices without MSI-X despite what the document says does
sound suspicious, but that's what the driver had been doing before the
WARN_ON is added.

Fixes: d7d8ab87e3e7 ("PCI: vmd: Switch to msi_create_parent_irq_domain()")
Link: https://cdrdv2-public.intel.com/776857/VMD_White_Paper.pdf [1]
Reported-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
Closes: https://lore.kernel.org/linux-pci/aJXYhfc%2F6DfcqfqF@linux.gnuweeb.org/
Tested-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
Signed-off-by: Nam Cao <namcao@linutronix.de>
---
 drivers/pci/controller/vmd.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
index b679c7f28f51..1bd5bf4a6097 100644
--- a/drivers/pci/controller/vmd.c
+++ b/drivers/pci/controller/vmd.c
@@ -306,9 +306,6 @@ static bool vmd_init_dev_msi_info(struct device *dev, struct irq_domain *domain,
 				  struct irq_domain *real_parent,
 				  struct msi_domain_info *info)
 {
-	if (WARN_ON_ONCE(info->bus_token != DOMAIN_BUS_PCI_DEVICE_MSIX))
-		return false;
-
 	if (!msi_lib_init_dev_msi_info(dev, domain, real_parent, info))
 		return false;
 
-- 
2.39.5

On Mon, Aug 11, 2025 at 07:39:35AM +0200, Nam Cao wrote:
> Commit d7d8ab87e3e7 ("PCI: vmd: Switch to msi_create_parent_irq_domain()")
> added a WARN_ON sanity check that child devices support MSI-X, because VMD
> document says [1]:
> 
>     "Intel VMD only supports MSIx Interrupts from child devices and
>     therefore the BIOS must enable PCIe Hot Plug and MSIx interrups [sic]."

Can VMD tell the difference between an incoming MSI MWr transaction
and an MSI-X MWr?

I wonder if "MSIx" was meant to mean "VMD only supports MSI or MSI-X
interrupts, not INTx interrupts, from child devices"?

I put this on pci/for-linus for v6.17, but it seems like we might want
to clarify the commit log.

> However, on Ammar's machine, a PCIe port below VMD does not support MSI-X,
> triggering this WARN_ON.
> 
> This inconsistency between the document and reality should be investigated
> further. For now, remove the MSI-X check.
> 
> Allowing child devices without MSI-X despite what the document says does
> sound suspicious, but that's what the driver had been doing before the
> WARN_ON is added.
> 
> Fixes: d7d8ab87e3e7 ("PCI: vmd: Switch to msi_create_parent_irq_domain()")
> Link: https://cdrdv2-public.intel.com/776857/VMD_White_Paper.pdf [1]
> Reported-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
> Closes: https://lore.kernel.org/linux-pci/aJXYhfc%2F6DfcqfqF@linux.gnuweeb.org/
> Tested-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
> Signed-off-by: Nam Cao <namcao@linutronix.de>
> ---
>  drivers/pci/controller/vmd.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
> index b679c7f28f51..1bd5bf4a6097 100644
> --- a/drivers/pci/controller/vmd.c
> +++ b/drivers/pci/controller/vmd.c
> @@ -306,9 +306,6 @@ static bool vmd_init_dev_msi_info(struct device *dev, struct irq_domain *domain,
>  				  struct irq_domain *real_parent,
>  				  struct msi_domain_info *info)
>  {
> -	if (WARN_ON_ONCE(info->bus_token != DOMAIN_BUS_PCI_DEVICE_MSIX))
> -		return false;
> -
>  	if (!msi_lib_init_dev_msi_info(dev, domain, real_parent, info))
>  		return false;
>  
> -- 
> 2.39.5
>

On Mon, Aug 11, 2025 at 05:46:59PM -0500, Bjorn Helgaas wrote:
> On Mon, Aug 11, 2025 at 07:39:35AM +0200, Nam Cao wrote:
> > Commit d7d8ab87e3e7 ("PCI: vmd: Switch to msi_create_parent_irq_domain()")
> > added a WARN_ON sanity check that child devices support MSI-X, because VMD
> > document says [1]:
> > 
> >     "Intel VMD only supports MSIx Interrupts from child devices and
> >     therefore the BIOS must enable PCIe Hot Plug and MSIx interrups [sic]."
> 
> Can VMD tell the difference between an incoming MSI MWr transaction
> and an MSI-X MWr?
> 
> I wonder if "MSIx" was meant to mean "VMD only supports MSI or MSI-X
> interrupts, not INTx interrupts, from child devices"?

I don't know, sorry. I am hoping that the VMD maintainers can help us here.

But I don't expect to get an answer soon, and this is affecting users, so I
prefer to merge it regardless.

And to be honest, depending on the answer, this patch may be the wrong fix.
But in this worst case, we would just bring the driver back to how it had
always behaved, and no one complained before. Therefore it should be safe to
do.

Nam

On Tue, Aug 12, 2025 at 08:27:15AM +0200, Nam Cao wrote:
> On Mon, Aug 11, 2025 at 05:46:59PM -0500, Bjorn Helgaas wrote:
> > On Mon, Aug 11, 2025 at 07:39:35AM +0200, Nam Cao wrote:
> > > Commit d7d8ab87e3e7 ("PCI: vmd: Switch to msi_create_parent_irq_domain()")
> > > added a WARN_ON sanity check that child devices support MSI-X, because VMD
> > > document says [1]:
> > > 
> > >     "Intel VMD only supports MSIx Interrupts from child devices and
> > >     therefore the BIOS must enable PCIe Hot Plug and MSIx interrups [sic]."
> > 
> > Can VMD tell the difference between an incoming MSI MWr transaction
> > and an MSI-X MWr?
> > 
> > I wonder if "MSIx" was meant to mean "VMD only supports MSI or MSI-X
> > interrupts, not INTx interrupts, from child devices"?
> 
> I don't know, sorry. I am hoping that the VMD maintainers can help us here.

The doc you linked is riddled with errors. The original vmd commit
message is more accurate: VMD domains support child devices with MSI and
MSI-x interrupts. The VMD device can't even tell the difference which
one the device is using. It just manipulates messages sent to the usual
APIC address 0xfeeXXXXX.

On Tue, Aug 12, 2025 at 08:49:50AM -0600, Keith Busch wrote:
> On Tue, Aug 12, 2025 at 08:27:15AM +0200, Nam Cao wrote:
> > On Mon, Aug 11, 2025 at 05:46:59PM -0500, Bjorn Helgaas wrote:
> > > On Mon, Aug 11, 2025 at 07:39:35AM +0200, Nam Cao wrote:
> > > > Commit d7d8ab87e3e7 ("PCI: vmd: Switch to msi_create_parent_irq_domain()")
> > > > added a WARN_ON sanity check that child devices support MSI-X, because VMD
> > > > document says [1]:
> > > > 
> > > >     "Intel VMD only supports MSIx Interrupts from child devices and
> > > >     therefore the BIOS must enable PCIe Hot Plug and MSIx interrups [sic]."
> > > 
> > > Can VMD tell the difference between an incoming MSI MWr transaction
> > > and an MSI-X MWr?
> > > 
> > > I wonder if "MSIx" was meant to mean "VMD only supports MSI or MSI-X
> > > interrupts, not INTx interrupts, from child devices"?
> > 
> > I don't know, sorry. I am hoping that the VMD maintainers can help us here.
> 
> The doc you linked is riddled with errors. The original vmd commit
> message is more accurate: VMD domains support child devices with MSI and
> MSI-x interrupts. The VMD device can't even tell the difference which
> one the device is using. It just manipulates messages sent to the usual
> APIC address 0xfeeXXXXX.

Thanks, Keith!  I updated the commit log like this:

  d7d8ab87e3e7 ("PCI: vmd: Switch to msi_create_parent_irq_domain()") added a
  WARN_ON sanity check that child devices support MSI-X, because VMD document
  says [1]:

    Intel VMD only supports MSIx Interrupts from child devices and therefore
    the BIOS must enable PCIe Hot Plug and MSIx interrups [sic].

  However, the VMD device can't even tell the difference between a child
  device using MSI and one using MSI-X.  Per 185a383ada2e ("x86/PCI: Add
  driver for Intel Volume Management Device (VMD)"), VMD does not support
  INTx interrupts, but does support child devices using either MSI or MSI-X.

  Remove the sanity check to avoid the unnecessary WARN_ON reported by Ammar.

On Tue, Aug 12, 2025 at 11:30:15AM -0500, Bjorn Helgaas wrote:
> On Tue, Aug 12, 2025 at 08:49:50AM -0600, Keith Busch wrote:
> > The doc you linked is riddled with errors. The original vmd commit
> > message is more accurate: VMD domains support child devices with MSI and
> > MSI-x interrupts. The VMD device can't even tell the difference which
> > one the device is using. It just manipulates messages sent to the usual
> > APIC address 0xfeeXXXXX.
> 
> Thanks, Keith!  I updated the commit log like this:
> 
>   d7d8ab87e3e7 ("PCI: vmd: Switch to msi_create_parent_irq_domain()") added a
>   WARN_ON sanity check that child devices support MSI-X, because VMD document
>   says [1]:
> 
>     Intel VMD only supports MSIx Interrupts from child devices and therefore
>     the BIOS must enable PCIe Hot Plug and MSIx interrups [sic].
> 
>   However, the VMD device can't even tell the difference between a child
>   device using MSI and one using MSI-X.  Per 185a383ada2e ("x86/PCI: Add
>   driver for Intel Volume Management Device (VMD)"), VMD does not support
>   INTx interrupts, but does support child devices using either MSI or MSI-X.
> 
>   Remove the sanity check to avoid the unnecessary WARN_ON reported by Ammar.

Minor correction, it is not just an unnecessary WARN_ON, but child devices'
drivers couldn't enable MSI at all.

So perhaps something like "Remove the sanity check to allow child devices
which only support MSI".

Thank you both,
Nam

On Tue, Aug 12, 2025 at 08:22:09PM +0200, Nam Cao wrote:
> On Tue, Aug 12, 2025 at 11:30:15AM -0500, Bjorn Helgaas wrote:
> > On Tue, Aug 12, 2025 at 08:49:50AM -0600, Keith Busch wrote:
> > > The doc you linked is riddled with errors. The original vmd commit
> > > message is more accurate: VMD domains support child devices with MSI and
> > > MSI-x interrupts. The VMD device can't even tell the difference which
> > > one the device is using. It just manipulates messages sent to the usual
> > > APIC address 0xfeeXXXXX.
> > 
> > Thanks, Keith!  I updated the commit log like this:
> > 
> >   d7d8ab87e3e7 ("PCI: vmd: Switch to msi_create_parent_irq_domain()") added a
> >   WARN_ON sanity check that child devices support MSI-X, because VMD document
> >   says [1]:
> > 
> >     Intel VMD only supports MSIx Interrupts from child devices and therefore
> >     the BIOS must enable PCIe Hot Plug and MSIx interrups [sic].
> > 
> >   However, the VMD device can't even tell the difference between a child
> >   device using MSI and one using MSI-X.  Per 185a383ada2e ("x86/PCI: Add
> >   driver for Intel Volume Management Device (VMD)"), VMD does not support
> >   INTx interrupts, but does support child devices using either MSI or MSI-X.
> > 
> >   Remove the sanity check to avoid the unnecessary WARN_ON reported by Ammar.
> 
> Minor correction, it is not just an unnecessary WARN_ON, but child devices'
> drivers couldn't enable MSI at all.
> 
> So perhaps something like "Remove the sanity check to allow child devices
> which only support MSI".

Thanks, updated.

On Tue, Aug 12, 2025 at 02:00:36PM -0500, Bjorn Helgaas wrote:
> On Tue, Aug 12, 2025 at 08:22:09PM +0200, Nam Cao wrote:
> > Minor correction, it is not just an unnecessary WARN_ON, but child devices'
> > drivers couldn't enable MSI at all.
> > 
> > So perhaps something like "Remove the sanity check to allow child devices
> > which only support MSI".
> 
> Thanks, updated.

Sorry for the extra work I have been putting on you this cycle. I trust you
understand it is for good reason.

It is unfortunate that I do not have hardware for most patches I sent. The
best I could do was staring at the patches until I think "yeah, that
probably isn't broken". That obviously isn't good enough to remove all
bugs. I wish that I could do better, but oh well...

Thanks Ammar as well, for helping me tracking down this bug.

I hope this one is the last regression report that we will see.

Nam

On Tue, Aug 12, 2025 at 09:32:19PM +0200, Nam Cao wrote:
> On Tue, Aug 12, 2025 at 02:00:36PM -0500, Bjorn Helgaas wrote:
> > On Tue, Aug 12, 2025 at 08:22:09PM +0200, Nam Cao wrote:
> > > Minor correction, it is not just an unnecessary WARN_ON, but child devices'
> > > drivers couldn't enable MSI at all.
> > > 
> > > So perhaps something like "Remove the sanity check to allow child devices
> > > which only support MSI".
> > 
> > Thanks, updated.
> 
> Sorry for the extra work I have been putting on you this cycle. I
> trust you understand it is for good reason.

No worries, it's the nature of the beast :)  I appreciate all your
work!

On Tue, Aug 12, 2025 at 09:32:19PM +0200, Nam Cao wrote:
> On Tue, Aug 12, 2025 at 02:00:36PM -0500, Bjorn Helgaas wrote:
> > On Tue, Aug 12, 2025 at 08:22:09PM +0200, Nam Cao wrote:
> > > Minor correction, it is not just an unnecessary WARN_ON, but child devices'
> > > drivers couldn't enable MSI at all.
> > > 
> > > So perhaps something like "Remove the sanity check to allow child devices
> > > which only support MSI".
> > 
> > Thanks, updated.
> 
> Sorry for the extra work I have been putting on you this cycle. I trust you
> understand it is for good reason.
> 
> It is unfortunate that I do not have hardware for most patches I sent. The
> best I could do was staring at the patches until I think "yeah, that
> probably isn't broken". That obviously isn't good enough to remove all
> bugs. I wish that I could do better, but oh well...
> 
> Thanks Ammar as well, for helping me tracking down this bug.
> 
> I hope this one is the last regression report that we will see.

I'm sure intel can get you a proper hardware spec. Referencing marketing
white paper material to guide software changes is not a good idea. :)

On Mon, Aug 11, 2025 at 07:39:35AM +0200, Nam Cao wrote:
> Commit d7d8ab87e3e7 ("PCI: vmd: Switch to msi_create_parent_irq_domain()")
> added a WARN_ON sanity check that child devices support MSI-X, because VMD
> document says [1]:
> 
>     "Intel VMD only supports MSIx Interrupts from child devices and
>     therefore the BIOS must enable PCIe Hot Plug and MSIx interrups [sic]."
> 
> However, on Ammar's machine, a PCIe port below VMD does not support MSI-X,
> triggering this WARN_ON.
> 
> This inconsistency between the document and reality should be investigated
> further. For now, remove the MSI-X check.

Posting the processor model in question here for Intel VMD team. It may
help the investigation. The processor model I use is i7-1165G7.

Let me know if you guys need more information from me.

-- 
Ammar Faizi