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

["Patel, Nirmal" <nirmal.patel@linux.intel.com>]

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.
>>