diff mbox series

[v2] PCI: hv: Fix the definition of vector in hv_compose_msi_msg()

Message ID 20221027205256.17678-1-decui@microsoft.com
State New
Headers
Series [v2] PCI: hv: Fix the definition of vector in hv_compose_msi_msg() |

Commit Message

Dexuan Cui Oct. 27, 2022, 8:52 p.m. UTC 
  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>
---

v1 was posted here (sorry, I forgot to follow this up...):
https://lwn.net/ml/linux-kernel/20220815185505.7626-1-decui@microsoft.com/

Changes in v2:
  Added the explicit "(u8)" cast in hv_compose_msi_msg().
  Added and improved the comments.
  Fixed a typo in the subject in v1: s/definiton/definition

 drivers/pci/controller/pci-hyperv.c | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

Comments

Jeffrey Hugo Oct. 28, 2022, 2:08 p.m. UTC | #1 
On 10/27/2022 2:52 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>

Reviewed-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
Wei Liu Nov. 2, 2022, 1:06 p.m. UTC | #2 
On Fri, Oct 28, 2022 at 08:08:03AM -0600, Jeffrey Hugo wrote:
> On 10/27/2022 2:52 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>
> 
> Reviewed-by: Jeffrey Hugo <quic_jhugo@quicinc.com>

Applied to hyperv-fixes. Thanks.
Boqun Feng March 30, 2023, 1:56 a.m. UTC | #3 
[Cc stable]

On Thu, Oct 27, 2022 at 01:52:56PM -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.
> 

Dexuan, I think this patch should only be in 5.15, because...

> Fixes: a2bad844a67b ("PCI: hv: Fix interrupt mapping for multi-MSI")

^^^ this commit is already in 5.15.y (commit id 92dcb50f7f09).

Upstream id e70af8d040d2b7904dca93d942ba23fb722e21b1
Cc: <stable@vger.kernel.org> # 5.15.x

Regards,
Boqun

> Signed-off-by: Dexuan Cui <decui@microsoft.com>
> Cc: Jeffrey Hugo <quic_jhugo@quicinc.com>
> Cc: Carl Vanderlip <quic_carlv@quicinc.com>
> ---
> 
> v1 was posted here (sorry, I forgot to follow this up...):
> https://lwn.net/ml/linux-kernel/20220815185505.7626-1-decui@microsoft.com/
> 
> Changes in v2:
>   Added the explicit "(u8)" cast in hv_compose_msi_msg().
>   Added and improved the comments.
>   Fixed a typo in the subject in v1: s/definiton/definition
> 
>  drivers/pci/controller/pci-hyperv.c | 22 ++++++++++++++++------
>  1 file changed, 16 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
> index e7c6f6629e7c..ba64284eaf9f 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, const 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(const struct cpumask *affinity)
>  
>  static u32 hv_compose_msi_req_v2(
>  	struct pci_create_interrupt2 *int_pkt, const 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, const struct cpumask *affinity,
> -	u32 slot, u32 vector, u8 vector_count)
> +	u32 slot, u32 vector, u16 vector_count)
>  {
>  	int cpu;
>  
> @@ -1701,7 +1701,12 @@ static void hv_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
>  	struct compose_comp_ctxt comp;
>  	struct tran_int_desc *int_desc;
>  	struct msi_desc *msi_desc;
> -	u8 vector, vector_count;
> +	/*
> +	 * vector_count should be u16: see hv_msi_desc, hv_msi_desc2
> +	 * and hv_msi_desc3. vector must be u32: see hv_msi_desc3.
> +	 */
> +	u16 vector_count;
> +	u32 vector;
>  	struct {
>  		struct pci_packet pci_pkt;
>  		union {
> @@ -1767,6 +1772,11 @@ static void hv_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
>  		vector_count = 1;
>  	}
>  
> +	/*
> +	 * hv_compose_msi_req_v1 and v2 are for x86 only, meaning 'vector'
> +	 * can't exceed u8. Cast 'vector' down to u8 for v1/v2 explicitly
> +	 * for better readability.
> +	 */
>  	memset(&ctxt, 0, sizeof(ctxt));
>  	init_completion(&comp.comp_pkt.host_event);
>  	ctxt.pci_pkt.completion_func = hv_pci_compose_compl;
> @@ -1777,7 +1787,7 @@ static void hv_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
>  		size = hv_compose_msi_req_v1(&ctxt.int_pkts.v1,
>  					dest,
>  					hpdev->desc.win_slot.slot,
> -					vector,
> +					(u8)vector,
>  					vector_count);
>  		break;
>  
> @@ -1786,7 +1796,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;
>  
> -- 
> 2.25.1
>
Boqun Feng March 30, 2023, 2:55 a.m. UTC | #4 
On Wed, Mar 29, 2023 at 06:56:12PM -0700, Boqun Feng wrote:
> [Cc stable]
> 
> On Thu, Oct 27, 2022 at 01:52:56PM -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.
> > 
> 
> Dexuan, I think this patch should only be in 5.15, because...
> 

Sorry, I meant:

"this patch should also be backported in 5.15"

Regards,
Boqun

> > Fixes: a2bad844a67b ("PCI: hv: Fix interrupt mapping for multi-MSI")
> 
> ^^^ this commit is already in 5.15.y (commit id 92dcb50f7f09).
> 
> Upstream id e70af8d040d2b7904dca93d942ba23fb722e21b1
> Cc: <stable@vger.kernel.org> # 5.15.x
> 
> Regards,
> Boqun
> 
> > Signed-off-by: Dexuan Cui <decui@microsoft.com>
> > Cc: Jeffrey Hugo <quic_jhugo@quicinc.com>
> > Cc: Carl Vanderlip <quic_carlv@quicinc.com>
> > ---
> > 
> > v1 was posted here (sorry, I forgot to follow this up...):
> > https://lwn.net/ml/linux-kernel/20220815185505.7626-1-decui@microsoft.com/
> > 
> > Changes in v2:
> >   Added the explicit "(u8)" cast in hv_compose_msi_msg().
> >   Added and improved the comments.
> >   Fixed a typo in the subject in v1: s/definiton/definition
> > 
> >  drivers/pci/controller/pci-hyperv.c | 22 ++++++++++++++++------
> >  1 file changed, 16 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
> > index e7c6f6629e7c..ba64284eaf9f 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, const 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(const struct cpumask *affinity)
> >  
> >  static u32 hv_compose_msi_req_v2(
> >  	struct pci_create_interrupt2 *int_pkt, const 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, const struct cpumask *affinity,
> > -	u32 slot, u32 vector, u8 vector_count)
> > +	u32 slot, u32 vector, u16 vector_count)
> >  {
> >  	int cpu;
> >  
> > @@ -1701,7 +1701,12 @@ static void hv_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
> >  	struct compose_comp_ctxt comp;
> >  	struct tran_int_desc *int_desc;
> >  	struct msi_desc *msi_desc;
> > -	u8 vector, vector_count;
> > +	/*
> > +	 * vector_count should be u16: see hv_msi_desc, hv_msi_desc2
> > +	 * and hv_msi_desc3. vector must be u32: see hv_msi_desc3.
> > +	 */
> > +	u16 vector_count;
> > +	u32 vector;
> >  	struct {
> >  		struct pci_packet pci_pkt;
> >  		union {
> > @@ -1767,6 +1772,11 @@ static void hv_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
> >  		vector_count = 1;
> >  	}
> >  
> > +	/*
> > +	 * hv_compose_msi_req_v1 and v2 are for x86 only, meaning 'vector'
> > +	 * can't exceed u8. Cast 'vector' down to u8 for v1/v2 explicitly
> > +	 * for better readability.
> > +	 */
> >  	memset(&ctxt, 0, sizeof(ctxt));
> >  	init_completion(&comp.comp_pkt.host_event);
> >  	ctxt.pci_pkt.completion_func = hv_pci_compose_compl;
> > @@ -1777,7 +1787,7 @@ static void hv_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
> >  		size = hv_compose_msi_req_v1(&ctxt.int_pkts.v1,
> >  					dest,
> >  					hpdev->desc.win_slot.slot,
> > -					vector,
> > +					(u8)vector,
> >  					vector_count);
> >  		break;
> >  
> > @@ -1786,7 +1796,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;
> >  
> > -- 
> > 2.25.1
> >
Dexuan Cui March 30, 2023, 3:23 a.m. UTC | #5 
> From: Boqun Feng <boqun.feng@gmail.com>
> Sent: Wednesday, March 29, 2023 7:56 PM
> To: Dexuan Cui <decui@microsoft.com>
>  ...
> On Wed, Mar 29, 2023 at 06:56:12PM -0700, Boqun Feng wrote:
> > [Cc stable]
> >
> > On Thu, Oct 27, 2022 at 01:52:56PM -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.
> > >
> >
> > Dexuan, I think this patch should only be in 5.15, because...
> >
> 
> Sorry, I meant:
> 
> "this patch should also be backported in 5.15"
> 
> Regards,
> Boqun
> 
> > > Fixes: a2bad844a67b ("PCI: hv: Fix interrupt mapping for multi-MSI")
> >
> > ^^^ this commit is already in 5.15.y (commit id 92dcb50f7f09).
> >
> > Upstream id e70af8d040d2b7904dca93d942ba23fb722e21b1
> > Cc: <stable@vger.kernel.org> # 5.15.x

The faulty commit a2bad844a67b ("PCI: hv: Fix interrupt mapping for multi-MSI")
is in all the stable branches, even including 4.14.y, so yes, the commit
e70af8d040d2 ("PCI: hv: Fix the definition of vector in hv_compose_msi_msg()")
should be backported to all the stable branches as well, including
v5.15.y, v5.10.y, v5.4.y, v4.19.y, v4.14.y.

e70af8d040d2 has a Fixes tag. Not sure why it's not automatically backported.
Greg KH March 30, 2023, 5:58 a.m. UTC | #6 
On Thu, Mar 30, 2023 at 03:23:45AM +0000, Dexuan Cui wrote:
> > From: Boqun Feng <boqun.feng@gmail.com>
> > Sent: Wednesday, March 29, 2023 7:56 PM
> > To: Dexuan Cui <decui@microsoft.com>
> >  ...
> > On Wed, Mar 29, 2023 at 06:56:12PM -0700, Boqun Feng wrote:
> > > [Cc stable]
> > >
> > > On Thu, Oct 27, 2022 at 01:52:56PM -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.
> > > >
> > >
> > > Dexuan, I think this patch should only be in 5.15, because...
> > >
> > 
> > Sorry, I meant:
> > 
> > "this patch should also be backported in 5.15"
> > 
> > Regards,
> > Boqun
> > 
> > > > Fixes: a2bad844a67b ("PCI: hv: Fix interrupt mapping for multi-MSI")
> > >
> > > ^^^ this commit is already in 5.15.y (commit id 92dcb50f7f09).
> > >
> > > Upstream id e70af8d040d2b7904dca93d942ba23fb722e21b1
> > > Cc: <stable@vger.kernel.org> # 5.15.x
> 
> The faulty commit a2bad844a67b ("PCI: hv: Fix interrupt mapping for multi-MSI")
> is in all the stable branches, even including 4.14.y, so yes, the commit
> e70af8d040d2 ("PCI: hv: Fix the definition of vector in hv_compose_msi_msg()")
> should be backported to all the stable branches as well, including
> v5.15.y, v5.10.y, v5.4.y, v4.19.y, v4.14.y.
> 
> e70af8d040d2 has a Fixes tag. Not sure why it's not automatically backported.

Because "Fixes:" is not the flag that we are sure to trigger off of.
Please read:
    https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
for how to do this properly.

That being said, because some subsystem maintainers do NOT put cc:
stable in their patches, we do sometimes sweep the tree and try to pick
up things with only "Fixes:" but we don't always catch everything.

So if you want to be sure a patch is applied, please always add a cc:
stable in the patch.

thanks,

greg k-h
Dexuan Cui March 30, 2023, 7:50 p.m. UTC | #7 
> From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > ...
> > e70af8d040d2 has a Fixes tag. Not sure why it's not automatically
> backported.
> 
> Because "Fixes:" is not the flag that we are sure to trigger off of.
> Please read:
> https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
> for how to do this properly.

Thanks, I just read this again to refresh my memory :-)
I remember Sasha has an AI algorithm to pick up patches into the stable
tree and a "Fixes" tag should be a strong indicator. 

I tired to manually cherry-pick e70af8d040d2 to 5.15.y and got some
merge conflicts. Probably that's why Sasha's script didn't automatically 
do the backport. @Carl, @Jeffrey, may I know if you have some cycles to
help backport e70af8d040d2?

> That being said, because some subsystem maintainers do NOT put cc:
> stable in their patches, we do sometimes sweep the tree and try to pick
> up things with only "Fixes:" but we don't always catch everything.
> 
> So if you want to be sure a patch is applied, please always add a cc:
> stable in the patch.
> 
> greg k-h

Hi Greg, since you're here, I'd like to ask a question:

If I add the cc: stable line in a patch and use git-send-email to post
the patch, git-send-email also posts the patch to the stable list -- is
this acceptable? Sometimes a patch may have to undergo multiple
revisions, meaning all the discussion emails go to the stable list
unnecessarily, and I guess this is not good?

It looks like there is no git-send-email option to exclude an email.

Thanks,
Dexuan
Greg KH March 30, 2023, 8:56 p.m. UTC | #8 
On Thu, Mar 30, 2023 at 07:50:11PM +0000, Dexuan Cui wrote:
> > From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > ...
> > > e70af8d040d2 has a Fixes tag. Not sure why it's not automatically
> > backported.
> > 
> > Because "Fixes:" is not the flag that we are sure to trigger off of.
> > Please read:
> > https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
> > for how to do this properly.
> 
> Thanks, I just read this again to refresh my memory :-)
> I remember Sasha has an AI algorithm to pick up patches into the stable
> tree and a "Fixes" tag should be a strong indicator. 

Yes, we have to rely on "hints" like that due to maintainers not wanting
to put any cc: stable tags for many subsystems so we have to dig them
out somehow.

> If I add the cc: stable line in a patch and use git-send-email to post
> the patch, git-send-email also posts the patch to the stable list -- is
> this acceptable?

Totally acceptable.

> Sometimes a patch may have to undergo multiple
> revisions, meaning all the discussion emails go to the stable list
> unnecessarily, and I guess this is not good?

It's not a problem at all, happens all the time and in fact I like it as
it gives us a heads-up that a patch is going to be eventually merged for
us to handle.

> It looks like there is no git-send-email option to exclude an email.

No need to, don't worry about that at all.

thanks,

greg k-h
Greg KH April 3, 2023, 1:07 p.m. UTC | #9 
On Thu, Mar 30, 2023 at 03:23:45AM +0000, Dexuan Cui wrote:
> > From: Boqun Feng <boqun.feng@gmail.com>
> > Sent: Wednesday, March 29, 2023 7:56 PM
> > To: Dexuan Cui <decui@microsoft.com>
> >  ...
> > On Wed, Mar 29, 2023 at 06:56:12PM -0700, Boqun Feng wrote:
> > > [Cc stable]
> > >
> > > On Thu, Oct 27, 2022 at 01:52:56PM -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.
> > > >
> > >
> > > Dexuan, I think this patch should only be in 5.15, because...
> > >
> > 
> > Sorry, I meant:
> > 
> > "this patch should also be backported in 5.15"
> > 
> > Regards,
> > Boqun
> > 
> > > > Fixes: a2bad844a67b ("PCI: hv: Fix interrupt mapping for multi-MSI")
> > >
> > > ^^^ this commit is already in 5.15.y (commit id 92dcb50f7f09).
> > >
> > > Upstream id e70af8d040d2b7904dca93d942ba23fb722e21b1
> > > Cc: <stable@vger.kernel.org> # 5.15.x
> 
> The faulty commit a2bad844a67b ("PCI: hv: Fix interrupt mapping for multi-MSI")
> is in all the stable branches, even including 4.14.y, so yes, the commit
> e70af8d040d2 ("PCI: hv: Fix the definition of vector in hv_compose_msi_msg()")
> should be backported to all the stable branches as well, including
> v5.15.y, v5.10.y, v5.4.y, v4.19.y, v4.14.y.
> 
> e70af8d040d2 has a Fixes tag. Not sure why it's not automatically backported.

Also, the most obvious reason, it does NOT apply there!  If this needs
to go to 5.15.y and older, please send working backports of it.

thanks,

greg k-h
Dexuan Cui April 3, 2023, 6:29 p.m. UTC | #10 
> From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Sent: Monday, April 3, 2023 6:08 AM
> To: Dexuan Cui <decui@microsoft.com>
> Cc: Boqun Feng <boqun.feng@gmail.com>; quic_jhugo@quicinc.com;
> quic_carlv@quicinc.com; wei.liu@kernel.org; KY Srinivasan
> <kys@microsoft.com>; Haiyang Zhang <haiyangz@microsoft.com>;
> sthemmin@microsoft.com; lpieralisi@kernel.org; bhelgaas@google.com;
> linux-hyperv@vger.kernel.org; linux-pci@vger.kernel.org;
> linux-kernel@vger.kernel.org; Michael Kelley (LINUX)
> <mikelley@microsoft.com>; robh@kernel.org; kw@linux.com;
> helgaas@kernel.org; alex.williamson@redhat.com; stable@vger.kernel.org;
> Sasha Levin <sashal@kernel.org>
> Subject: Re: [PATCH v2] PCI: hv: Fix the definition of vector in
> hv_compose_msi_msg()
> 
> On Thu, Mar 30, 2023 at 03:23:45AM +0000, Dexuan Cui wrote:
> > > From: Boqun Feng <boqun.feng@gmail.com>
> > > Sent: Wednesday, March 29, 2023 7:56 PM
> > > To: Dexuan Cui <decui@microsoft.com>
> > >  ...
> > > On Wed, Mar 29, 2023 at 06:56:12PM -0700, Boqun Feng wrote:
> > > > [Cc stable]
> > > >
> > > > On Thu, Oct 27, 2022 at 01:52:56PM -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.
> > > > >
> > > >
> > > > Dexuan, I think this patch should only be in 5.15, because...
> > > >
> > >
> > > Sorry, I meant:
> > >
> > > "this patch should also be backported in 5.15"
> > >
> > > Regards,
> > > Boqun
> > >
> > > > > Fixes: a2bad844a67b ("PCI: hv: Fix interrupt mapping for multi-MSI")
> > > >
> > > > ^^^ this commit is already in 5.15.y (commit id 92dcb50f7f09).
> > > >
> > > > Upstream id e70af8d040d2b7904dca93d942ba23fb722e21b1
> > > > Cc: <stable@vger.kernel.org> # 5.15.x
> >
> > The faulty commit a2bad844a67b ("PCI: hv: Fix interrupt mapping for
> multi-MSI")
> > is in all the stable branches, even including 4.14.y, so yes, the commit
> > e70af8d040d2 ("PCI: hv: Fix the definition of vector in
> hv_compose_msi_msg()")
> > should be backported to all the stable branches as well, including
> > v5.15.y, v5.10.y, v5.4.y, v4.19.y, v4.14.y.
> >
> > e70af8d040d2 has a Fixes tag. Not sure why it's not automatically
> backported.
> 
> Also, the most obvious reason, it does NOT apply there!  If this needs
> to go to 5.15.y and older, please send working backports of it.
> 
> thanks,
> 
> greg k-h

I also found that we need to manually rebase the patch for 5.15.y
and older. +Jose, who will help to do the backport.
Jeffrey Hugo April 3, 2023, 6:50 p.m. UTC | #11 
On 3/30/2023 1:50 PM, Dexuan Cui wrote:
>> From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>> ...
>>> e70af8d040d2 has a Fixes tag. Not sure why it's not automatically
>> backported.
>>
>> Because "Fixes:" is not the flag that we are sure to trigger off of.
>> Please read:
>> https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
>> for how to do this properly.
> 
> Thanks, I just read this again to refresh my memory :-)
> I remember Sasha has an AI algorithm to pick up patches into the stable
> tree and a "Fixes" tag should be a strong indicator.
> 
> I tired to manually cherry-pick e70af8d040d2 to 5.15.y and got some
> merge conflicts. Probably that's why Sasha's script didn't automatically
> do the backport. @Carl, @Jeffrey, may I know if you have some cycles to
> help backport e70af8d040d2?

We might have some cycles this week.

-Jeff
Dexuan Cui April 3, 2023, 7:29 p.m. UTC | #12 
> From: Jeffrey Hugo <quic_jhugo@quicinc.com>
> Sent: Monday, April 3, 2023 11:51 AM
> 
> On 3/30/2023 1:50 PM, Dexuan Cui wrote:
> > ...
> > I tired to manually cherry-pick e70af8d040d2 to 5.15.y and got some
> > merge conflicts. Probably that's why Sasha's script didn't automatically
> > do the backport. @Carl, @Jeffrey, may I know if you have some cycles to
> > help backport e70af8d040d2?
> 
> We might have some cycles this week.
> 
> -Jeff

Thanks, Jeff. We already have Jose on our team working on the backport.
If you think you can do the backport sooner, please let us know so we can
avoid duplicate efforts. :-)
diff mbox series

Patch

diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
index e7c6f6629e7c..ba64284eaf9f 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, const 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(const struct cpumask *affinity)
 
 static u32 hv_compose_msi_req_v2(
 	struct pci_create_interrupt2 *int_pkt, const 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, const struct cpumask *affinity,
-	u32 slot, u32 vector, u8 vector_count)
+	u32 slot, u32 vector, u16 vector_count)
 {
 	int cpu;
 
@@ -1701,7 +1701,12 @@  static void hv_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
 	struct compose_comp_ctxt comp;
 	struct tran_int_desc *int_desc;
 	struct msi_desc *msi_desc;
-	u8 vector, vector_count;
+	/*
+	 * vector_count should be u16: see hv_msi_desc, hv_msi_desc2
+	 * and hv_msi_desc3. vector must be u32: see hv_msi_desc3.
+	 */
+	u16 vector_count;
+	u32 vector;
 	struct {
 		struct pci_packet pci_pkt;
 		union {
@@ -1767,6 +1772,11 @@  static void hv_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
 		vector_count = 1;
 	}
 
+	/*
+	 * hv_compose_msi_req_v1 and v2 are for x86 only, meaning 'vector'
+	 * can't exceed u8. Cast 'vector' down to u8 for v1/v2 explicitly
+	 * for better readability.
+	 */
 	memset(&ctxt, 0, sizeof(ctxt));
 	init_completion(&comp.comp_pkt.host_event);
 	ctxt.pci_pkt.completion_func = hv_pci_compose_compl;
@@ -1777,7 +1787,7 @@  static void hv_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
 		size = hv_compose_msi_req_v1(&ctxt.int_pkts.v1,
 					dest,
 					hpdev->desc.win_slot.slot,
-					vector,
+					(u8)vector,
 					vector_count);
 		break;
 
@@ -1786,7 +1796,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;