All of lore.kernel.org
 help / color / mirror / Atom feed
* Re: [PATCH v2 1/2] PCI: vmd: Assign VMD IRQ domain before enumeration
       [not found] <20220316155103.8415-1-nirmal.patel@intel.com>
@ 2022-03-29 22:47 ` Patel, Nirmal
  2022-03-29 23:27   ` Dan Williams
       [not found] ` <20220316155103.8415-2-nirmal.patel@intel.com>
  1 sibling, 1 reply; 10+ messages in thread
From: Patel, Nirmal @ 2022-03-29 22:47 UTC (permalink / raw)
  To: Lorenzo Pieralisi; +Cc: Dan Williams, linux-pci, Jon Derrick, Nirmal Patel

On 3/16/2022 8:51 AM, Nirmal Patel wrote:
> From: Nirmal Patel <nirmal.patel@linux.intel.com>
>
> VMD creates and assigns a 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 by
> intel-iommu all the PCI devices are assigned INTEL-IR-MSI domain
> including VMD endpoints. But devices behind VMD get PCI-MSI IRQ domain
> when VMD create a 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
>
> Acked-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Nirmal Patel <nirmal.patel@linux.intel.com>
> ---
>  drivers/pci/controller/vmd.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
> index cc166c683638..3a6570e5b765 100644
> --- a/drivers/pci/controller/vmd.c
> +++ b/drivers/pci/controller/vmd.c
> @@ -853,6 +853,8 @@ 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, dev_get_msi_domain(&vmd->dev->dev));
>  
>  	vmd_acpi_begin();
>  

Gentle ping!

Thanks
nirmal


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 2/2] Allow VMD to disable MSIX remapping with interrupt remapping enabled.
       [not found] ` <20220316155103.8415-2-nirmal.patel@intel.com>
@ 2022-03-29 22:48   ` Patel, Nirmal
  2022-03-30 17:49     ` Bjorn Helgaas
  0 siblings, 1 reply; 10+ messages in thread
From: Patel, Nirmal @ 2022-03-29 22:48 UTC (permalink / raw)
  To: Lorenzo Pieralisi; +Cc: linux-pci, Nirmal Patel

On 3/16/2022 8:51 AM, Nirmal Patel wrote:
> This patch removes a placeholder patch 2565e5b69c44 ("PCI: vmd: Do
> not disable MSI-X remapping if interrupt remapping is enabled by IOMMU.")
> This patch was added as a workaround to disable MSI remapping if iommu
> enables interrupt remapping. VMD does not assign proper IRQ domain to
> child devices when MSIX is disabled. There is no dependency between MSI
> remapping by VMD and interrupt remapping by iommu. MSI remapping can be
> enabled or disabled with and without interrupt remap.
>
> Signed-off-by: Nirmal Patel <nirmal.patel@intel.com>
> ---
>  drivers/pci/controller/vmd.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
> index 3a6570e5b765..91bc1b40d40c 100644
> --- a/drivers/pci/controller/vmd.c
> +++ b/drivers/pci/controller/vmd.c
> @@ -6,7 +6,6 @@
>  
>  #include <linux/device.h>
>  #include <linux/interrupt.h>
> -#include <linux/iommu.h>
>  #include <linux/irq.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
> @@ -813,8 +812,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)

Gentle ping!

Thanks
nirmal


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 1/2] PCI: vmd: Assign VMD IRQ domain before enumeration
  2022-03-29 22:47 ` [PATCH v2 1/2] PCI: vmd: Assign VMD IRQ domain before enumeration Patel, Nirmal
@ 2022-03-29 23:27   ` Dan Williams
  2022-03-30 15:54     ` Patel, Nirmal
  0 siblings, 1 reply; 10+ messages in thread
From: Dan Williams @ 2022-03-29 23:27 UTC (permalink / raw)
  To: Patel, Nirmal; +Cc: Lorenzo Pieralisi, Linux PCI, Jon Derrick, Nirmal Patel

On Tue, Mar 29, 2022 at 3:48 PM Patel, Nirmal
<nirmal.patel@linux.intel.com> wrote:
>
> On 3/16/2022 8:51 AM, Nirmal Patel wrote:
> > From: Nirmal Patel <nirmal.patel@linux.intel.com>
> >
> > VMD creates and assigns a 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 by
> > intel-iommu all the PCI devices are assigned INTEL-IR-MSI domain
> > including VMD endpoints. But devices behind VMD get PCI-MSI IRQ domain
> > when VMD create a 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
> >
> > Acked-by: Dan Williams <dan.j.williams@intel.com>
> > Signed-off-by: Nirmal Patel <nirmal.patel@linux.intel.com>
> > ---
> >  drivers/pci/controller/vmd.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
> > index cc166c683638..3a6570e5b765 100644
> > --- a/drivers/pci/controller/vmd.c
> > +++ b/drivers/pci/controller/vmd.c
> > @@ -853,6 +853,8 @@ 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, dev_get_msi_domain(&vmd->dev->dev));
> >
> >       vmd_acpi_begin();
> >
>
> Gentle ping!

It helps to be explicit when you send a patch and a follow-up ping.
Are you asking Lorenzo to take this? Is this urgent such that Bjorn
should consider taking it directly? The changelog notes what happens,
but not the severity of end user visible impact. The merge window is
presently open so the natural inclination is to just wait until that
closes to circle back to outstanding patches.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 1/2] PCI: vmd: Assign VMD IRQ domain before enumeration
  2022-03-29 23:27   ` Dan Williams
@ 2022-03-30 15:54     ` Patel, Nirmal
  2022-03-30 17:54       ` Bjorn Helgaas
  0 siblings, 1 reply; 10+ messages in thread
From: Patel, Nirmal @ 2022-03-30 15:54 UTC (permalink / raw)
  To: Dan Williams; +Cc: Lorenzo Pieralisi, Linux PCI, Jon Derrick, Nirmal Patel

On 3/29/2022 4:27 PM, Dan Williams wrote:
> On Tue, Mar 29, 2022 at 3:48 PM Patel, Nirmal
> <nirmal.patel@linux.intel.com> wrote:
>> On 3/16/2022 8:51 AM, Nirmal Patel wrote:
>>> From: Nirmal Patel <nirmal.patel@linux.intel.com>
>>>
>>> VMD creates and assigns a 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 by
>>> intel-iommu all the PCI devices are assigned INTEL-IR-MSI domain
>>> including VMD endpoints. But devices behind VMD get PCI-MSI IRQ domain
>>> when VMD create a 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
>>>
>>> Acked-by: Dan Williams <dan.j.williams@intel.com>
>>> Signed-off-by: Nirmal Patel <nirmal.patel@linux.intel.com>
>>> ---
>>>  drivers/pci/controller/vmd.c | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
>>> index cc166c683638..3a6570e5b765 100644
>>> --- a/drivers/pci/controller/vmd.c
>>> +++ b/drivers/pci/controller/vmd.c
>>> @@ -853,6 +853,8 @@ 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, dev_get_msi_domain(&vmd->dev->dev));
>>>
>>>       vmd_acpi_begin();
>>>
>> Gentle ping!
> It helps to be explicit when you send a patch and a follow-up ping.
> Are you asking Lorenzo to take this? Is this urgent such that Bjorn
> should consider taking it directly? The changelog notes what happens,
> but not the severity of end user visible impact. The merge window is
> presently open so the natural inclination is to just wait until that
> closes to circle back to outstanding patches.

This patch removes a flag that bypasses MSI disable feature of VMD and
improves the performance. So it would be nice if the patch gets accepted
sooner. I tend to send follow-up ping after a week or so if I do not get any
feedback and to allow it to get accepted in time.

Thanks.


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 2/2] Allow VMD to disable MSIX remapping with interrupt remapping enabled.
  2022-03-29 22:48   ` [PATCH v2 2/2] Allow VMD to disable MSIX remapping with interrupt remapping enabled Patel, Nirmal
@ 2022-03-30 17:49     ` Bjorn Helgaas
  2022-03-30 18:07       ` Patel, Nirmal
  0 siblings, 1 reply; 10+ messages in thread
From: Bjorn Helgaas @ 2022-03-30 17:49 UTC (permalink / raw)
  To: Patel, Nirmal; +Cc: Lorenzo Pieralisi, linux-pci, Nirmal Patel

On Tue, Mar 29, 2022 at 03:48:21PM -0700, Patel, Nirmal wrote:
> On 3/16/2022 8:51 AM, Nirmal Patel wrote:
> > This patch removes a placeholder patch 2565e5b69c44 ("PCI: vmd: Do
> > not disable MSI-X remapping if interrupt remapping is enabled by IOMMU.")
> > This patch was added as a workaround to disable MSI remapping if iommu
> > enables interrupt remapping. VMD does not assign proper IRQ domain to
> > child devices when MSIX is disabled. There is no dependency between MSI
> > remapping by VMD and interrupt remapping by iommu. MSI remapping can be
> > enabled or disabled with and without interrupt remap.
> >
> > Signed-off-by: Nirmal Patel <nirmal.patel@intel.com>
> > ---
> >  drivers/pci/controller/vmd.c | 4 +---
> >  1 file changed, 1 insertion(+), 3 deletions(-)
> >
> > diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
> > index 3a6570e5b765..91bc1b40d40c 100644
> > --- a/drivers/pci/controller/vmd.c
> > +++ b/drivers/pci/controller/vmd.c
> > @@ -6,7 +6,6 @@
> >  
> >  #include <linux/device.h>
> >  #include <linux/interrupt.h>
> > -#include <linux/iommu.h>
> >  #include <linux/irq.h>
> >  #include <linux/kernel.h>
> >  #include <linux/module.h>
> > @@ -813,8 +812,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)

If/when you repost this, please update the subject and commit log to
use "MSI-X" consistently instead of the current mix of "MSI-X" and
"MSIX".

Also s/iommu/IOMMU/.

In subject line, add "PCI: vmd: " prefix and drop trailing period.

Also rewrite commit log in imperative mood, e.g., "Revert 2565e5b69c44
..." instead of "This patch removes ..."  It's 100% clear that the
commit log refers to *this* patch, so it's pointless to include that.

It's further confusing that "This patch was added ..." refers to
*2565e5b69c44*, not this revert.

This reverts 2565e5b69c44 (but doesn't remove the #include
<linux/iommu.h>" added by 2565e5b69c44).

2565e5b69c44 fixed a problem.  If that fix is no longer necessary
because of some other change, the commit log should mention that
change.  Otherwise somebody will backport this fix too far and
reintroduce the problem solved by 2565e5b69c44.

Bjorn

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 1/2] PCI: vmd: Assign VMD IRQ domain before enumeration
  2022-03-30 15:54     ` Patel, Nirmal
@ 2022-03-30 17:54       ` Bjorn Helgaas
  2022-03-30 18:06         ` Patel, Nirmal
  0 siblings, 1 reply; 10+ messages in thread
From: Bjorn Helgaas @ 2022-03-30 17:54 UTC (permalink / raw)
  To: Patel, Nirmal
  Cc: Dan Williams, Lorenzo Pieralisi, Linux PCI, Jon Derrick, Nirmal Patel

On Wed, Mar 30, 2022 at 08:54:15AM -0700, Patel, Nirmal wrote:
> On 3/29/2022 4:27 PM, Dan Williams wrote:
> > On Tue, Mar 29, 2022 at 3:48 PM Patel, Nirmal
> > <nirmal.patel@linux.intel.com> wrote:
> >> On 3/16/2022 8:51 AM, Nirmal Patel wrote:
> >>> From: Nirmal Patel <nirmal.patel@linux.intel.com>
> >>>
> >>> VMD creates and assigns a 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 by
> >>> intel-iommu all the PCI devices are assigned INTEL-IR-MSI domain
> >>> including VMD endpoints. But devices behind VMD get PCI-MSI IRQ domain
> >>> when VMD create a 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
> >>>
> >>> Acked-by: Dan Williams <dan.j.williams@intel.com>
> >>> Signed-off-by: Nirmal Patel <nirmal.patel@linux.intel.com>
> >>> ---
> >>>  drivers/pci/controller/vmd.c | 2 ++
> >>>  1 file changed, 2 insertions(+)
> >>>
> >>> diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
> >>> index cc166c683638..3a6570e5b765 100644
> >>> --- a/drivers/pci/controller/vmd.c
> >>> +++ b/drivers/pci/controller/vmd.c
> >>> @@ -853,6 +853,8 @@ 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, dev_get_msi_domain(&vmd->dev->dev));
> >>>
> >>>       vmd_acpi_begin();
> >>>
> >> Gentle ping!
> > It helps to be explicit when you send a patch and a follow-up ping.
> > Are you asking Lorenzo to take this? Is this urgent such that Bjorn
> > should consider taking it directly? The changelog notes what happens,
> > but not the severity of end user visible impact. The merge window is
> > presently open so the natural inclination is to just wait until that
> > closes to circle back to outstanding patches.
> 
> This patch removes a flag that bypasses MSI disable feature of VMD and
> improves the performance. So it would be nice if the patch gets accepted
> sooner. I tend to send follow-up ping after a week or so if I do not get any
> feedback and to allow it to get accepted in time.

There are only a few days left in the v5.18 merge window, so unless
it's an emergency, this would be v5.19 material.

This claims to be a v2, but I missed the v1, and the lore archives [1]
seem incomplete.  Maybe the v1 (and maybe the cover letter?) were HTML
or got lost for some other reason?

Bjorn

[1] https://lore.kernel.org/all/?q=f%3Anirmal.patel

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 1/2] PCI: vmd: Assign VMD IRQ domain before enumeration
  2022-03-30 17:54       ` Bjorn Helgaas
@ 2022-03-30 18:06         ` Patel, Nirmal
  2022-03-30 18:20           ` Bjorn Helgaas
  0 siblings, 1 reply; 10+ messages in thread
From: Patel, Nirmal @ 2022-03-30 18:06 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Dan Williams, Lorenzo Pieralisi, Linux PCI, Jon Derrick, Nirmal Patel

On 3/30/2022 10:54 AM, Bjorn Helgaas wrote:
> On Wed, Mar 30, 2022 at 08:54:15AM -0700, Patel, Nirmal wrote:
>> On 3/29/2022 4:27 PM, Dan Williams wrote:
>>> On Tue, Mar 29, 2022 at 3:48 PM Patel, Nirmal
>>> <nirmal.patel@linux.intel.com> wrote:
>>>> On 3/16/2022 8:51 AM, Nirmal Patel wrote:
>>>>> From: Nirmal Patel <nirmal.patel@linux.intel.com>
>>>>>
>>>>> VMD creates and assigns a 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 by
>>>>> intel-iommu all the PCI devices are assigned INTEL-IR-MSI domain
>>>>> including VMD endpoints. But devices behind VMD get PCI-MSI IRQ domain
>>>>> when VMD create a 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
>>>>>
>>>>> Acked-by: Dan Williams <dan.j.williams@intel.com>
>>>>> Signed-off-by: Nirmal Patel <nirmal.patel@linux.intel.com>
>>>>> ---
>>>>>  drivers/pci/controller/vmd.c | 2 ++
>>>>>  1 file changed, 2 insertions(+)
>>>>>
>>>>> diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
>>>>> index cc166c683638..3a6570e5b765 100644
>>>>> --- a/drivers/pci/controller/vmd.c
>>>>> +++ b/drivers/pci/controller/vmd.c
>>>>> @@ -853,6 +853,8 @@ 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, dev_get_msi_domain(&vmd->dev->dev));
>>>>>
>>>>>       vmd_acpi_begin();
>>>>>
>>>> Gentle ping!
>>> It helps to be explicit when you send a patch and a follow-up ping.
>>> Are you asking Lorenzo to take this? Is this urgent such that Bjorn
>>> should consider taking it directly? The changelog notes what happens,
>>> but not the severity of end user visible impact. The merge window is
>>> presently open so the natural inclination is to just wait until that
>>> closes to circle back to outstanding patches.
>> This patch removes a flag that bypasses MSI disable feature of VMD and
>> improves the performance. So it would be nice if the patch gets accepted
>> sooner. I tend to send follow-up ping after a week or so if I do not get any
>> feedback and to allow it to get accepted in time.
> There are only a few days left in the v5.18 merge window, so unless
> it's an emergency, this would be v5.19 material.
>
> This claims to be a v2, but I missed the v1, and the lore archives [1]
> seem incomplete.  Maybe the v1 (and maybe the cover letter?) were HTML
> or got lost for some other reason?
>
> Bjorn
>
> [1] https://lore.kernel.org/all/?q=f%3Anirmal.patel

Initially I created one patch [1] and I was advised to create two separate patches.

[1] https://lore.kernel.org/all/358b0673-f90f-78ca-be66-51d5f76cc42b@linux.intel.com/


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 2/2] Allow VMD to disable MSIX remapping with interrupt remapping enabled.
  2022-03-30 17:49     ` Bjorn Helgaas
@ 2022-03-30 18:07       ` Patel, Nirmal
  0 siblings, 0 replies; 10+ messages in thread
From: Patel, Nirmal @ 2022-03-30 18:07 UTC (permalink / raw)
  To: Bjorn Helgaas; +Cc: Lorenzo Pieralisi, linux-pci, Nirmal Patel

On 3/30/2022 10:49 AM, Bjorn Helgaas wrote:
> On Tue, Mar 29, 2022 at 03:48:21PM -0700, Patel, Nirmal wrote:
>> On 3/16/2022 8:51 AM, Nirmal Patel wrote:
>>> This patch removes a placeholder patch 2565e5b69c44 ("PCI: vmd: Do
>>> not disable MSI-X remapping if interrupt remapping is enabled by IOMMU.")
>>> This patch was added as a workaround to disable MSI remapping if iommu
>>> enables interrupt remapping. VMD does not assign proper IRQ domain to
>>> child devices when MSIX is disabled. There is no dependency between MSI
>>> remapping by VMD and interrupt remapping by iommu. MSI remapping can be
>>> enabled or disabled with and without interrupt remap.
>>>
>>> Signed-off-by: Nirmal Patel <nirmal.patel@intel.com>
>>> ---
>>>  drivers/pci/controller/vmd.c | 4 +---
>>>  1 file changed, 1 insertion(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
>>> index 3a6570e5b765..91bc1b40d40c 100644
>>> --- a/drivers/pci/controller/vmd.c
>>> +++ b/drivers/pci/controller/vmd.c
>>> @@ -6,7 +6,6 @@
>>>  
>>>  #include <linux/device.h>
>>>  #include <linux/interrupt.h>
>>> -#include <linux/iommu.h>
>>>  #include <linux/irq.h>
>>>  #include <linux/kernel.h>
>>>  #include <linux/module.h>
>>> @@ -813,8 +812,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)
> If/when you repost this, please update the subject and commit log to
> use "MSI-X" consistently instead of the current mix of "MSI-X" and
> "MSIX".
>
> Also s/iommu/IOMMU/.
>
> In subject line, add "PCI: vmd: " prefix and drop trailing period.
>
> Also rewrite commit log in imperative mood, e.g., "Revert 2565e5b69c44
> ..." instead of "This patch removes ..."  It's 100% clear that the
> commit log refers to *this* patch, so it's pointless to include that.
>
> It's further confusing that "This patch was added ..." refers to
> *2565e5b69c44*, not this revert.
>
> This reverts 2565e5b69c44 (but doesn't remove the #include
> <linux/iommu.h>" added by 2565e5b69c44).
>
> 2565e5b69c44 fixed a problem.  If that fix is no longer necessary
> because of some other change, the commit log should mention that
> change.  Otherwise somebody will backport this fix too far and
> reintroduce the problem solved by 2565e5b69c44.
>
> Bjorn

I will apply these changes.

Thank you
Nirmal


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 1/2] PCI: vmd: Assign VMD IRQ domain before enumeration
  2022-03-30 18:06         ` Patel, Nirmal
@ 2022-03-30 18:20           ` Bjorn Helgaas
  0 siblings, 0 replies; 10+ messages in thread
From: Bjorn Helgaas @ 2022-03-30 18:20 UTC (permalink / raw)
  To: Patel, Nirmal; +Cc: Dan Williams, Lorenzo Pieralisi, Linux PCI, Jon Derrick

On Wed, Mar 30, 2022 at 11:06:43AM -0700, Patel, Nirmal wrote:
> On 3/30/2022 10:54 AM, Bjorn Helgaas wrote:
> ...

> > This claims to be a v2, but I missed the v1, and the lore archives [1]
> > seem incomplete.  Maybe the v1 (and maybe the cover letter?) were HTML
> > or got lost for some other reason?
> >
> > Bjorn
> >
> > [1] https://lore.kernel.org/all/?q=f%3Anirmal.patel
> 
> Initially I created one patch [1] and I was advised to create two separate patches.
> 
> [1] https://lore.kernel.org/all/358b0673-f90f-78ca-be66-51d5f76cc42b@linux.intel.com/

The point is that
https://lore.kernel.org/all/358b0673-f90f-78ca-be66-51d5f76cc42b@linux.intel.com/
is a reply to something that didn't make it into the archives.

It says:

  In-Reply-To: <20220223075245.17744-1-nirmal.patel@linux.intel.com>

but 20220223075245.17744-1-nirmal.patel@linux.intel.com is unknown to
lore.

You can't fix this now; I'm just pointing it out in case there's some
problem with the way you're sending patches.

Bjorn

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v2 1/2] PCI: vmd: Assign VMD IRQ domain before enumeration
  2022-05-11  9:57 [PATCH v2 0/2] PCI: vmd: IRQ domain assignment to sub devices Nirmal Patel
@ 2022-05-11  9:57 ` Nirmal Patel
  0 siblings, 0 replies; 10+ messages in thread
From: Nirmal Patel @ 2022-05-11  9:57 UTC (permalink / raw)
  To: linux-pci, Nirmal Patel

During the boot process all the PCI devices are assigned default PCI-MSI
IRQ domain including VMD endpoint devices. If interrupt-remapping is
enabled by IOMMU, the PCI devices except VMD get new INTEL-IR-MSI IRQ
domain. And VMD is supposed to create and assign a separate VMD-MSI IRQ
domain for its child devices in order to support MSI-X remapping
capabilities.

Now when MSI-X remapping in VMD is disabled in order to improve
performance, VMD skips VMD-MSI IRQ domain assignment process to its
child devices. Thus the devices behind VMD get default PCI-MSI IRQ
domain instead of INTEL-IR-MSI IRQ domain when VMD creates root bus and
configures child devices.

As a result host OS fails to boot and 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

To fix this issue, dev_msi_info struct in dev struct maintains correct
value of IRQ domain. VMD will use this information to assign proper IRQ
domain to its child devices when it doesn't create a separate IRQ domain.

Signed-off-by: Nirmal Patel <nirmal.patel@linux.intel.com>
---
v1->v2: Adding more information to commit log.
---
 drivers/pci/controller/vmd.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
index eb05cceab964..5015adc04d19 100644
--- a/drivers/pci/controller/vmd.c
+++ b/drivers/pci/controller/vmd.c
@@ -853,6 +853,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,
+				   dev_get_msi_domain(&vmd->dev->dev));
 
 	vmd_acpi_begin();
 
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2022-05-11 17:03 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <20220316155103.8415-1-nirmal.patel@intel.com>
2022-03-29 22:47 ` [PATCH v2 1/2] PCI: vmd: Assign VMD IRQ domain before enumeration Patel, Nirmal
2022-03-29 23:27   ` Dan Williams
2022-03-30 15:54     ` Patel, Nirmal
2022-03-30 17:54       ` Bjorn Helgaas
2022-03-30 18:06         ` Patel, Nirmal
2022-03-30 18:20           ` Bjorn Helgaas
     [not found] ` <20220316155103.8415-2-nirmal.patel@intel.com>
2022-03-29 22:48   ` [PATCH v2 2/2] Allow VMD to disable MSIX remapping with interrupt remapping enabled Patel, Nirmal
2022-03-30 17:49     ` Bjorn Helgaas
2022-03-30 18:07       ` Patel, Nirmal
2022-05-11  9:57 [PATCH v2 0/2] PCI: vmd: IRQ domain assignment to sub devices Nirmal Patel
2022-05-11  9:57 ` [PATCH v2 1/2] PCI: vmd: Assign VMD IRQ domain before enumeration Nirmal Patel

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.