[PATCH] PCI: hv: Fix the definiton of vector in hv_compose_msi_msg()

[PATCH] PCI: hv: Fix the definiton of vector in hv_compose_msi_msg()

[Dexuan Cui]

The local variable 'vector' must be u32 rather than u8: see the
struct hv_msi_desc3.

'vector_count' should be u16 rather than u8: see struct hv_msi_desc,
hv_msi_desc2 and hv_msi_desc3.

Fixes: a2bad844a67b ("PCI: hv: Fix interrupt mapping for multi-MSI")
Signed-off-by: Dexuan Cui <decui@microsoft.com>
Cc: Jeffrey Hugo <quic_jhugo@quicinc.com>
Cc: Carl Vanderlip <quic_carlv@quicinc.com>
---

The patch should be appplied after the earlier patch:
    [PATCH] PCI: hv: Only reuse existing IRTE allocation for Multi-MSI
    https://lwn.net/ml/linux-kernel/20220804025104.15673-1-decui%40microsoft.com/

 drivers/pci/controller/pci-hyperv.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
index 65d0dab25deb..53580899c859 100644
--- a/drivers/pci/controller/pci-hyperv.c
+++ b/drivers/pci/controller/pci-hyperv.c
@@ -1614,7 +1614,7 @@ static void hv_pci_compose_compl(void *context, struct pci_response *resp,
 
 static u32 hv_compose_msi_req_v1(
 	struct pci_create_interrupt *int_pkt, struct cpumask *affinity,
-	u32 slot, u8 vector, u8 vector_count)
+	u32 slot, u8 vector, u16 vector_count)
 {
 	int_pkt->message_type.type = PCI_CREATE_INTERRUPT_MESSAGE;
 	int_pkt->wslot.slot = slot;
@@ -1642,7 +1642,7 @@ static int hv_compose_msi_req_get_cpu(struct cpumask *affinity)
 
 static u32 hv_compose_msi_req_v2(
 	struct pci_create_interrupt2 *int_pkt, struct cpumask *affinity,
-	u32 slot, u8 vector, u8 vector_count)
+	u32 slot, u8 vector, u16 vector_count)
 {
 	int cpu;
 
@@ -1661,7 +1661,7 @@ static u32 hv_compose_msi_req_v2(
 
 static u32 hv_compose_msi_req_v3(
 	struct pci_create_interrupt3 *int_pkt, struct cpumask *affinity,
-	u32 slot, u32 vector, u8 vector_count)
+	u32 slot, u32 vector, u16 vector_count)
 {
 	int cpu;
 
@@ -1702,7 +1702,8 @@ static void hv_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
 	struct tran_int_desc *int_desc;
 	struct msi_desc *msi_desc;
 	bool multi_msi;
-	u8 vector, vector_count;
+	u32 vector; /* Must be u32: see the struct hv_msi_desc3 */
+	u16 vector_count;
 	struct {
 		struct pci_packet pci_pkt;
 		union {
-- 
2.25.1

Re: [PATCH] PCI: hv: Fix the definiton of vector in hv_compose_msi_msg()

[Bjorn Helgaas]

s/definiton/definition/ in subject
(only if you have other occasion to repost this)

On Mon, Aug 15, 2022 at 11:55:05AM -0700, Dexuan Cui wrote:
> The local variable 'vector' must be u32 rather than u8: see the
> struct hv_msi_desc3.
> 
> 'vector_count' should be u16 rather than u8: see struct hv_msi_desc,
> hv_msi_desc2 and hv_msi_desc3.
> 
> Fixes: a2bad844a67b ("PCI: hv: Fix interrupt mapping for multi-MSI")
> Signed-off-by: Dexuan Cui <decui@microsoft.com>
> Cc: Jeffrey Hugo <quic_jhugo@quicinc.com>
> Cc: Carl Vanderlip <quic_carlv@quicinc.com>

Looks like Wei has been applying most changes to pci-hyperv.c, so I
assume the same will happen here.

> ---
> 
> The patch should be appplied after the earlier patch:
>     [PATCH] PCI: hv: Only reuse existing IRTE allocation for Multi-MSI
>     https://lwn.net/ml/linux-kernel/20220804025104.15673-1-decui%40microsoft.com/
> 
>  drivers/pci/controller/pci-hyperv.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
> index 65d0dab25deb..53580899c859 100644
> --- a/drivers/pci/controller/pci-hyperv.c
> +++ b/drivers/pci/controller/pci-hyperv.c
> @@ -1614,7 +1614,7 @@ static void hv_pci_compose_compl(void *context, struct pci_response *resp,
>  
>  static u32 hv_compose_msi_req_v1(
>  	struct pci_create_interrupt *int_pkt, struct cpumask *affinity,
> -	u32 slot, u8 vector, u8 vector_count)
> +	u32 slot, u8 vector, u16 vector_count)
>  {
>  	int_pkt->message_type.type = PCI_CREATE_INTERRUPT_MESSAGE;
>  	int_pkt->wslot.slot = slot;
> @@ -1642,7 +1642,7 @@ static int hv_compose_msi_req_get_cpu(struct cpumask *affinity)
>  
>  static u32 hv_compose_msi_req_v2(
>  	struct pci_create_interrupt2 *int_pkt, struct cpumask *affinity,
> -	u32 slot, u8 vector, u8 vector_count)
> +	u32 slot, u8 vector, u16 vector_count)
>  {
>  	int cpu;
>  
> @@ -1661,7 +1661,7 @@ static u32 hv_compose_msi_req_v2(
>  
>  static u32 hv_compose_msi_req_v3(
>  	struct pci_create_interrupt3 *int_pkt, struct cpumask *affinity,
> -	u32 slot, u32 vector, u8 vector_count)
> +	u32 slot, u32 vector, u16 vector_count)
>  {
>  	int cpu;
>  
> @@ -1702,7 +1702,8 @@ static void hv_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
>  	struct tran_int_desc *int_desc;
>  	struct msi_desc *msi_desc;
>  	bool multi_msi;
> -	u8 vector, vector_count;
> +	u32 vector; /* Must be u32: see the struct hv_msi_desc3 */
> +	u16 vector_count;
>  	struct {
>  		struct pci_packet pci_pkt;
>  		union {
> -- 
> 2.25.1
> 

RE: [PATCH] PCI: hv: Fix the definiton of vector in hv_compose_msi_msg()

[Dexuan Cui]

> From: Bjorn Helgaas <helgaas@kernel.org>
> Sent: Monday, August 15, 2022 1:36 PM
> To: Dexuan Cui <decui@microsoft.com>
> 
> s/definiton/definition/ in subject
> (only if you have other occasion to repost this)

Thanks, Bjorn! I suppose Wei can help fix this. :-)
 
> On Mon, Aug 15, 2022 at 11:55:05AM -0700, Dexuan Cui wrote:
> > The local variable 'vector' must be u32 rather than u8: see the
> > struct hv_msi_desc3.
> >
> > 'vector_count' should be u16 rather than u8: see struct hv_msi_desc,
> > hv_msi_desc2 and hv_msi_desc3.
> >
> > Fixes: a2bad844a67b ("PCI: hv: Fix interrupt mapping for multi-MSI")
> > Signed-off-by: Dexuan Cui <decui@microsoft.com>
> > Cc: Jeffrey Hugo <quic_jhugo@quicinc.com>
> > Cc: Carl Vanderlip <quic_carlv@quicinc.com>
> 
> Looks like Wei has been applying most changes to pci-hyperv.c, so I
> assume the same will happen here.

So I interpret this as an ack for Wei to apply the earlier patch
    [PATCH] PCI: hv: Only reuse existing IRTE allocation for Multi-MSI
and this patch.

The two small patches are pure Hyper-V specific changes, so IMO it's
better for them to go through Wei's Hyper-V tree rather than the PCI tree.
(It looks like the PCI folks are too busy right now)

> > ---
> >
> > The patch should be appplied after the earlier patch:
> >     [PATCH] PCI: hv: Only reuse existing IRTE allocation for Multi-MSI

Re: [PATCH] PCI: hv: Fix the definiton of vector in hv_compose_msi_msg()

[Wei Liu]

On Mon, Aug 15, 2022 at 03:35:45PM -0500, Bjorn Helgaas wrote:
> s/definiton/definition/ in subject
> (only if you have other occasion to repost this)
> 
> On Mon, Aug 15, 2022 at 11:55:05AM -0700, Dexuan Cui wrote:
> > The local variable 'vector' must be u32 rather than u8: see the
> > struct hv_msi_desc3.
> > 
> > 'vector_count' should be u16 rather than u8: see struct hv_msi_desc,
> > hv_msi_desc2 and hv_msi_desc3.
> > 
> > Fixes: a2bad844a67b ("PCI: hv: Fix interrupt mapping for multi-MSI")
> > Signed-off-by: Dexuan Cui <decui@microsoft.com>
> > Cc: Jeffrey Hugo <quic_jhugo@quicinc.com>
> > Cc: Carl Vanderlip <quic_carlv@quicinc.com>
> 
> Looks like Wei has been applying most changes to pci-hyperv.c, so I
> assume the same will happen here.

I can take care of this one via hyperv-fixes, but ...

> 
> > ---
> > 
> > The patch should be appplied after the earlier patch:
> >     [PATCH] PCI: hv: Only reuse existing IRTE allocation for Multi-MSI
> >     https://lwn.net/ml/linux-kernel/20220804025104.15673-1-decui%40microsoft.com/
> > 

... this patch looks to be rejected.

Thanks,
Wei.

RE: [PATCH] PCI: hv: Fix the definiton of vector in hv_compose_msi_msg()

[Dexuan Cui]

> From: Wei Liu <wei.liu@kernel.org>
> Sent: Friday, August 19, 2022 8:53 AM
> To: Bjorn Helgaas <helgaas@kernel.org>
> 
> On Mon, Aug 15, 2022 at 03:35:45PM -0500, Bjorn Helgaas wrote:
> > s/definiton/definition/ in subject
> > (only if you have other occasion to repost this)
> >
> > On Mon, Aug 15, 2022 at 11:55:05AM -0700, Dexuan Cui wrote:
> > > The local variable 'vector' must be u32 rather than u8: see the
> > > struct hv_msi_desc3.
> > >
> > > 'vector_count' should be u16 rather than u8: see struct hv_msi_desc,
> > > hv_msi_desc2 and hv_msi_desc3.
> > >
> > > Fixes: a2bad844a67b ("PCI: hv: Fix interrupt mapping for multi-MSI")
> > > Signed-off-by: Dexuan Cui <decui@microsoft.com>
> > > Cc: Jeffrey Hugo <quic_jhugo@quicinc.com>
> > > Cc: Carl Vanderlip <quic_carlv@quicinc.com>
> >
> > Looks like Wei has been applying most changes to pci-hyperv.c, so I
> > assume the same will happen here.
> 
> I can take care of this one via hyperv-fixes, but ...

Wei, please ignore this patch. I'll post v2 of this patch with v2 of the other patch.

> > > ---
> > >
> > > The patch should be appplied after the earlier patch:
> > >     [PATCH] PCI: hv: Only reuse existing IRTE allocation for Multi-MSI
> > >
> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Flwn.ne
> t%2Fml%2Flinux-kernel%2F20220804025104.15673-1-decui%2540microsoft.co
> m%2F&amp;data=05%7C01%7Cdecui%40microsoft.com%7Cc8ab6638b7d747b
> cddf008da81fadc12%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C6
> 37965211688628404%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMD
> AiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&
> amp;sdata=Rb2MfkSFmJ%2B8ze%2FllN0THBhODCtmnZ8oSMB0EOn20u4%3D&
> amp;reserved=0
> > >
> 
> ... this patch looks to be rejected.

Correct. I'm working on a new version.

> 
> Thanks,
> Wei.

Re: [PATCH] PCI: hv: Fix the definiton of vector in hv_compose_msi_msg()

[Jeffrey Hugo]

On 8/15/2022 12:55 PM, Dexuan Cui wrote:
> The local variable 'vector' must be u32 rather than u8: see the
> struct hv_msi_desc3.
> 
> 'vector_count' should be u16 rather than u8: see struct hv_msi_desc,
> hv_msi_desc2 and hv_msi_desc3.
> 
> Fixes: a2bad844a67b ("PCI: hv: Fix interrupt mapping for multi-MSI")
> Signed-off-by: Dexuan Cui <decui@microsoft.com>
> Cc: Jeffrey Hugo <quic_jhugo@quicinc.com>
> Cc: Carl Vanderlip <quic_carlv@quicinc.com>
> ---
> 
> The patch should be appplied after the earlier patch:
>      [PATCH] PCI: hv: Only reuse existing IRTE allocation for Multi-MSI
>      https://lwn.net/ml/linux-kernel/20220804025104.15673-1-decui%40microsoft.com/
> 
>   drivers/pci/controller/pci-hyperv.c | 9 +++++----
>   1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
> index 65d0dab25deb..53580899c859 100644
> --- a/drivers/pci/controller/pci-hyperv.c
> +++ b/drivers/pci/controller/pci-hyperv.c
> @@ -1614,7 +1614,7 @@ static void hv_pci_compose_compl(void *context, struct pci_response *resp,
>   
>   static u32 hv_compose_msi_req_v1(
>   	struct pci_create_interrupt *int_pkt, struct cpumask *affinity,
> -	u32 slot, u8 vector, u8 vector_count)
> +	u32 slot, u8 vector, u16 vector_count)
>   {
>   	int_pkt->message_type.type = PCI_CREATE_INTERRUPT_MESSAGE;
>   	int_pkt->wslot.slot = slot;
> @@ -1642,7 +1642,7 @@ static int hv_compose_msi_req_get_cpu(struct cpumask *affinity)
>   
>   static u32 hv_compose_msi_req_v2(
>   	struct pci_create_interrupt2 *int_pkt, struct cpumask *affinity,
> -	u32 slot, u8 vector, u8 vector_count)
> +	u32 slot, u8 vector, u16 vector_count)
>   {
>   	int cpu;
>   
> @@ -1661,7 +1661,7 @@ static u32 hv_compose_msi_req_v2(
>   
>   static u32 hv_compose_msi_req_v3(
>   	struct pci_create_interrupt3 *int_pkt, struct cpumask *affinity,
> -	u32 slot, u32 vector, u8 vector_count)
> +	u32 slot, u32 vector, u16 vector_count)
>   {
>   	int cpu;
>   
> @@ -1702,7 +1702,8 @@ static void hv_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
>   	struct tran_int_desc *int_desc;
>   	struct msi_desc *msi_desc;
>   	bool multi_msi;
> -	u8 vector, vector_count;
> +	u32 vector; /* Must be u32: see the struct hv_msi_desc3 */

Don't you need to cast this down to a u8 for v1 and v2?
Feels like this should be generating a compiler warning...

RE: [PATCH] PCI: hv: Fix the definiton of vector in hv_compose_msi_msg()

[Dexuan Cui]

> From: Jeffrey Hugo <quic_jhugo@quicinc.com>
> Sent: Tuesday, August 16, 2022 9:01 AM
> > ...
> > @@ -1702,7 +1702,8 @@ static void hv_compose_msi_msg(struct irq_data
> *data, struct msi_msg *msg)
> >   	struct tran_int_desc *int_desc;
> >   	struct msi_desc *msi_desc;
> >   	bool multi_msi;
> > -	u8 vector, vector_count;
> > +	u32 vector; /* Must be u32: see the struct hv_msi_desc3 */
> 
> Don't you need to cast this down to a u8 for v1 and v2?
> Feels like this should be generating a compiler warning...

My gcc 9.4.0 didn't generate a warning.

hv_compose_msi_req_v3() is for both ARM64 and x86. In the case of ARM64
the 'vector' can be a u32 integer according to the comment around struct 
hv_msi_desc3.

hv_compose_msi_req_v1 and v2 are for x86 only, and the 'vector' can't be
longer than u8. I can post a v2 with the extra changes below:


diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
index 53580899c859..c7fd76bc8b4c 100644
--- a/drivers/pci/controller/pci-hyperv.c
+++ b/drivers/pci/controller/pci-hyperv.c
@@ -1703,7 +1703,7 @@ static void hv_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
        struct msi_desc *msi_desc;
        bool multi_msi;
        u32 vector; /* Must be u32: see the struct hv_msi_desc3 */
-       u16 vector_count;
+       u16 vector_count; /* see hv_msi_desc, hv_msi_desc2 and hv_msi_desc3 */
        struct {
                struct pci_packet pci_pkt;
                union {
@@ -1788,12 +1788,17 @@ static void hv_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
        ctxt.pci_pkt.completion_func = hv_pci_compose_compl;
        ctxt.pci_pkt.compl_ctxt = &comp;

+       /*
+        * hv_compose_msi_req_v1 and v2 are for x86 only, meaning 'vector'
+        * can't be longer than u8. Cast 'vector' down to u8 explicitly for
+        * better readability.
+        */
        switch (hbus->protocol_version) {
        case PCI_PROTOCOL_VERSION_1_1:
                size = hv_compose_msi_req_v1(&ctxt.int_pkts.v1,
                                        dest,
                                        hpdev->desc.win_slot.slot,
-                                       vector,
+                                       (u8)vector,
                                        vector_count);
                break;

@@ -1802,7 +1807,7 @@ static void hv_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
                size = hv_compose_msi_req_v2(&ctxt.int_pkts.v2,
                                        dest,
                                        hpdev->desc.win_slot.slot,
-                                       vector,
+                                       (u8)vector,
                                        vector_count);
                break;


Thanks,
-- Dexuan