[PATCH 4/4][Focal/linux-azure] PCI: hv: Fix interrupt mapping for multi-MSI

[Tim Gardner]

On 7/14/22 03:08, Bartlomiej Zolnierkiewicz wrote:
> Hi Tim,
> 
> On Wed, Jul 13, 2022 at 2:57 PM Tim Gardner <tim.gardner at canonical.com> wrote:
>>
>> From: Jeffrey Hugo <quic_jhugo at quicinc.com>
>>
>> BugLink: https://bugs.launchpad.net/bugs/1981577
>>
>> According to Dexuan, the hypervisor folks beleive that multi-msi
>> allocations are not correct.  compose_msi_msg() will allocate multi-msi
>> one by one.  However, multi-msi is a block of related MSIs, with alignment
>> requirements.  In order for the hypervisor to allocate properly aligned
>> and consecutive entries in the IOMMU Interrupt Remapping Table, there
>> should be a single mapping request that requests all of the multi-msi
>> vectors in one shot.
>>
>> Dexuan suggests detecting the multi-msi case and composing a single
>> request related to the first MSI.  Then for the other MSIs in the same
>> block, use the cached information.  This appears to be viable, so do it.
>>
>> Suggested-by: Dexuan Cui <decui at microsoft.com>
>> Signed-off-by: Jeffrey Hugo <quic_jhugo at quicinc.com>
>> Reviewed-by: Dexuan Cui <decui at microsoft.com>
>> Tested-by: Michael Kelley <mikelley at microsoft.com>
>> Link: https://lore.kernel.org/r/1652282599-21643-1-git-send-email-quic_jhugo@quicinc.com
>> Signed-off-by: Wei Liu <wei.liu at kernel.org>
>> (backported from commit a2bad844a67b1c7740bda63e87453baf63c3a7f7)
>> [rtg - msi_desc attributes are in a different place in the structure ]
>> Signed-off-by: Tim Gardner <tim.gardner at canonical.com>
>> ---
>>   drivers/pci/controller/pci-hyperv.c | 67 ++++++++++++++++++++++++-----
>>   1 file changed, 57 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
>> index 04165db6d385..6ce774de3525 100644
>> --- a/drivers/pci/controller/pci-hyperv.c
>> +++ b/drivers/pci/controller/pci-hyperv.c
>> @@ -1398,6 +1398,10 @@ static void hv_int_desc_free(struct hv_pci_dev *hpdev,
>>                  u8 buffer[sizeof(struct pci_delete_interrupt)];
>>          } ctxt;
>>
>> +       if (!int_desc->vector_count) {
>> +               kfree(int_desc);
>> +               return;
>> +       }
>>          memset(&ctxt, 0, sizeof(ctxt));
>>          int_pkt = (struct pci_delete_interrupt *)&ctxt.pkt.message;
>>          int_pkt->message_type.type =
>> @@ -1491,8 +1495,15 @@ static void hv_irq_unmask(struct irq_data *data)
>>          memset(params, 0, sizeof(*params));
>>          params->partition_id = HV_PARTITION_ID_SELF;
>>          params->int_entry.source = HV_INTERRUPT_SOURCE_MSI;
>> +#ifdef CONFIG_X86
>>          params->int_entry.msi_entry.address.as_uint32 = int_desc->address & 0xffffffff;
>>          params->int_entry.msi_entry.data.as_uint32 = int_desc->data;
>> +#elif CONFIG_ARM64
>> +       params->int_entry.msi_entry.address = int_desc->address & 0xffffffff;
>> +       params->int_entry.msi_entry.data = int_desc->data;
>> +#else
>> +#error Unsupported architecture
>> +#endif
> 
> Shouldn't the above change be included in the patch #1/4 instead?
> 

That block really should have gone into patch 2 in order to enable 
bisecting to work. I'll do that when I apply the patches, but otherwise 
won't change the code.

> [ Even better if we could backport to focal:linux-azure commit
> d06957d7a6929e6a4aa959cb59d66f0c095fc974 ("PCI: hv: Avoid the retarget
> interrupt hypercall in irq_unmask() on ARM64") like it is done for
> jammy:linux-azure. I assume that it was not backported because it is
> more risky for focal as It introduces behavior change for ARM64.. ]
> 

This is at an awkward point in the architecture code consolidation. I 
tried adding d06957d7a6929e6a4aa959cb59d66f0c095fc974 ("PCI: hv: Avoid 
the retarget interrupt hypercall in irq_unmask() on ARM64") but it 
caused more back port issues then it was worth.

This is the code that Microsoft tested, so I'm content to leave it as is.

rtg



-- 
-----------
Tim Gardner
Canonical, Inc