diff mbox series

[09/11,jammy/linux-azure] PCI: hv: Fix hv_arch_irq_unmask() for multi-MSI

Message ID 20220713125649.24063-14-tim.gardner@canonical.com
State New
Headers show
Series Azure: Support multi-MSI | expand

Commit Message

Tim Gardner July 13, 2022, 12:56 p.m. UTC 
From: Jeffrey Hugo <quic_jhugo@quicinc.com>

BugLink: https://bugs.launchpad.net/bugs/1981577

In the multi-MSI case, hv_arch_irq_unmask() will only operate on the first
MSI of the N allocated.  This is because only the first msi_desc is cached
and it is shared by all the MSIs of the multi-MSI block.  This means that
hv_arch_irq_unmask() gets the correct address, but the wrong data (always
0).

This can break MSIs.

Lets assume MSI0 is vector 34 on CPU0, and MSI1 is vector 33 on CPU0.

hv_arch_irq_unmask() is called on MSI0.  It uses a hypercall to configure
the MSI address and data (0) to vector 34 of CPU0.  This is correct.  Then
hv_arch_irq_unmask is called on MSI1.  It uses another hypercall to
configure the MSI address and data (0) to vector 33 of CPU0.  This is
wrong, and results in both MSI0 and MSI1 being routed to vector 33.  Linux
will observe extra instances of MSI1 and no instances of MSI0 despite the
endpoint device behaving correctly.

For the multi-MSI case, we need unique address and data info for each MSI,
but the cached msi_desc does not provide that.  However, that information
can be gotten from the int_desc cached in the chip_data by
compose_msi_msg().  Fix the multi-MSI case to use that cached information
instead.  Since hv_set_msi_entry_from_desc() is no longer applicable,
remove it.

Signed-off-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
Reviewed-by: Michael Kelley <mikelley@microsoft.com>
Link: https://lore.kernel.org/r/1651068453-29588-1-git-send-email-quic_jhugo@quicinc.com
Signed-off-by: Wei Liu <wei.liu@kernel.org>
(cherry picked from commit 455880dfe292a2bdd3b4ad6a107299fce610e64b)
Signed-off-by: Tim Gardner <tim.gardner@canonical.com>
---
 drivers/pci/controller/pci-hyperv.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

Comments

Bartlomiej Zolnierkiewicz July 14, 2022, 9:18 a.m. UTC | #1 
On Wed, Jul 13, 2022 at 2:58 PM Tim Gardner <tim.gardner@canonical.com> wrote:
>
> From: Jeffrey Hugo <quic_jhugo@quicinc.com>
>
> BugLink: https://bugs.launchpad.net/bugs/1981577
>
> In the multi-MSI case, hv_arch_irq_unmask() will only operate on the first
> MSI of the N allocated.  This is because only the first msi_desc is cached
> and it is shared by all the MSIs of the multi-MSI block.  This means that
> hv_arch_irq_unmask() gets the correct address, but the wrong data (always
> 0).
>
> This can break MSIs.
>
> Lets assume MSI0 is vector 34 on CPU0, and MSI1 is vector 33 on CPU0.
>
> hv_arch_irq_unmask() is called on MSI0.  It uses a hypercall to configure
> the MSI address and data (0) to vector 34 of CPU0.  This is correct.  Then
> hv_arch_irq_unmask is called on MSI1.  It uses another hypercall to
> configure the MSI address and data (0) to vector 33 of CPU0.  This is
> wrong, and results in both MSI0 and MSI1 being routed to vector 33.  Linux
> will observe extra instances of MSI1 and no instances of MSI0 despite the
> endpoint device behaving correctly.
>
> For the multi-MSI case, we need unique address and data info for each MSI,
> but the cached msi_desc does not provide that.  However, that information
> can be gotten from the int_desc cached in the chip_data by
> compose_msi_msg().  Fix the multi-MSI case to use that cached information
> instead.  Since hv_set_msi_entry_from_desc() is no longer applicable,
> remove it.
>
> Signed-off-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
> Reviewed-by: Michael Kelley <mikelley@microsoft.com>
> Link: https://lore.kernel.org/r/1651068453-29588-1-git-send-email-quic_jhugo@quicinc.com
> Signed-off-by: Wei Liu <wei.liu@kernel.org>
> (cherry picked from commit 455880dfe292a2bdd3b4ad6a107299fce610e64b)
> Signed-off-by: Tim Gardner <tim.gardner@canonical.com>
> ---
>  drivers/pci/controller/pci-hyperv.c | 12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
> index 2244338a56ba..317d8d8e52e9 100644
> --- a/drivers/pci/controller/pci-hyperv.c
> +++ b/drivers/pci/controller/pci-hyperv.c
> @@ -605,13 +605,6 @@ static unsigned int hv_msi_get_int_vector(struct irq_data *data)
>         return cfg->vector;
>  }
>
> -static void hv_set_msi_entry_from_desc(union hv_msi_entry *msi_entry,
> -                                      struct msi_desc *msi_desc)
> -{
> -       msi_entry->address.as_uint32 = msi_desc->msg.address_lo;
> -       msi_entry->data.as_uint32 = msi_desc->msg.data;
> -}
> -

Don't we also need to backport commit
22ef7ee3eeb2a41e07f611754ab9a2663232fedf ("PCI: hv: Remove unused
hv_set_msi_entry_from_desc()") after applying patch #8 [ which is
commit d06957d7a6929e6a4aa959cb59d66f0c095fc974 ("PCI: hv: Avoid the
retarget interrupt hypercall in irq_unmask() on ARM64") ] and before
applying this patch?

Best regards,
Bartlomiej

>  static int hv_msi_prepare(struct irq_domain *domain, struct device *dev,
>                           int nvec, msi_alloc_info_t *info)
>  {
> @@ -641,6 +634,7 @@ static void hv_arch_irq_unmask(struct irq_data *data)
>  {
>         struct msi_desc *msi_desc = irq_data_get_msi_desc(data);
>         struct hv_retarget_device_interrupt *params;
> +       struct tran_int_desc *int_desc;
>         struct hv_pcibus_device *hbus;
>         struct cpumask *dest;
>         cpumask_var_t tmp;
> @@ -655,6 +649,7 @@ static void hv_arch_irq_unmask(struct irq_data *data)
>         pdev = msi_desc_to_pci_dev(msi_desc);
>         pbus = pdev->bus;
>         hbus = container_of(pbus->sysdata, struct hv_pcibus_device, sysdata);
> +       int_desc = data->chip_data;
>
>         spin_lock_irqsave(&hbus->retarget_msi_interrupt_lock, flags);
>
> @@ -662,7 +657,8 @@ static void hv_arch_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;
> -       hv_set_msi_entry_from_desc(&params->int_entry.msi_entry, msi_desc);
> +       params->int_entry.msi_entry.address.as_uint32 = int_desc->address & 0xffffffff;
> +       params->int_entry.msi_entry.data.as_uint32 = int_desc->data;
>         params->device_id = (hbus->hdev->dev_instance.b[5] << 24) |
>                            (hbus->hdev->dev_instance.b[4] << 16) |
>                            (hbus->hdev->dev_instance.b[7] << 8) |
> --
> 2.37.0
>
>
> --
> kernel-team mailing list
> kernel-team@lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team
Tim Gardner July 18, 2022, 5:44 p.m. UTC | #2 
On 7/14/22 03:18, Bartlomiej Zolnierkiewicz wrote:
> On Wed, Jul 13, 2022 at 2:58 PM Tim Gardner <tim.gardner@canonical.com> wrote:
>>
>> From: Jeffrey Hugo <quic_jhugo@quicinc.com>
>>
>> BugLink: https://bugs.launchpad.net/bugs/1981577
>>
>> In the multi-MSI case, hv_arch_irq_unmask() will only operate on the first
>> MSI of the N allocated.  This is because only the first msi_desc is cached
>> and it is shared by all the MSIs of the multi-MSI block.  This means that
>> hv_arch_irq_unmask() gets the correct address, but the wrong data (always
>> 0).
>>
>> This can break MSIs.
>>
>> Lets assume MSI0 is vector 34 on CPU0, and MSI1 is vector 33 on CPU0.
>>
>> hv_arch_irq_unmask() is called on MSI0.  It uses a hypercall to configure
>> the MSI address and data (0) to vector 34 of CPU0.  This is correct.  Then
>> hv_arch_irq_unmask is called on MSI1.  It uses another hypercall to
>> configure the MSI address and data (0) to vector 33 of CPU0.  This is
>> wrong, and results in both MSI0 and MSI1 being routed to vector 33.  Linux
>> will observe extra instances of MSI1 and no instances of MSI0 despite the
>> endpoint device behaving correctly.
>>
>> For the multi-MSI case, we need unique address and data info for each MSI,
>> but the cached msi_desc does not provide that.  However, that information
>> can be gotten from the int_desc cached in the chip_data by
>> compose_msi_msg().  Fix the multi-MSI case to use that cached information
>> instead.  Since hv_set_msi_entry_from_desc() is no longer applicable,
>> remove it.
>>
>> Signed-off-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
>> Reviewed-by: Michael Kelley <mikelley@microsoft.com>
>> Link: https://lore.kernel.org/r/1651068453-29588-1-git-send-email-quic_jhugo@quicinc.com
>> Signed-off-by: Wei Liu <wei.liu@kernel.org>
>> (cherry picked from commit 455880dfe292a2bdd3b4ad6a107299fce610e64b)
>> Signed-off-by: Tim Gardner <tim.gardner@canonical.com>
>> ---
>>   drivers/pci/controller/pci-hyperv.c | 12 ++++--------
>>   1 file changed, 4 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
>> index 2244338a56ba..317d8d8e52e9 100644
>> --- a/drivers/pci/controller/pci-hyperv.c
>> +++ b/drivers/pci/controller/pci-hyperv.c
>> @@ -605,13 +605,6 @@ static unsigned int hv_msi_get_int_vector(struct irq_data *data)
>>          return cfg->vector;
>>   }
>>
>> -static void hv_set_msi_entry_from_desc(union hv_msi_entry *msi_entry,
>> -                                      struct msi_desc *msi_desc)
>> -{
>> -       msi_entry->address.as_uint32 = msi_desc->msg.address_lo;
>> -       msi_entry->data.as_uint32 = msi_desc->msg.data;
>> -}
>> -
> 
> Don't we also need to backport commit
> 22ef7ee3eeb2a41e07f611754ab9a2663232fedf ("PCI: hv: Remove unused
> hv_set_msi_entry_from_desc()") after applying patch #8 [ which is
> commit d06957d7a6929e6a4aa959cb59d66f0c095fc974 ("PCI: hv: Avoid the
> retarget interrupt hypercall in irq_unmask() on ARM64") ] and before
> applying this patch?
> 

22ef7ee3eeb2a41e07f611754ab9a2663232fedf ("PCI: hv: Remove unused 
hv_set_msi_entry_from_desc()") does apply, but its not important for our 
build since it doesn't fail with an unused function warning. I can 
certainly insert it when I push the patches.

rtg
diff mbox series

Patch

diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
index 2244338a56ba..317d8d8e52e9 100644
--- a/drivers/pci/controller/pci-hyperv.c
+++ b/drivers/pci/controller/pci-hyperv.c
@@ -605,13 +605,6 @@  static unsigned int hv_msi_get_int_vector(struct irq_data *data)
 	return cfg->vector;
 }
 
-static void hv_set_msi_entry_from_desc(union hv_msi_entry *msi_entry,
-				       struct msi_desc *msi_desc)
-{
-	msi_entry->address.as_uint32 = msi_desc->msg.address_lo;
-	msi_entry->data.as_uint32 = msi_desc->msg.data;
-}
-
 static int hv_msi_prepare(struct irq_domain *domain, struct device *dev,
 			  int nvec, msi_alloc_info_t *info)
 {
@@ -641,6 +634,7 @@  static void hv_arch_irq_unmask(struct irq_data *data)
 {
 	struct msi_desc *msi_desc = irq_data_get_msi_desc(data);
 	struct hv_retarget_device_interrupt *params;
+	struct tran_int_desc *int_desc;
 	struct hv_pcibus_device *hbus;
 	struct cpumask *dest;
 	cpumask_var_t tmp;
@@ -655,6 +649,7 @@  static void hv_arch_irq_unmask(struct irq_data *data)
 	pdev = msi_desc_to_pci_dev(msi_desc);
 	pbus = pdev->bus;
 	hbus = container_of(pbus->sysdata, struct hv_pcibus_device, sysdata);
+	int_desc = data->chip_data;
 
 	spin_lock_irqsave(&hbus->retarget_msi_interrupt_lock, flags);
 
@@ -662,7 +657,8 @@  static void hv_arch_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;
-	hv_set_msi_entry_from_desc(&params->int_entry.msi_entry, msi_desc);
+	params->int_entry.msi_entry.address.as_uint32 = int_desc->address & 0xffffffff;
+	params->int_entry.msi_entry.data.as_uint32 = int_desc->data;
 	params->device_id = (hbus->hdev->dev_instance.b[5] << 24) |
 			   (hbus->hdev->dev_instance.b[4] << 16) |
 			   (hbus->hdev->dev_instance.b[7] << 8) |