Re: [PATCH] PCI: vmd: Check EIME mode before MSI remapping

Re: [PATCH] PCI: vmd: Check EIME mode before MSI remapping

[Patel, Nirmal]

On 2/4/2022 1:40 AM, Nirmal Patel wrote:
> We are observing DMAR errors from vt-d when vmd is enabled along with
> interrupt remapping and extended interrupt mode. As a result the host
> machine is not able boot successfully.
>
> DMAR: DRHD: handling fault status reg 2
> DMAR: [INTR-REMAP] Request device [0xc9:0x05.0] fault index 0xa00
> [fault reason 0x25] Blocked a compatibility format interrupt request
>
> The issue was observed in intel Whitley platform and newer with ICE
> Lake processor with latest kernel. The issued was also reproduced in
> 5.10 by backporting patches related to commit ee81ee84f873 ("PCI: vmd:
> Disable MSI-X remapping when possible")
>
> According to Intel VT-d specs section "5.1.4 Interrupt-Remapping
> Hardware Operation", If Extended Interrupt Mode is enabled (EIME), or
> if the Compatibility format interrupts are disabled (CFIS), the
> Compatibility format interrupts are blocked.
>
> Do not disable MSI remapping if interrupt remapping enabled and
> x2apic_opt_out mode is disabled.
>
> Signed-off-by: Nirmal Patel <nirmal.patel@linux.intel.com>
> ---
>  drivers/pci/controller/vmd.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
> index cc166c683638..4eb38c6bd578 100644
> --- a/drivers/pci/controller/vmd.c
> +++ b/drivers/pci/controller/vmd.c
> @@ -17,6 +17,7 @@
>  #include <linux/srcu.h>
>  #include <linux/rculist.h>
>  #include <linux/rcupdate.h>
> +#include <asm/apic.h>
>  
>  #include <asm/irqdomain.h>
>  
> @@ -814,7 +815,7 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
>  	 * 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) ||
> +	    x2apic_enabled() || !(features & VMD_FEAT_CAN_BYPASS_MSI_REMAP) ||
>  	    offset[0] || offset[1]) {
>  		ret = vmd_alloc_irqs(vmd);
>  		if (ret)

Hello,

Gentle ping. Please let me know if there is a suggestion.

Thanks.

Re: [PATCH] PCI: vmd: Check EIME mode before MSI remapping

[Jonathan Derrick]

On 2/9/2022 1:56 PM, Patel, Nirmal wrote:
> On 2/4/2022 1:40 AM, Nirmal Patel wrote:
>> We are observing DMAR errors from vt-d when vmd is enabled along with
>> interrupt remapping and extended interrupt mode. As a result the host
>> machine is not able boot successfully.
>>
>> DMAR: DRHD: handling fault status reg 2
>> DMAR: [INTR-REMAP] Request device [0xc9:0x05.0] fault index 0xa00
>> [fault reason 0x25] Blocked a compatibility format interrupt request
>>
>> The issue was observed in intel Whitley platform and newer with ICE
Capitalize 'Intel'


>> Lake processor with latest kernel. The issued was also reproduced in
>> 5.10 by backporting patches related to commit ee81ee84f873 ("PCI: vmd:
>> Disable MSI-X remapping when possible")
>>
>> According to Intel VT-d specs section "5.1.4 Interrupt-Remapping
>> Hardware Operation", If Extended Interrupt Mode is enabled (EIME), or
>> if the Compatibility format interrupts are disabled (CFIS), the
>> Compatibility format interrupts are blocked.
>>
>> Do not disable MSI remapping if interrupt remapping enabled and
>> x2apic_opt_out mode is disabled.
This doesn't really satisfy the explanation as to why this mode is not
working. The compatibility format interrupt requests are correctly blocked,
however they should not be being programmed with compatibility format
interrupt requests in the first place. The interrupt request programming
should be done according to the Intel IRQ remapping formats and this should
be VMD-compatible (or at least, it was at one point). Though I don't know
how EIME differs from what I observed before...

If this patch is just a placeholder until the issue can be investigated
and fixed, can you please say that in the commit message?


>>
>> Signed-off-by: Nirmal Patel <nirmal.patel@linux.intel.com>
>> ---
>>   drivers/pci/controller/vmd.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
>> index cc166c683638..4eb38c6bd578 100644
>> --- a/drivers/pci/controller/vmd.c
>> +++ b/drivers/pci/controller/vmd.c
>> @@ -17,6 +17,7 @@
>>   #include <linux/srcu.h>
>>   #include <linux/rculist.h>
>>   #include <linux/rcupdate.h>
>> +#include <asm/apic.h>
>>   
>>   #include <asm/irqdomain.h>
>>   
>> @@ -814,7 +815,7 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
>>   	 * 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) ||
>> +	    x2apic_enabled() || !(features & VMD_FEAT_CAN_BYPASS_MSI_REMAP) ||
>>   	    offset[0] || offset[1]) {
This conditional is getting a little unsightly and over-encompassing.
Can you split it up into multiple conditionals and add appropriate comments?


>>   		ret = vmd_alloc_irqs(vmd);
>>   		if (ret)
> 
> Hello,
> 
> Gentle ping. Please let me know if there is a suggestion.
> 
> Thanks.
> 

Re: [PATCH] PCI: vmd: Check EIME mode before MSI remapping

[Patel, Nirmal]

On 2/10/2022 10:40 AM, Jonathan Derrick wrote:
>
>
> On 2/9/2022 1:56 PM, Patel, Nirmal wrote:
>> On 2/4/2022 1:40 AM, Nirmal Patel wrote:
>>> We are observing DMAR errors from vt-d when vmd is enabled along with
>>> interrupt remapping and extended interrupt mode. As a result the host
>>> machine is not able boot successfully.
>>>
>>> DMAR: DRHD: handling fault status reg 2
>>> DMAR: [INTR-REMAP] Request device [0xc9:0x05.0] fault index 0xa00
>>> [fault reason 0x25] Blocked a compatibility format interrupt request
>>>
>>> The issue was observed in intel Whitley platform and newer with ICE
> Capitalize 'Intel'
Sure.
>
>
>>> Lake processor with latest kernel. The issued was also reproduced in
>>> 5.10 by backporting patches related to commit ee81ee84f873 ("PCI: vmd:
>>> Disable MSI-X remapping when possible")
>>>
>>> According to Intel VT-d specs section "5.1.4 Interrupt-Remapping
>>> Hardware Operation", If Extended Interrupt Mode is enabled (EIME), or
>>> if the Compatibility format interrupts are disabled (CFIS), the
>>> Compatibility format interrupts are blocked.
>>>
>>> Do not disable MSI remapping if interrupt remapping enabled and
>>> x2apic_opt_out mode is disabled.
> This doesn't really satisfy the explanation as to why this mode is not
> working. The compatibility format interrupt requests are correctly blocked,
> however they should not be being programmed with compatibility format
> interrupt requests in the first place. The interrupt request programming
> should be done according to the Intel IRQ remapping formats and this should
> be VMD-compatible (or at least, it was at one point). Though I don't know
> how EIME differs from what I observed before...
>
> If this patch is just a placeholder until the issue can be investigated
> and fixed, can you please say that in the commit message?
>
>
This patch is indeed a placeholder which allow platforms to boot normally.
I will try to figure out how I can keep the interrupts in remappable format
instead of compatibility format.

>>>
>>> Signed-off-by: Nirmal Patel <nirmal.patel@linux.intel.com>
>>> ---
>>>   drivers/pci/controller/vmd.c | 3 ++-
>>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
>>> index cc166c683638..4eb38c6bd578 100644
>>> --- a/drivers/pci/controller/vmd.c
>>> +++ b/drivers/pci/controller/vmd.c
>>> @@ -17,6 +17,7 @@
>>>   #include <linux/srcu.h>
>>>   #include <linux/rculist.h>
>>>   #include <linux/rcupdate.h>
>>> +#include <asm/apic.h>
>>>     #include <asm/irqdomain.h>
>>>   @@ -814,7 +815,7 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
>>>        * 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) ||
>>> +        x2apic_enabled() || !(features & VMD_FEAT_CAN_BYPASS_MSI_REMAP) ||
>>>           offset[0] || offset[1]) {
> This conditional is getting a little unsightly and over-encompassing.
> Can you split it up into multiple conditionals and add appropriate comments?
>
Sure. I will add corrections.
>
>>>           ret = vmd_alloc_irqs(vmd);
>>>           if (ret)
>>
>> Hello,
>>
>> Gentle ping. Please let me know if there is a suggestion.
>>
>> Thanks.
>>