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

[Bjorn Helgaas <helgaas@kernel.org>]

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
>