* Re: [PATCH] PCI: vmd: Assign vmd irq domain before enumeration
[not found] <20220223075245.17744-1-nirmal.patel@linux.intel.com>
@ 2022-03-02 17:41 ` Patel, Nirmal
2022-03-02 19:35 ` Jonathan Derrick
0 siblings, 1 reply; 4+ messages in thread
From: Patel, Nirmal @ 2022-03-02 17:41 UTC (permalink / raw)
To: Jonathan Derrick, Lorenzo Pieralisi; +Cc: linux-pci, Nirmal Patel
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.
>
> 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
>
> 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.) MSI remapping
> should be enabled or disabled with and without Interrupt remap.
>
> 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);
> +
> vmd_acpi_begin();
>
> pci_scan_child_bus(vmd->bus);
Gentle ping!
Thanks
nirmal
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] PCI: vmd: Assign vmd irq domain before enumeration
2022-03-02 17:41 ` [PATCH] PCI: vmd: Assign vmd irq domain before enumeration Patel, Nirmal
@ 2022-03-02 19:35 ` Jonathan Derrick
2022-03-02 20:55 ` Bjorn Helgaas
0 siblings, 1 reply; 4+ messages in thread
From: Jonathan Derrick @ 2022-03-02 19:35 UTC (permalink / raw)
To: Patel, Nirmal, Lorenzo Pieralisi; +Cc: linux-pci
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?
>>
>> 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
>>
>> 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.) 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
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] PCI: vmd: Assign vmd irq domain before enumeration
2022-03-02 19:35 ` Jonathan Derrick
@ 2022-03-02 20:55 ` Bjorn Helgaas
2022-03-14 18:13 ` Patel, Nirmal
0 siblings, 1 reply; 4+ messages in thread
From: Bjorn Helgaas @ 2022-03-02 20:55 UTC (permalink / raw)
To: Jonathan Derrick; +Cc: Patel, Nirmal, Lorenzo Pieralisi, linux-pci
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
> >
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] PCI: vmd: Assign vmd irq domain before enumeration
2022-03-02 20:55 ` Bjorn Helgaas
@ 2022-03-14 18:13 ` Patel, Nirmal
0 siblings, 0 replies; 4+ messages in thread
From: Patel, Nirmal @ 2022-03-14 18:13 UTC (permalink / raw)
To: Bjorn Helgaas, Jonathan Derrick; +Cc: Lorenzo Pieralisi, linux-pci
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@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) ?
Good idea.
>>
>>>> +
>>>> vmd_acpi_begin();
>>>> pci_scan_child_bus(vmd->bus);
>>> Gentle ping!
>>>
>>> Thanks
>>>
>>> nirmal
>>>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-03-14 18:13 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20220223075245.17744-1-nirmal.patel@linux.intel.com>
2022-03-02 17:41 ` [PATCH] PCI: vmd: Assign vmd irq domain before enumeration Patel, Nirmal
2022-03-02 19:35 ` Jonathan Derrick
2022-03-02 20:55 ` Bjorn Helgaas
2022-03-14 18:13 ` Patel, Nirmal
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).