Re: [PATCH] PCI: vmd: Assign vmd irq domain before enumeration

[Bjorn Helgaas <helgaas@kernel.org>]

On Wed, Mar 02, 2022 at 12:35:20PM -0700, Jonathan Derrick wrote:
> Hey Nirmal,
> 
> Sorry I didn't catch this earlier.
> 
> On 3/2/2022 10:41 AM, Patel, Nirmal wrote:
> > On 2/23/2022 12:52 AM, Nirmal Patel wrote:
> > > vmd creates and assigns separate irq domain only when MSI remapping is
> > > enabled. For example VMD-MSI. But vmd doesn't assign irq domain when
> > > MSI remapping is disabled resulting child devices getting default
> > > PCI-MSI irq domain. Now when Interrupt remapping is enabled all the
> > > pci devices are assigned INTEL-IR-MSI domain including vmd endpoints.
> > > But devices behind vmd gets PCI-MSI irq domain when vmd creates root
> > > bus and configures child devices.
> can you capitalize VMD for consistency?

And IRQ, while you're at it.  Both occur several times above (and
below).  And PCI.

Also, s/Interrupt remapping/interrupt remapping/ above, since
there's no reason to capitalize "Interrupt" there.

s/behind vmd gets/behind VMD get/

> > > As a result DMAR errors were observed when interrupt remapping was
> > > enabled on Intel Icelake CPUs.
> > > For instance:
> > > DMAR: DRHD: handling fault status reg 2
> > > DMAR: [INTR-REMAP] Request device [0xe2:0x00.0] fault index 0xa00
> > > [fault reason 0x25] Blocked a compatibility format interrupt request

Quote material as in 2565e5b69c44, e.g.,

  As a result DMAR errors were observed ...  For instance:

    DMAR: DRHD: handling fault status reg 2
    DMAR: [INTR-REMAP] Request device [0xe2:0x00.0] fault index 0xa00 [fault reason 0x25] Blocked a compatibility format interrupt request

(Blank line before and after quoted material, indent quote two spaces,
don't break messages over lines, so they're easy to grep for.)

> > > So make sure vmd assigns proper irq domain. This patch also removes
> > > a placeholder patch 2565e5b69c44 (PCI: vmd: Do not disable MSI-X
> > > remapping if interrupt remapping is enabled by IOMMU.)

Usual commit citation style is

  2565e5b69c44 ("PCI: vmd: Do not disable MSI-X remapping if interrupt
  remapping is enabled by IOMMU")

Why is this revert part of this patch?  Could it be a separate patch?
Either way, we should explain why we can now revert it.

> > > MSI remapping
> > > should be enabled or disabled with and without Interrupt remap.
> > > 
> So this keeps the performance path working with remapping?
> Great!
> 
> 
> > > Signed-off-by: Nirmal Patel <nirmal.patel@linux.intel.com>
> > > ---
> > >   drivers/pci/controller/vmd.c | 7 ++++---
> > >   1 file changed, 4 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
> > > index cc166c683638..c8d73d75a1c0 100644
> > > --- a/drivers/pci/controller/vmd.c
> > > +++ b/drivers/pci/controller/vmd.c
> > > @@ -813,8 +813,7 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
> > >   	 * acceptable because the guest is usually CPU-limited and MSI
> > >   	 * remapping doesn't become a performance bottleneck.
> > >   	 */
> > > -	if (iommu_capable(vmd->dev->dev.bus, IOMMU_CAP_INTR_REMAP) ||
> > > -	    !(features & VMD_FEAT_CAN_BYPASS_MSI_REMAP) ||
> > > +	if (!(features & VMD_FEAT_CAN_BYPASS_MSI_REMAP) ||
> > >   	    offset[0] || offset[1]) {
> > >   		ret = vmd_alloc_irqs(vmd);
> > >   		if (ret)
> > > @@ -853,7 +852,9 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
> > >   	vmd_attach_resources(vmd);
> > >   	if (vmd->irq_domain)
> > >   		dev_set_msi_domain(&vmd->bus->dev, vmd->irq_domain);
> > > -
> > > +	else
> > > +		dev_set_msi_domain(&vmd->bus->dev, vmd->dev->dev.msi.domain);
> how about dev_get_msi_domain(vmd->dev) ?
> 
> > > +	
> > >   	vmd_acpi_begin();
> > >   	pci_scan_child_bus(vmd->bus);
> > 
> > Gentle ping!
> > 
> > Thanks
> > 
> > nirmal
> >