[PATCH v4 0/2] PCI: hv: Refactor hv_irq_unmask() to use hv_vpset and cpumask_to_vpset()

[PATCH v4 0/2] PCI: hv: Refactor hv_irq_unmask() to use hv_vpset and cpumask_to_vpset()

[Maya Nakamura]

This patchset removes a duplicate definition of VP set (hv_vp_set) and
uses the common definition (hv_vpset) that is used in other places. It
changes the order of the members in struct hv_pcibus_device due to
flexible array in hv_vpset.

It also removes the duplicate implementation of cpumask_to_vpset(), uses
the shared implementation, and exports hv_max_vp_index, which is
required by cpumask_to_vpset().

Based on Vitaly's findings, two changes were applied: replace GFP_KERNEL
with GFP_ATOMIC for alloc_cpumask_var() because hv_irq_unmask() runs
while a spinlock is held, and add __aligned(8) to struct
retarget_msi_interrupt because Hyper-V requires that hypercall arguments
be aligned on an 8 byte boundary.

Vitaly, thank you for finding the issues, and Lorenzo and Michael, thank
you for your guidance and support!

Maya Nakamura (2):
  PCI: hv: Replace hv_vp_set with hv_vpset
  PCI: hv: Refactor hv_irq_unmask() to use cpumask_to_vpset()

 arch/x86/hyperv/hv_init.c           |  1 +
 drivers/pci/controller/pci-hyperv.c | 61 +++++++++++++----------------
 2 files changed, 29 insertions(+), 33 deletions(-)

-- 
2.17.1

[PATCH v4 1/2] PCI: hv: Replace hv_vp_set with hv_vpset

[Maya Nakamura]

Remove a duplicate definition of VP set (hv_vp_set) and use the common
definition (hv_vpset) that is used in other places.

Change the order of the members in struct hv_pcibus_device so that the
declaration of retarget_msi_interrupt_params is the last member. Struct
hv_vpset, which contains a flexible array, is nested two levels deep in
struct hv_pcibus_device via retarget_msi_interrupt_params.

Add a comment that retarget_msi_interrupt_params should be the last
member of struct hv_pcibus_device.

Based on Vitaly's finding, add __aligned(8) to struct
retarget_msi_interrupt because Hyper-V requires that hypercall arguments
be aligned on an 8 byte boundary.

Signed-off-by: Maya Nakamura <m.maya.nakamura@gmail.com>
---
Changes in v4:
- Add __aligned(8) to struct retarget_msi_interrupt.
- Update the commit message.

Change in v3:
- Correct the v2 change log.

Change in v2:
- Update the commit message.

 drivers/pci/controller/pci-hyperv.c | 27 +++++++++++++--------------
 1 file changed, 13 insertions(+), 14 deletions(-)

diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
index 9ba4d12c179c..d71695db1ba0 100644
--- a/drivers/pci/controller/pci-hyperv.c
+++ b/drivers/pci/controller/pci-hyperv.c
@@ -393,12 +393,6 @@ struct hv_interrupt_entry {
 
 #define HV_VP_SET_BANK_COUNT_MAX	5 /* current implementation limit */
 
-struct hv_vp_set {
-	u64	format;			/* 0 (HvGenericSetSparse4k) */
-	u64	valid_banks;
-	u64	masks[HV_VP_SET_BANK_COUNT_MAX];
-};
-
 /*
  * flags for hv_device_interrupt_target.flags
  */
@@ -410,7 +404,7 @@ struct hv_device_interrupt_target {
 	u32	flags;
 	union {
 		u64		 vp_mask;
-		struct hv_vp_set vp_set;
+		struct hv_vpset vp_set;
 	};
 };
 
@@ -420,7 +414,7 @@ struct retarget_msi_interrupt {
 	struct hv_interrupt_entry int_entry;
 	u64	reserved2;
 	struct hv_device_interrupt_target int_target;
-} __packed;
+} __packed __aligned(8);
 
 /*
  * Driver specific state.
@@ -460,12 +454,16 @@ struct hv_pcibus_device {
 	struct msi_controller msi_chip;
 	struct irq_domain *irq_domain;
 
-	/* hypercall arg, must not cross page boundary */
-	struct retarget_msi_interrupt retarget_msi_interrupt_params;
-
 	spinlock_t retarget_msi_interrupt_lock;
 
 	struct workqueue_struct *wq;
+
+	/* hypercall arg, must not cross page boundary */
+	struct retarget_msi_interrupt retarget_msi_interrupt_params;
+
+	/*
+	 * Don't put anything here: retarget_msi_interrupt_params must be last
+	 */
 };
 
 /*
@@ -955,12 +953,13 @@ static void hv_irq_unmask(struct irq_data *data)
 		 */
 		params->int_target.flags |=
 			HV_DEVICE_INTERRUPT_TARGET_PROCESSOR_SET;
-		params->int_target.vp_set.valid_banks =
+		params->int_target.vp_set.valid_bank_mask =
 			(1ull << HV_VP_SET_BANK_COUNT_MAX) - 1;
 
 		/*
 		 * var-sized hypercall, var-size starts after vp_mask (thus
-		 * vp_set.format does not count, but vp_set.valid_banks does).
+		 * vp_set.format does not count, but vp_set.valid_bank_mask
+		 * does).
 		 */
 		var_size = 1 + HV_VP_SET_BANK_COUNT_MAX;
 
@@ -974,7 +973,7 @@ static void hv_irq_unmask(struct irq_data *data)
 				goto exit_unlock;
 			}
 
-			params->int_target.vp_set.masks[cpu_vmbus / 64] |=
+			params->int_target.vp_set.bank_contents[cpu_vmbus / 64]	|=
 				(1ULL << (cpu_vmbus & 63));
 		}
 	} else {
-- 
2.17.1

[PATCH v4 2/2] PCI: hv: Refactor hv_irq_unmask() to use cpumask_to_vpset()

[Maya Nakamura]

Remove the duplicate implementation of cpumask_to_vpset() and use the
shared implementation. Export hv_max_vp_index, which is required by
cpumask_to_vpset().

Apply changes to hv_irq_unmask() based on feedback.

Based on Vitaly's finding, use GFP_ATOMIC instead of GFP_KERNEL for
alloc_cpumask_var() because hv_irq_unmask() runs while a spinlock is
held.

Signed-off-by: Maya Nakamura <m.maya.nakamura@gmail.com>
---
Changes in v4:
 - Replace GFP_KERNEL with GFP_ATOMIC for alloc_cpumask_var().
 - Update the commit message.

Changes in v3:
 - Modify to catch all failures from cpumask_to_vpset().
 - Correct the v2 change log about the commit message.

Changes in v2:
 - Remove unnecessary nr_bank initialization.
 - Delete two unnecessary dev_err()'s.
 - Unlock before returning.
 - Update the commit message.

 arch/x86/hyperv/hv_init.c           |  1 +
 drivers/pci/controller/pci-hyperv.c | 38 +++++++++++++----------------
 2 files changed, 18 insertions(+), 21 deletions(-)

diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
index 7abb09e2eeb8..7f2eed1fc81b 100644
--- a/arch/x86/hyperv/hv_init.c
+++ b/arch/x86/hyperv/hv_init.c
@@ -96,6 +96,7 @@ void  __percpu **hyperv_pcpu_input_arg;
 EXPORT_SYMBOL_GPL(hyperv_pcpu_input_arg);
 
 u32 hv_max_vp_index;
+EXPORT_SYMBOL_GPL(hv_max_vp_index);
 
 static int hv_cpu_init(unsigned int cpu)
 {
diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
index d71695db1ba0..95441a35eceb 100644
--- a/drivers/pci/controller/pci-hyperv.c
+++ b/drivers/pci/controller/pci-hyperv.c
@@ -391,8 +391,6 @@ struct hv_interrupt_entry {
 	u32	data;
 };
 
-#define HV_VP_SET_BANK_COUNT_MAX	5 /* current implementation limit */
-
 /*
  * flags for hv_device_interrupt_target.flags
  */
@@ -908,12 +906,12 @@ static void hv_irq_unmask(struct irq_data *data)
 	struct retarget_msi_interrupt *params;
 	struct hv_pcibus_device *hbus;
 	struct cpumask *dest;
+	cpumask_var_t tmp;
 	struct pci_bus *pbus;
 	struct pci_dev *pdev;
 	unsigned long flags;
 	u32 var_size = 0;
-	int cpu_vmbus;
-	int cpu;
+	int cpu, nr_bank;
 	u64 res;
 
 	dest = irq_data_get_effective_affinity_mask(data);
@@ -953,29 +951,27 @@ static void hv_irq_unmask(struct irq_data *data)
 		 */
 		params->int_target.flags |=
 			HV_DEVICE_INTERRUPT_TARGET_PROCESSOR_SET;
-		params->int_target.vp_set.valid_bank_mask =
-			(1ull << HV_VP_SET_BANK_COUNT_MAX) - 1;
+
+		if (!alloc_cpumask_var(&tmp, GFP_ATOMIC)) {
+			res = 1;
+			goto exit_unlock;
+		}
+
+		cpumask_and(tmp, dest, cpu_online_mask);
+		nr_bank = cpumask_to_vpset(&params->int_target.vp_set, tmp);
+		free_cpumask_var(tmp);
+
+		if (nr_bank <= 0) {
+			res = 1;
+			goto exit_unlock;
+		}
 
 		/*
 		 * var-sized hypercall, var-size starts after vp_mask (thus
 		 * vp_set.format does not count, but vp_set.valid_bank_mask
 		 * does).
 		 */
-		var_size = 1 + HV_VP_SET_BANK_COUNT_MAX;
-
-		for_each_cpu_and(cpu, dest, cpu_online_mask) {
-			cpu_vmbus = hv_cpu_number_to_vp_number(cpu);
-
-			if (cpu_vmbus >= HV_VP_SET_BANK_COUNT_MAX * 64) {
-				dev_err(&hbus->hdev->device,
-					"too high CPU %d", cpu_vmbus);
-				res = 1;
-				goto exit_unlock;
-			}
-
-			params->int_target.vp_set.bank_contents[cpu_vmbus / 64]	|=
-				(1ULL << (cpu_vmbus & 63));
-		}
+		var_size = 1 + nr_bank;
 	} else {
 		for_each_cpu_and(cpu, dest, cpu_online_mask) {
 			params->int_target.vp_mask |=
-- 
2.17.1

Re: [PATCH v4 1/2] PCI: hv: Replace hv_vp_set with hv_vpset

[Lorenzo Pieralisi]

On Thu, Feb 28, 2019 at 02:35:06AM +0000, Maya Nakamura wrote:
> Remove a duplicate definition of VP set (hv_vp_set) and use the common
> definition (hv_vpset) that is used in other places.
> 
> Change the order of the members in struct hv_pcibus_device so that the
> declaration of retarget_msi_interrupt_params is the last member. Struct
> hv_vpset, which contains a flexible array, is nested two levels deep in
> struct hv_pcibus_device via retarget_msi_interrupt_params.
> 
> Add a comment that retarget_msi_interrupt_params should be the last
> member of struct hv_pcibus_device.
> 
> Based on Vitaly's finding, add __aligned(8) to struct
> retarget_msi_interrupt because Hyper-V requires that hypercall arguments
> be aligned on an 8 byte boundary.

Hi Maya,

a couple of comments:

1) Please always carry review tags, I can do it for you but you would
   make my life easier.
2) Vitaly's comment should be addressed in a separate patch that should
   come before this patch in chronological order, as he said, it is not
   an issue with this patch per-se.
3) "Based on Vitaly's finding" is meaningless to a commit log reader.
   Please add a "Link:" tag pointing at the relevant mailing list
   discussion.

Comments (1)-(3) apply to patch 2 in this series as well.

Update the patches with my comments above and if Vitaly ACKs them I
will try to merge them for v5.1 but this must be made very promptly.

Lorenzo

> Signed-off-by: Maya Nakamura <m.maya.nakamura@gmail.com>
> ---
> Changes in v4:
> - Add __aligned(8) to struct retarget_msi_interrupt.
> - Update the commit message.
> 
> Change in v3:
> - Correct the v2 change log.
> 
> Change in v2:
> - Update the commit message.
> 
>  drivers/pci/controller/pci-hyperv.c | 27 +++++++++++++--------------
>  1 file changed, 13 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
> index 9ba4d12c179c..d71695db1ba0 100644
> --- a/drivers/pci/controller/pci-hyperv.c
> +++ b/drivers/pci/controller/pci-hyperv.c
> @@ -393,12 +393,6 @@ struct hv_interrupt_entry {
>  
>  #define HV_VP_SET_BANK_COUNT_MAX	5 /* current implementation limit */
>  
> -struct hv_vp_set {
> -	u64	format;			/* 0 (HvGenericSetSparse4k) */
> -	u64	valid_banks;
> -	u64	masks[HV_VP_SET_BANK_COUNT_MAX];
> -};
> -
>  /*
>   * flags for hv_device_interrupt_target.flags
>   */
> @@ -410,7 +404,7 @@ struct hv_device_interrupt_target {
>  	u32	flags;
>  	union {
>  		u64		 vp_mask;
> -		struct hv_vp_set vp_set;
> +		struct hv_vpset vp_set;
>  	};
>  };
>  
> @@ -420,7 +414,7 @@ struct retarget_msi_interrupt {
>  	struct hv_interrupt_entry int_entry;
>  	u64	reserved2;
>  	struct hv_device_interrupt_target int_target;
> -} __packed;
> +} __packed __aligned(8);
>  
>  /*
>   * Driver specific state.
> @@ -460,12 +454,16 @@ struct hv_pcibus_device {
>  	struct msi_controller msi_chip;
>  	struct irq_domain *irq_domain;
>  
> -	/* hypercall arg, must not cross page boundary */
> -	struct retarget_msi_interrupt retarget_msi_interrupt_params;
> -
>  	spinlock_t retarget_msi_interrupt_lock;
>  
>  	struct workqueue_struct *wq;
> +
> +	/* hypercall arg, must not cross page boundary */
> +	struct retarget_msi_interrupt retarget_msi_interrupt_params;
> +
> +	/*
> +	 * Don't put anything here: retarget_msi_interrupt_params must be last
> +	 */
>  };
>  
>  /*
> @@ -955,12 +953,13 @@ static void hv_irq_unmask(struct irq_data *data)
>  		 */
>  		params->int_target.flags |=
>  			HV_DEVICE_INTERRUPT_TARGET_PROCESSOR_SET;
> -		params->int_target.vp_set.valid_banks =
> +		params->int_target.vp_set.valid_bank_mask =
>  			(1ull << HV_VP_SET_BANK_COUNT_MAX) - 1;
>  
>  		/*
>  		 * var-sized hypercall, var-size starts after vp_mask (thus
> -		 * vp_set.format does not count, but vp_set.valid_banks does).
> +		 * vp_set.format does not count, but vp_set.valid_bank_mask
> +		 * does).
>  		 */
>  		var_size = 1 + HV_VP_SET_BANK_COUNT_MAX;
>  
> @@ -974,7 +973,7 @@ static void hv_irq_unmask(struct irq_data *data)
>  				goto exit_unlock;
>  			}
>  
> -			params->int_target.vp_set.masks[cpu_vmbus / 64] |=
> +			params->int_target.vp_set.bank_contents[cpu_vmbus / 64]	|=
>  				(1ULL << (cpu_vmbus & 63));
>  		}
>  	} else {
> -- 
> 2.17.1
> 

Re: [PATCH v4 0/2] PCI: hv: Refactor hv_irq_unmask() to use hv_vpset and cpumask_to_vpset()

[Vitaly Kuznetsov]

Maya Nakamura <m.maya.nakamura@gmail.com> writes:

> This patchset removes a duplicate definition of VP set (hv_vp_set) and
> uses the common definition (hv_vpset) that is used in other places. It
> changes the order of the members in struct hv_pcibus_device due to
> flexible array in hv_vpset.
>
> It also removes the duplicate implementation of cpumask_to_vpset(), uses
> the shared implementation, and exports hv_max_vp_index, which is
> required by cpumask_to_vpset().
>
> Based on Vitaly's findings, two changes were applied: replace GFP_KERNEL
> with GFP_ATOMIC for alloc_cpumask_var() because hv_irq_unmask() runs
> while a spinlock is held, and add __aligned(8) to struct
> retarget_msi_interrupt because Hyper-V requires that hypercall arguments
> be aligned on an 8 byte boundary.
>
> Vitaly, thank you for finding the issues, and Lorenzo and Michael, thank
> you for your guidance and support!
>

Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>
Tested-by: Vitaly Kuznetsov <vkuznets@redhat.com>

I, however, agree with Lorenzo that '__aligned(8)' change deserves its
own patch as it fixes an already present issue (which may actually be
masked but still present).

Going forward I'd like to get rid of the newly added cpumask allocation:
while it is very unlikely to fail and it only happens when we reassign
IRQs to some different CPU (thus it's not a hotpath or anything)
failures are still possible. This work can definitely be postponed, I
don't think it should block your patches.

-- 
Vitaly