On 3/2/2022 1:55 PM, Bjorn Helgaas wrote: > 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/ Understood. > >>>> 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. I will add some more details. > >>>> 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@xxxxxxxxxxxxxxx> >>>> --- >>>> 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) ? Good idea. >> >>>> + >>>> vmd_acpi_begin(); >>>> pci_scan_child_bus(vmd->bus); >>> Gentle ping! >>> >>> Thanks >>> >>> nirmal >>>