| Message ID | 20220815185505.7626-1-decui@microsoft.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | PCI: hv: Fix the definiton of vector in hv_compose_msi_msg() | expand |
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 >
> 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
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...
> 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 = ∁ + /* + * 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
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.
> 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&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.
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 {
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(-)