PCI: vmd: Disable MSI remapping after suspend

From: Nirmal Patel <nirmal.patel@linux.intel.com>

MSI remapping is disbaled by VMD driver for intel's icelake and
newer systems in order to improve performance by setting MSI_RMP_DIS
bit. By design MSI_RMP_DIS bit of VMCONFIG registers is cleared.
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.

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(+)

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)

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

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

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

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