diff mbox series

[v2,2/3] PCI: hv: Move retarget related structures into tlfs header

Message ID 20200203050313.69247-3-boqun.feng@gmail.com (mailing list archive)
State New, archived
Headers show
Series PCI: hv: Generify pci-hyperv.c | expand

Commit Message

Boqun Feng Feb. 3, 2020, 5:03 a.m. UTC
Currently, retarget_msi_interrupt and other structures it relys on are
defined in pci-hyperv.c. However, those structures are actually defined
in Hypervisor Top-Level Functional Specification [1] and may be
different in sizes of fields or layout from architecture to
architecture. Therefore, this patch moves those definitions into x86's
tlfs header file to support virtual PCI on non-x86 architectures in the
future.

Besides, while I'm at it, rename retarget_msi_interrupt to
hv_retarget_msi_interrupt for the consistent name convention, also
mirroring the name in TLFS.

[1]: https://docs.microsoft.com/en-us/virtualization/hyper-v-on-windows/reference/tlfs

Signed-off-by: Boqun Feng (Microsoft) <boqun.feng@gmail.com>
---
 arch/x86/include/asm/hyperv-tlfs.h  | 31 ++++++++++++++++++++++++++
 drivers/pci/controller/pci-hyperv.c | 34 ++---------------------------
 2 files changed, 33 insertions(+), 32 deletions(-)

Comments

Andrew Murray Feb. 3, 2020, 9:41 a.m. UTC | #1
On Mon, Feb 03, 2020 at 01:03:12PM +0800, Boqun Feng wrote:
> Currently, retarget_msi_interrupt and other structures it relys on are
> defined in pci-hyperv.c. However, those structures are actually defined
> in Hypervisor Top-Level Functional Specification [1] and may be
> different in sizes of fields or layout from architecture to
> architecture. Therefore, this patch moves those definitions into x86's

Nit: Rather than 'Therefore, this patch moves ...' - how about 'Let's move
...'?

> tlfs header file to support virtual PCI on non-x86 architectures in the
> future.
> 
> Besides, while I'm at it, rename retarget_msi_interrupt to

Nit: 'Besides, while I'm at it' - this type of wording describes what
*you've* done rather than what the patch is doing. You could replace
that quoted text with 'Additionally, '

> hv_retarget_msi_interrupt for the consistent name convention, also

Nit: s/name/naming

> mirroring the name in TLFS.
> 
> [1]: https://docs.microsoft.com/en-us/virtualization/hyper-v-on-windows/reference/tlfs
> 
> Signed-off-by: Boqun Feng (Microsoft) <boqun.feng@gmail.com>
> ---
>  arch/x86/include/asm/hyperv-tlfs.h  | 31 ++++++++++++++++++++++++++
>  drivers/pci/controller/pci-hyperv.c | 34 ++---------------------------
>  2 files changed, 33 insertions(+), 32 deletions(-)
> 
> diff --git a/arch/x86/include/asm/hyperv-tlfs.h b/arch/x86/include/asm/hyperv-tlfs.h
> index 739bd89226a5..4a76e442481a 100644
> --- a/arch/x86/include/asm/hyperv-tlfs.h
> +++ b/arch/x86/include/asm/hyperv-tlfs.h
> @@ -911,4 +911,35 @@ struct hv_tlb_flush_ex {
>  struct hv_partition_assist_pg {
>  	u32 tlb_lock_count;
>  };
> +
> +struct hv_interrupt_entry {
> +	u32 source;			/* 1 for MSI(-X) */
> +	u32 reserved1;
> +	u32 address;
> +	u32 data;
> +} __packed;

Why have you added __packed here? There is no mention of this change in the
commit log? Is it needed?

> +
> +/*
> + * flags for hv_device_interrupt_target.flags
> + */
> +#define HV_DEVICE_INTERRUPT_TARGET_MULTICAST		1
> +#define HV_DEVICE_INTERRUPT_TARGET_PROCESSOR_SET	2
> +
> +struct hv_device_interrupt_target {
> +	u32 vector;
> +	u32 flags;
> +	union {
> +		u64 vp_mask;
> +		struct hv_vpset vp_set;
> +	};
> +} __packed;

Same here.

> +
> +/* HvRetargetDeviceInterrupt hypercall */
> +struct hv_retarget_device_interrupt {
> +	u64 partition_id;

Why drop the 'self' comment?

> +	u64 device_id;
> +	struct hv_interrupt_entry int_entry;
> +	u64 reserved2;
> +	struct hv_device_interrupt_target int_target;
> +} __packed __aligned(8);
>  #endif
> diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
> index aacfcc90d929..0d9b74503577 100644
> --- a/drivers/pci/controller/pci-hyperv.c
> +++ b/drivers/pci/controller/pci-hyperv.c
> @@ -406,36 +406,6 @@ struct pci_eject_response {
>  
>  static int pci_ring_size = (4 * PAGE_SIZE);
>  
> -struct hv_interrupt_entry {
> -	u32	source;			/* 1 for MSI(-X) */
> -	u32	reserved1;
> -	u32	address;
> -	u32	data;
> -};
> -
> -/*
> - * flags for hv_device_interrupt_target.flags
> - */
> -#define HV_DEVICE_INTERRUPT_TARGET_MULTICAST		1
> -#define HV_DEVICE_INTERRUPT_TARGET_PROCESSOR_SET	2
> -
> -struct hv_device_interrupt_target {
> -	u32	vector;
> -	u32	flags;
> -	union {
> -		u64		 vp_mask;
> -		struct hv_vpset vp_set;
> -	};
> -};
> -
> -struct retarget_msi_interrupt {
> -	u64	partition_id;		/* use "self" */
> -	u64	device_id;
> -	struct hv_interrupt_entry int_entry;
> -	u64	reserved2;
> -	struct hv_device_interrupt_target int_target;
> -} __packed __aligned(8);
> -
>  /*
>   * Driver specific state.
>   */
> @@ -482,7 +452,7 @@ struct hv_pcibus_device {
>  	struct workqueue_struct *wq;
>  
>  	/* hypercall arg, must not cross page boundary */
> -	struct retarget_msi_interrupt retarget_msi_interrupt_params;
> +	struct hv_retarget_device_interrupt retarget_msi_interrupt_params;
>  
>  	/*
>  	 * Don't put anything here: retarget_msi_interrupt_params must be last
> @@ -1178,7 +1148,7 @@ static void hv_irq_unmask(struct irq_data *data)
>  {
>  	struct msi_desc *msi_desc = irq_data_get_msi_desc(data);
>  	struct irq_cfg *cfg = irqd_cfg(data);
> -	struct retarget_msi_interrupt *params;
> +	struct hv_retarget_device_interrupt *params;

pci-hyperv.c also makes use of retarget_msi_interrupt_lock - it's really clear
from this name what it protects, however your rename now makes this more
confusing.

Likewise there is a comment in hv_pci_probe that refers to
retarget_msi_interrupt_params which is now stale.

It may be helpful to rename hv_retarget_device_interrupt for consistency with
the docs - however please make sure you catch all the references - I'd suggest
that the move and the rename are in different patches.

Thanks,

Andrew Murray

>  	struct hv_pcibus_device *hbus;
>  	struct cpumask *dest;
>  	cpumask_var_t tmp;
> -- 
> 2.24.1
>
Boqun Feng Feb. 3, 2020, 2:09 p.m. UTC | #2
On Mon, Feb 03, 2020 at 09:41:18AM +0000, Andrew Murray wrote:
> On Mon, Feb 03, 2020 at 01:03:12PM +0800, Boqun Feng wrote:
> > Currently, retarget_msi_interrupt and other structures it relys on are
> > defined in pci-hyperv.c. However, those structures are actually defined
> > in Hypervisor Top-Level Functional Specification [1] and may be
> > different in sizes of fields or layout from architecture to
> > architecture. Therefore, this patch moves those definitions into x86's
> 
> Nit: Rather than 'Therefore, this patch moves ...' - how about 'Let's move
> ...'?
> 
> > tlfs header file to support virtual PCI on non-x86 architectures in the
> > future.
> > 
> > Besides, while I'm at it, rename retarget_msi_interrupt to
> 
> Nit: 'Besides, while I'm at it' - this type of wording describes what
> *you've* done rather than what the patch is doing. You could replace
> that quoted text with 'Additionally, '
> 
> > hv_retarget_msi_interrupt for the consistent name convention, also
> 
> Nit: s/name/naming
> 

Thanks for the suggestion on wording ;-)

> > mirroring the name in TLFS.
> > 
> > [1]: https://docs.microsoft.com/en-us/virtualization/hyper-v-on-windows/reference/tlfs
> > 
> > Signed-off-by: Boqun Feng (Microsoft) <boqun.feng@gmail.com>
> > ---
> >  arch/x86/include/asm/hyperv-tlfs.h  | 31 ++++++++++++++++++++++++++
> >  drivers/pci/controller/pci-hyperv.c | 34 ++---------------------------
> >  2 files changed, 33 insertions(+), 32 deletions(-)
> > 
> > diff --git a/arch/x86/include/asm/hyperv-tlfs.h b/arch/x86/include/asm/hyperv-tlfs.h
> > index 739bd89226a5..4a76e442481a 100644
> > --- a/arch/x86/include/asm/hyperv-tlfs.h
> > +++ b/arch/x86/include/asm/hyperv-tlfs.h
> > @@ -911,4 +911,35 @@ struct hv_tlb_flush_ex {
> >  struct hv_partition_assist_pg {
> >  	u32 tlb_lock_count;
> >  };
> > +
> > +struct hv_interrupt_entry {
> > +	u32 source;			/* 1 for MSI(-X) */
> > +	u32 reserved1;
> > +	u32 address;
> > +	u32 data;
> > +} __packed;
> 
> Why have you added __packed here? There is no mention of this change in the
> commit log? Is it needed?
> 

I'm simply following the convention of hyperv-tlfs.h: most of the
structures have this "__packed" attribute. I personally don't think this
attribute is necessary, but I was afraid that I was missing something
subtle. So a question for folks working on Hyper-V: why we need this
attribute on TLFS-defined structures? Most of those will have no
difference with or without this attribute, IIUC.

> > +
> > +/*
> > + * flags for hv_device_interrupt_target.flags
> > + */
> > +#define HV_DEVICE_INTERRUPT_TARGET_MULTICAST		1
> > +#define HV_DEVICE_INTERRUPT_TARGET_PROCESSOR_SET	2
> > +
> > +struct hv_device_interrupt_target {
> > +	u32 vector;
> > +	u32 flags;
> > +	union {
> > +		u64 vp_mask;
> > +		struct hv_vpset vp_set;
> > +	};
> > +} __packed;
> 
> Same here.
> 
> > +
> > +/* HvRetargetDeviceInterrupt hypercall */
> > +struct hv_retarget_device_interrupt {
> > +	u64 partition_id;
> 
> Why drop the 'self' comment?
> 

Good catch, TLFS does say this field must be 'self'. I will add it in
next version.

> > +	u64 device_id;
> > +	struct hv_interrupt_entry int_entry;
> > +	u64 reserved2;
> > +	struct hv_device_interrupt_target int_target;
> > +} __packed __aligned(8);
> >  #endif
> > diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
> > index aacfcc90d929..0d9b74503577 100644
> > --- a/drivers/pci/controller/pci-hyperv.c
> > +++ b/drivers/pci/controller/pci-hyperv.c
> > @@ -406,36 +406,6 @@ struct pci_eject_response {
> >  
> >  static int pci_ring_size = (4 * PAGE_SIZE);
> >  
> > -struct hv_interrupt_entry {
> > -	u32	source;			/* 1 for MSI(-X) */
> > -	u32	reserved1;
> > -	u32	address;
> > -	u32	data;
> > -};
> > -
> > -/*
> > - * flags for hv_device_interrupt_target.flags
> > - */
> > -#define HV_DEVICE_INTERRUPT_TARGET_MULTICAST		1
> > -#define HV_DEVICE_INTERRUPT_TARGET_PROCESSOR_SET	2
> > -
> > -struct hv_device_interrupt_target {
> > -	u32	vector;
> > -	u32	flags;
> > -	union {
> > -		u64		 vp_mask;
> > -		struct hv_vpset vp_set;
> > -	};
> > -};
> > -
> > -struct retarget_msi_interrupt {
> > -	u64	partition_id;		/* use "self" */
> > -	u64	device_id;
> > -	struct hv_interrupt_entry int_entry;
> > -	u64	reserved2;
> > -	struct hv_device_interrupt_target int_target;
> > -} __packed __aligned(8);
> > -
> >  /*
> >   * Driver specific state.
> >   */
> > @@ -482,7 +452,7 @@ struct hv_pcibus_device {
> >  	struct workqueue_struct *wq;
> >  
> >  	/* hypercall arg, must not cross page boundary */
> > -	struct retarget_msi_interrupt retarget_msi_interrupt_params;
> > +	struct hv_retarget_device_interrupt retarget_msi_interrupt_params;
> >  
> >  	/*
> >  	 * Don't put anything here: retarget_msi_interrupt_params must be last
> > @@ -1178,7 +1148,7 @@ static void hv_irq_unmask(struct irq_data *data)
> >  {
> >  	struct msi_desc *msi_desc = irq_data_get_msi_desc(data);
> >  	struct irq_cfg *cfg = irqd_cfg(data);
> > -	struct retarget_msi_interrupt *params;
> > +	struct hv_retarget_device_interrupt *params;
> 
> pci-hyperv.c also makes use of retarget_msi_interrupt_lock - it's really clear
> from this name what it protects, however your rename now makes this more
> confusing.
> 
> Likewise there is a comment in hv_pci_probe that refers to
> retarget_msi_interrupt_params which is now stale.
> 

But 'retarget_msi_interrupt_params' is the name of field in
hv_pcibus_device, so is 'retarget_msi_interrupt_lock'. And what I change
is the name of type. I believe people can tell the relationship from
the name of the fields, and the comment of hv_pci_probe actually refers
to the field rather than the type.

> It may be helpful to rename hv_retarget_device_interrupt for consistency with
> the docs - however please make sure you catch all the references - I'd suggest
> that the move and the rename are in different patches.
> 

If the renaming requires a lot of work (e.g. need to change multiple
references), I will follow your suggestion. But seems it's not the case
for this renaming.

Regards,
Boqun

> Thanks,
> 
> Andrew Murray
> 
> >  	struct hv_pcibus_device *hbus;
> >  	struct cpumask *dest;
> >  	cpumask_var_t tmp;
> > -- 
> > 2.24.1
> >
Boqun Feng Feb. 7, 2020, 7:58 a.m. UTC | #3
On Mon, Feb 03, 2020 at 10:09:02PM +0800, Boqun Feng wrote:
[...]
> > > mirroring the name in TLFS.
> > > 
> > > [1]: https://docs.microsoft.com/en-us/virtualization/hyper-v-on-windows/reference/tlfs
> > > 
> > > Signed-off-by: Boqun Feng (Microsoft) <boqun.feng@gmail.com>
> > > ---
> > >  arch/x86/include/asm/hyperv-tlfs.h  | 31 ++++++++++++++++++++++++++
> > >  drivers/pci/controller/pci-hyperv.c | 34 ++---------------------------
> > >  2 files changed, 33 insertions(+), 32 deletions(-)
> > > 
> > > diff --git a/arch/x86/include/asm/hyperv-tlfs.h b/arch/x86/include/asm/hyperv-tlfs.h
> > > index 739bd89226a5..4a76e442481a 100644
> > > --- a/arch/x86/include/asm/hyperv-tlfs.h
> > > +++ b/arch/x86/include/asm/hyperv-tlfs.h
> > > @@ -911,4 +911,35 @@ struct hv_tlb_flush_ex {
> > >  struct hv_partition_assist_pg {
> > >  	u32 tlb_lock_count;
> > >  };
> > > +
> > > +struct hv_interrupt_entry {
> > > +	u32 source;			/* 1 for MSI(-X) */
> > > +	u32 reserved1;
> > > +	u32 address;
> > > +	u32 data;
> > > +} __packed;
> > 
> > Why have you added __packed here? There is no mention of this change in the
> > commit log? Is it needed?
> > 
> 
> I'm simply following the convention of hyperv-tlfs.h: most of the
> structures have this "__packed" attribute. I personally don't think this
> attribute is necessary, but I was afraid that I was missing something
> subtle. So a question for folks working on Hyper-V: why we need this
> attribute on TLFS-defined structures? Most of those will have no
> difference with or without this attribute, IIUC.
> 

I find this patch:

	https://lore.kernel.org/lkml/20181212175701.18754-1-vkuznets@redhat.com/

The reason why the "__packed" attribute is needed is to protect the
hypervisor-guet communication structures from unexpected behaviors of
compilers.

I will keep the code as it is and add some words in the commit log.

Regards,
Boqun

> > > +
> > > +/*
> > > + * flags for hv_device_interrupt_target.flags
> > > + */
> > > +#define HV_DEVICE_INTERRUPT_TARGET_MULTICAST		1
> > > +#define HV_DEVICE_INTERRUPT_TARGET_PROCESSOR_SET	2
> > > +
> > > +struct hv_device_interrupt_target {
> > > +	u32 vector;
> > > +	u32 flags;
> > > +	union {
> > > +		u64 vp_mask;
> > > +		struct hv_vpset vp_set;
> > > +	};
> > > +} __packed;
> > 
> > Same here.
> > 
> > > +
> > > +/* HvRetargetDeviceInterrupt hypercall */
> > > +struct hv_retarget_device_interrupt {
> > > +	u64 partition_id;
> > 
> > Why drop the 'self' comment?
> > 
> 
> Good catch, TLFS does say this field must be 'self'. I will add it in
> next version.
> 
> > > +	u64 device_id;
> > > +	struct hv_interrupt_entry int_entry;
> > > +	u64 reserved2;
> > > +	struct hv_device_interrupt_target int_target;
> > > +} __packed __aligned(8);
> > >  #endif
> > > diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
> > > index aacfcc90d929..0d9b74503577 100644
> > > --- a/drivers/pci/controller/pci-hyperv.c
> > > +++ b/drivers/pci/controller/pci-hyperv.c
> > > @@ -406,36 +406,6 @@ struct pci_eject_response {
> > >  
> > >  static int pci_ring_size = (4 * PAGE_SIZE);
> > >  
> > > -struct hv_interrupt_entry {
> > > -	u32	source;			/* 1 for MSI(-X) */
> > > -	u32	reserved1;
> > > -	u32	address;
> > > -	u32	data;
> > > -};
> > > -
> > > -/*
> > > - * flags for hv_device_interrupt_target.flags
> > > - */
> > > -#define HV_DEVICE_INTERRUPT_TARGET_MULTICAST		1
> > > -#define HV_DEVICE_INTERRUPT_TARGET_PROCESSOR_SET	2
> > > -
> > > -struct hv_device_interrupt_target {
> > > -	u32	vector;
> > > -	u32	flags;
> > > -	union {
> > > -		u64		 vp_mask;
> > > -		struct hv_vpset vp_set;
> > > -	};
> > > -};
> > > -
> > > -struct retarget_msi_interrupt {
> > > -	u64	partition_id;		/* use "self" */
> > > -	u64	device_id;
> > > -	struct hv_interrupt_entry int_entry;
> > > -	u64	reserved2;
> > > -	struct hv_device_interrupt_target int_target;
> > > -} __packed __aligned(8);
> > > -
> > >  /*
> > >   * Driver specific state.
> > >   */
> > > @@ -482,7 +452,7 @@ struct hv_pcibus_device {
> > >  	struct workqueue_struct *wq;
> > >  
> > >  	/* hypercall arg, must not cross page boundary */
> > > -	struct retarget_msi_interrupt retarget_msi_interrupt_params;
> > > +	struct hv_retarget_device_interrupt retarget_msi_interrupt_params;
> > >  
> > >  	/*
> > >  	 * Don't put anything here: retarget_msi_interrupt_params must be last
> > > @@ -1178,7 +1148,7 @@ static void hv_irq_unmask(struct irq_data *data)
> > >  {
> > >  	struct msi_desc *msi_desc = irq_data_get_msi_desc(data);
> > >  	struct irq_cfg *cfg = irqd_cfg(data);
> > > -	struct retarget_msi_interrupt *params;
> > > +	struct hv_retarget_device_interrupt *params;
> > 
> > pci-hyperv.c also makes use of retarget_msi_interrupt_lock - it's really clear
> > from this name what it protects, however your rename now makes this more
> > confusing.
> > 
> > Likewise there is a comment in hv_pci_probe that refers to
> > retarget_msi_interrupt_params which is now stale.
> > 
> 
> But 'retarget_msi_interrupt_params' is the name of field in
> hv_pcibus_device, so is 'retarget_msi_interrupt_lock'. And what I change
> is the name of type. I believe people can tell the relationship from
> the name of the fields, and the comment of hv_pci_probe actually refers
> to the field rather than the type.
> 
> > It may be helpful to rename hv_retarget_device_interrupt for consistency with
> > the docs - however please make sure you catch all the references - I'd suggest
> > that the move and the rename are in different patches.
> > 
> 
> If the renaming requires a lot of work (e.g. need to change multiple
> references), I will follow your suggestion. But seems it's not the case
> for this renaming.
> 
> Regards,
> Boqun
> 
> > Thanks,
> > 
> > Andrew Murray
> > 
> > >  	struct hv_pcibus_device *hbus;
> > >  	struct cpumask *dest;
> > >  	cpumask_var_t tmp;
> > > -- 
> > > 2.24.1
> > >
diff mbox series

Patch

diff --git a/arch/x86/include/asm/hyperv-tlfs.h b/arch/x86/include/asm/hyperv-tlfs.h
index 739bd89226a5..4a76e442481a 100644
--- a/arch/x86/include/asm/hyperv-tlfs.h
+++ b/arch/x86/include/asm/hyperv-tlfs.h
@@ -911,4 +911,35 @@  struct hv_tlb_flush_ex {
 struct hv_partition_assist_pg {
 	u32 tlb_lock_count;
 };
+
+struct hv_interrupt_entry {
+	u32 source;			/* 1 for MSI(-X) */
+	u32 reserved1;
+	u32 address;
+	u32 data;
+} __packed;
+
+/*
+ * flags for hv_device_interrupt_target.flags
+ */
+#define HV_DEVICE_INTERRUPT_TARGET_MULTICAST		1
+#define HV_DEVICE_INTERRUPT_TARGET_PROCESSOR_SET	2
+
+struct hv_device_interrupt_target {
+	u32 vector;
+	u32 flags;
+	union {
+		u64 vp_mask;
+		struct hv_vpset vp_set;
+	};
+} __packed;
+
+/* HvRetargetDeviceInterrupt hypercall */
+struct hv_retarget_device_interrupt {
+	u64 partition_id;
+	u64 device_id;
+	struct hv_interrupt_entry int_entry;
+	u64 reserved2;
+	struct hv_device_interrupt_target int_target;
+} __packed __aligned(8);
 #endif
diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
index aacfcc90d929..0d9b74503577 100644
--- a/drivers/pci/controller/pci-hyperv.c
+++ b/drivers/pci/controller/pci-hyperv.c
@@ -406,36 +406,6 @@  struct pci_eject_response {
 
 static int pci_ring_size = (4 * PAGE_SIZE);
 
-struct hv_interrupt_entry {
-	u32	source;			/* 1 for MSI(-X) */
-	u32	reserved1;
-	u32	address;
-	u32	data;
-};
-
-/*
- * flags for hv_device_interrupt_target.flags
- */
-#define HV_DEVICE_INTERRUPT_TARGET_MULTICAST		1
-#define HV_DEVICE_INTERRUPT_TARGET_PROCESSOR_SET	2
-
-struct hv_device_interrupt_target {
-	u32	vector;
-	u32	flags;
-	union {
-		u64		 vp_mask;
-		struct hv_vpset vp_set;
-	};
-};
-
-struct retarget_msi_interrupt {
-	u64	partition_id;		/* use "self" */
-	u64	device_id;
-	struct hv_interrupt_entry int_entry;
-	u64	reserved2;
-	struct hv_device_interrupt_target int_target;
-} __packed __aligned(8);
-
 /*
  * Driver specific state.
  */
@@ -482,7 +452,7 @@  struct hv_pcibus_device {
 	struct workqueue_struct *wq;
 
 	/* hypercall arg, must not cross page boundary */
-	struct retarget_msi_interrupt retarget_msi_interrupt_params;
+	struct hv_retarget_device_interrupt retarget_msi_interrupt_params;
 
 	/*
 	 * Don't put anything here: retarget_msi_interrupt_params must be last
@@ -1178,7 +1148,7 @@  static void hv_irq_unmask(struct irq_data *data)
 {
 	struct msi_desc *msi_desc = irq_data_get_msi_desc(data);
 	struct irq_cfg *cfg = irqd_cfg(data);
-	struct retarget_msi_interrupt *params;
+	struct hv_retarget_device_interrupt *params;
 	struct hv_pcibus_device *hbus;
 	struct cpumask *dest;
 	cpumask_var_t tmp;