On 11/7/2022 12:19 PM, Bjorn Helgaas wrote:
> On Mon, Nov 07, 2022 at 10:27:35AM -0800, Francisco@xxxxxxxxxxxxxxx wrote:
>> From: Nirmal Patel <nirmal.patel@xxxxxxxxxxxxxxx>
>>
>> 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@xxxxxxxxxxxxxxx>
>> Reviewed-by: Francisco Munoz <francisco.munoz.ruiz@xxxxxxxxxxxxxxx>
>> ---
>> 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
>>
