Re: [PATCH] PCI: vmd: Disable MSI remapping after suspend

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

On 11/7/2022 12:19 PM, Bjorn Helgaas wrote:
> On Mon, Nov 07, 2022 at 10:27:35AM -0800, Francisco@vger.kernel.org wrote:
>> From: Nirmal Patel <nirmal.patel@linux.intel.com>
>>
>> MSI remapping is disbaled by VMD driver for intel's icelake and
> disabled
> Intel's
> Ice Lake (at least per intel.com)
ACK
>
>> newer systems in order to improve performance by setting MSI_RMP_DIS
>> bit. By design MSI_RMP_DIS bit of VMCONFIG registers is cleared.
> register is
>
> "MSI_RMP_DIS" doesn't appear in the kernel source.  Is it the same as
> VMCONFIG_MSI_REMAP?

Yes. Using VMCONFIG_MSI_REMAP makes more sense here.

>
>> The same register gets cleared when system is put in S3 power state.
>> VMD driver needs to set this register again in order to avoid
>> interrupt issues with devices behind VMD if MSI remapping was
>> disabled before.
> Should there be a Fixes: tag here?  I guess the failure symptom is
> that a driver doesn't see interrupts it expects?
>
>> Signed-off-by: Nirmal Patel <nirmal.patel@linux.intel.com>
>> Reviewed-by: Francisco Munoz <francisco.munoz.ruiz@linux.intel.com>
>> ---
>>  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 e06e9f4fc50f..247012b343fd 100644
>> --- a/drivers/pci/controller/vmd.c
>> +++ b/drivers/pci/controller/vmd.c
>> @@ -980,6 +980,9 @@ static int vmd_resume(struct device *dev)
>>  	struct vmd_dev *vmd = pci_get_drvdata(pdev);
>>  	int err, i;
>>  
>> +	if (!vmd->irq_domain)
>> +		vmd_set_msi_remapping(vmd, false);
> I suppose this is functionally equivalent to this code in
> vmd_enable_domain():
>
>         if (!(features & VMD_FEAT_CAN_BYPASS_MSI_REMAP) ||
>             offset[0] || offset[1]) {
>                 ret = vmd_create_irq_domain(vmd);
>         } else {
>                 vmd_set_msi_remapping(vmd, false);
>
> because vmd->irq_domain will be NULL if we disabled MSI remapping at
> probe-time.
>
> Maybe the vmd_enable_domain() code could be rearranged a little bit to
> make it obvious that we're doing the same thing both at probe-time and
> resume-time?
>
> Should the vmd_resume() code *enable* MSI remapping in the other case,
> e.g.,
>
>   if (vmd->irq_domain)
>     vmd_set_msi_remapping(vmd, true);
>   else
>     vmd_set_msi_remapping(vmd, false);
>
> I don't know what clears PCI_REG_VMCONFIG (which enabled MSI remapping
> IIUC).  If it's cleared by firmware, the current patch depends on that
> firmware behavior, so maybe it would be good to remove that
> dependency?
Good suggestion. I will make changes for both enabling and disabling MSI
remapping. That will help avoid dependency from firmware. Thanks.
>
>>  	for (i = 0; i < vmd->msix_count; i++) {
>>  		err = devm_request_irq(dev, vmd->irqs[i].virq,
>>  				       vmd_irq, IRQF_NO_THREAD,
>> -- 
>> 2.34.1
>>