Re: [PATCH] PCI: vmd: Assign vmd irq domain before enumeration

[Jonathan Derrick <jonathan.derrick@linux.dev>]

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
>