linux-pci.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 0/3] PCI: hv: Generify pci-hyperv.c
@ 2020-02-10  3:39 Boqun Feng
  2020-02-10  3:39 ` [PATCH v3 1/3] PCI: hv: Move hypercall related definitions into tlfs header Boqun Feng
                   ` (3 more replies)
  0 siblings, 4 replies; 15+ messages in thread
From: Boqun Feng @ 2020-02-10  3:39 UTC (permalink / raw)
  To: linux-pci, linux-hyperv, linux-kernel, linux-arm-kernel
  Cc: Michael Kelley, K. Y. Srinivasan, Haiyang Zhang,
	Stephen Hemminger, Sasha Levin, Thomas Gleixner, Ingo Molnar,
	Borislav Petkov, H. Peter Anvin, x86, Lorenzo Pieralisi,
	Andrew Murray, Bjorn Helgaas, Boqun Feng

Hi,

This is the first part for virtual PCI support of Hyper-V guest on
ARM64. The whole patchset doesn't have any functional change, but only
refactors the pci-hyperv.c code to make it more arch-independent.

Previous version:
v1: https://lore.kernel.org/lkml/20200121015713.69691-1-boqun.feng@gmail.com/
v2: https://lore.kernel.org/linux-arm-kernel/20200203050313.69247-1-boqun.feng@gmail.com/

Changes since v2:

*	Rebased on 5.6-rc1

*	Reword commit logs as per Andrew's suggestion.

*	It makes more sense to have a generic interface to set the whole
	msi_entry rather than only the "address" field. So change
	hv_set_msi_address_from_desc() to hv_set_msi_entry_from_desc().
	Additionally, make it an inline function as per the suggestion
	of Andrew and Thomas.

*	Add the missing comment saying the partition_id of
	hv_retarget_device_interrupt must be self.

*	Add the explanation for why "__packed" is needed for TLFS
	structures.

I've done compile and boot test of this patchset, also done some tests
with a pass-through NVMe device.

Suggestions and comments are welcome!

Regards,
Boqun

Boqun Feng (3):
  PCI: hv: Move hypercall related definitions into tlfs header
  PCI: hv: Move retarget related structures into tlfs header
  PCI: hv: Introduce hv_msi_entry

 arch/x86/include/asm/hyperv-tlfs.h  | 41 +++++++++++++++++++++++++++
 arch/x86/include/asm/mshyperv.h     |  8 ++++++
 drivers/pci/controller/pci-hyperv.c | 43 ++---------------------------
 3 files changed, 52 insertions(+), 40 deletions(-)

-- 
2.24.1


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v3 1/3] PCI: hv: Move hypercall related definitions into tlfs header
  2020-02-10  3:39 [PATCH v3 0/3] PCI: hv: Generify pci-hyperv.c Boqun Feng
@ 2020-02-10  3:39 ` Boqun Feng
  2020-02-13  4:17   ` Dexuan Cui
  2020-02-10  3:39 ` [PATCH v3 2/3] PCI: hv: Move retarget related structures " Boqun Feng
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 15+ messages in thread
From: Boqun Feng @ 2020-02-10  3:39 UTC (permalink / raw)
  To: linux-pci, linux-hyperv, linux-kernel, linux-arm-kernel
  Cc: Michael Kelley, K. Y. Srinivasan, Haiyang Zhang,
	Stephen Hemminger, Sasha Levin, Thomas Gleixner, Ingo Molnar,
	Borislav Petkov, H. Peter Anvin, x86, Lorenzo Pieralisi,
	Andrew Murray, Bjorn Helgaas, Boqun Feng, Andrew Murray

Currently HVCALL_RETARGET_INTERRUPT and HV_PARTITION_ID_SELF are defined
in pci-hyperv.c. However, similar to other hypercall related
definitions, it makes more sense to put them in the tlfs header file.

Besides, these definitions are arch-dependent, so for the support of
virtual PCI on non-x86 archs in the future, move them into arch-specific
tlfs header file.

Signed-off-by: Boqun Feng (Microsoft) <boqun.feng@gmail.com>
Reviewed-by: Andrew Murray <amurray@thegoodpenguin.co.uk>
---
 arch/x86/include/asm/hyperv-tlfs.h  | 3 +++
 drivers/pci/controller/pci-hyperv.c | 6 ------
 2 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/arch/x86/include/asm/hyperv-tlfs.h b/arch/x86/include/asm/hyperv-tlfs.h
index 92abc1e42bfc..dffed0e10a68 100644
--- a/arch/x86/include/asm/hyperv-tlfs.h
+++ b/arch/x86/include/asm/hyperv-tlfs.h
@@ -376,6 +376,7 @@ struct hv_tsc_emulation_status {
 #define HVCALL_SEND_IPI_EX			0x0015
 #define HVCALL_POST_MESSAGE			0x005c
 #define HVCALL_SIGNAL_EVENT			0x005d
+#define HVCALL_RETARGET_INTERRUPT		0x007e
 #define HVCALL_FLUSH_GUEST_PHYSICAL_ADDRESS_SPACE 0x00af
 #define HVCALL_FLUSH_GUEST_PHYSICAL_ADDRESS_LIST 0x00b0
 
@@ -405,6 +406,8 @@ enum HV_GENERIC_SET_FORMAT {
 	HV_GENERIC_SET_ALL,
 };
 
+#define HV_PARTITION_ID_SELF                    ((u64)-1)
+
 #define HV_HYPERCALL_RESULT_MASK	GENMASK_ULL(15, 0)
 #define HV_HYPERCALL_FAST_BIT		BIT(16)
 #define HV_HYPERCALL_VARHEAD_OFFSET	17
diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
index 9977abff92fc..aacfcc90d929 100644
--- a/drivers/pci/controller/pci-hyperv.c
+++ b/drivers/pci/controller/pci-hyperv.c
@@ -406,12 +406,6 @@ struct pci_eject_response {
 
 static int pci_ring_size = (4 * PAGE_SIZE);
 
-/*
- * Definitions or interrupt steering hypercall.
- */
-#define HV_PARTITION_ID_SELF		((u64)-1)
-#define HVCALL_RETARGET_INTERRUPT	0x7e
-
 struct hv_interrupt_entry {
 	u32	source;			/* 1 for MSI(-X) */
 	u32	reserved1;
-- 
2.24.1


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v3 2/3] PCI: hv: Move retarget related structures into tlfs header
  2020-02-10  3:39 [PATCH v3 0/3] PCI: hv: Generify pci-hyperv.c Boqun Feng
  2020-02-10  3:39 ` [PATCH v3 1/3] PCI: hv: Move hypercall related definitions into tlfs header Boqun Feng
@ 2020-02-10  3:39 ` Boqun Feng
  2020-02-13  4:17   ` Dexuan Cui
  2020-02-10  3:39 ` [PATCH v3 3/3] PCI: hv: Introduce hv_msi_entry Boqun Feng
  2020-02-21  2:33 ` [PATCH v3 0/3] PCI: hv: Generify pci-hyperv.c Boqun Feng
  3 siblings, 1 reply; 15+ messages in thread
From: Boqun Feng @ 2020-02-10  3:39 UTC (permalink / raw)
  To: linux-pci, linux-hyperv, linux-kernel, linux-arm-kernel
  Cc: Michael Kelley, K. Y. Srinivasan, Haiyang Zhang,
	Stephen Hemminger, Sasha Levin, Thomas Gleixner, Ingo Molnar,
	Borislav Petkov, H. Peter Anvin, x86, Lorenzo Pieralisi,
	Andrew Murray, Bjorn Helgaas, Boqun Feng, Andrew Murray

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. Let's move those definitions into x86's tlfs header file
to support virtual PCI on non-x86 architectures in the future. Note that
"__packed" attribute is added to these structures during the movement
for the same reason as we use the attribute for other TLFS structures in
the header file: make sure the structures meet the specification and
avoid anything unexpected from the compilers.

Additionally, rename struct retarget_msi_interrupt to
hv_retarget_msi_interrupt for the consistent naming 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(-)

diff --git a/arch/x86/include/asm/hyperv-tlfs.h b/arch/x86/include/asm/hyperv-tlfs.h
index dffed0e10a68..a0b6a88d2f05 100644
--- a/arch/x86/include/asm/hyperv-tlfs.h
+++ b/arch/x86/include/asm/hyperv-tlfs.h
@@ -912,4 +912,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;		/* use "self" */
+	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;
-- 
2.24.1


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v3 3/3] PCI: hv: Introduce hv_msi_entry
  2020-02-10  3:39 [PATCH v3 0/3] PCI: hv: Generify pci-hyperv.c Boqun Feng
  2020-02-10  3:39 ` [PATCH v3 1/3] PCI: hv: Move hypercall related definitions into tlfs header Boqun Feng
  2020-02-10  3:39 ` [PATCH v3 2/3] PCI: hv: Move retarget related structures " Boqun Feng
@ 2020-02-10  3:39 ` Boqun Feng
  2020-02-13  4:18   ` Dexuan Cui
  2020-02-21  2:33 ` [PATCH v3 0/3] PCI: hv: Generify pci-hyperv.c Boqun Feng
  3 siblings, 1 reply; 15+ messages in thread
From: Boqun Feng @ 2020-02-10  3:39 UTC (permalink / raw)
  To: linux-pci, linux-hyperv, linux-kernel, linux-arm-kernel
  Cc: Michael Kelley, K. Y. Srinivasan, Haiyang Zhang,
	Stephen Hemminger, Sasha Levin, Thomas Gleixner, Ingo Molnar,
	Borislav Petkov, H. Peter Anvin, x86, Lorenzo Pieralisi,
	Andrew Murray, Bjorn Helgaas, Boqun Feng, Andrew Murray

Add a new structure (hv_msi_entry), which is also defined in the TLFS,
to describe the msi entry for HVCALL_RETARGET_INTERRUPT. The structure
is needed because its layout may be different from architecture to
architecture.

Also add a new generic interface hv_set_msi_entry_from_desc() to allow
different archs to set the msi entry from msi_desc.

No functional change, only preparation for the future support of virtual
PCI on non-x86 architectures.

Signed-off-by: Boqun Feng (Microsoft) <boqun.feng@gmail.com>
---
 arch/x86/include/asm/hyperv-tlfs.h  | 11 +++++++++--
 arch/x86/include/asm/mshyperv.h     |  8 ++++++++
 drivers/pci/controller/pci-hyperv.c |  3 +--
 3 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/arch/x86/include/asm/hyperv-tlfs.h b/arch/x86/include/asm/hyperv-tlfs.h
index a0b6a88d2f05..29336574d0bc 100644
--- a/arch/x86/include/asm/hyperv-tlfs.h
+++ b/arch/x86/include/asm/hyperv-tlfs.h
@@ -913,11 +913,18 @@ struct hv_partition_assist_pg {
 	u32 tlb_lock_count;
 };
 
+union hv_msi_entry {
+	u64 as_uint64;
+	struct {
+		u32 address;
+		u32 data;
+	} __packed;
+};
+
 struct hv_interrupt_entry {
 	u32 source;			/* 1 for MSI(-X) */
 	u32 reserved1;
-	u32 address;
-	u32 data;
+	union hv_msi_entry msi_entry;
 } __packed;
 
 /*
diff --git a/arch/x86/include/asm/mshyperv.h b/arch/x86/include/asm/mshyperv.h
index 6b79515abb82..81fc30240122 100644
--- a/arch/x86/include/asm/mshyperv.h
+++ b/arch/x86/include/asm/mshyperv.h
@@ -4,6 +4,7 @@
 
 #include <linux/types.h>
 #include <linux/nmi.h>
+#include <linux/msi.h>
 #include <asm/io.h>
 #include <asm/hyperv-tlfs.h>
 #include <asm/nospec-branch.h>
@@ -240,6 +241,13 @@ bool hv_vcpu_is_preempted(int vcpu);
 static inline void hv_apic_init(void) {}
 #endif
 
+static inline void hv_set_msi_entry_from_desc(union hv_msi_entry *msi_entry,
+					      struct msi_desc *msi_desc)
+{
+	msi_entry->address = msi_desc->msg.address_lo;
+	msi_entry->data = msi_desc->msg.data;
+}
+
 #else /* CONFIG_HYPERV */
 static inline void hyperv_init(void) {}
 static inline void hyperv_setup_mmu_ops(void) {}
diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
index 0d9b74503577..3f9b220c23ec 100644
--- a/drivers/pci/controller/pci-hyperv.c
+++ b/drivers/pci/controller/pci-hyperv.c
@@ -1170,8 +1170,7 @@ static void hv_irq_unmask(struct irq_data *data)
 	memset(params, 0, sizeof(*params));
 	params->partition_id = HV_PARTITION_ID_SELF;
 	params->int_entry.source = 1; /* MSI(-X) */
-	params->int_entry.address = msi_desc->msg.address_lo;
-	params->int_entry.data = msi_desc->msg.data;
+	hv_set_msi_entry_from_desc(&params->int_entry.msi_entry, msi_desc);
 	params->device_id = (hbus->hdev->dev_instance.b[5] << 24) |
 			   (hbus->hdev->dev_instance.b[4] << 16) |
 			   (hbus->hdev->dev_instance.b[7] << 8) |
-- 
2.24.1


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* RE: [PATCH v3 1/3] PCI: hv: Move hypercall related definitions into tlfs header
  2020-02-10  3:39 ` [PATCH v3 1/3] PCI: hv: Move hypercall related definitions into tlfs header Boqun Feng
@ 2020-02-13  4:17   ` Dexuan Cui
  0 siblings, 0 replies; 15+ messages in thread
From: Dexuan Cui @ 2020-02-13  4:17 UTC (permalink / raw)
  To: Boqun Feng, linux-pci, linux-hyperv, linux-kernel, linux-arm-kernel
  Cc: Michael Kelley, KY Srinivasan, Haiyang Zhang, Stephen Hemminger,
	Sasha Levin, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	H. Peter Anvin, x86, Lorenzo Pieralisi, Andrew Murray,
	Bjorn Helgaas, Andrew Murray

> From: linux-hyperv-owner@vger.kernel.org
> <linux-hyperv-owner@vger.kernel.org> On Behalf Of Boqun Feng
> Sent: Sunday, February 9, 2020 7:40 PM
> 
> Currently HVCALL_RETARGET_INTERRUPT and HV_PARTITION_ID_SELF are
> defined
> in pci-hyperv.c. However, similar to other hypercall related
> definitions, it makes more sense to put them in the tlfs header file.
> 
> Besides, these definitions are arch-dependent, so for the support of
> virtual PCI on non-x86 archs in the future, move them into arch-specific
> tlfs header file.
> 
> Signed-off-by: Boqun Feng (Microsoft) <boqun.feng@gmail.com>
> Reviewed-by: Andrew Murray <amurray@thegoodpenguin.co.uk>

Reviewed-by: Dexuan Cui <decui@microsoft.com>



^ permalink raw reply	[flat|nested] 15+ messages in thread

* RE: [PATCH v3 2/3] PCI: hv: Move retarget related structures into tlfs header
  2020-02-10  3:39 ` [PATCH v3 2/3] PCI: hv: Move retarget related structures " Boqun Feng
@ 2020-02-13  4:17   ` Dexuan Cui
  2020-02-13  7:26     ` Boqun Feng
  0 siblings, 1 reply; 15+ messages in thread
From: Dexuan Cui @ 2020-02-13  4:17 UTC (permalink / raw)
  To: Boqun Feng, linux-pci, linux-hyperv, linux-kernel, linux-arm-kernel
  Cc: Michael Kelley, KY Srinivasan, Haiyang Zhang, Stephen Hemminger,
	Sasha Levin, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	H. Peter Anvin, x86, Lorenzo Pieralisi, Andrew Murray,
	Bjorn Helgaas, Andrew Murray

> From: linux-hyperv-owner@vger.kernel.org
> <linux-hyperv-owner@vger.kernel.org> On Behalf Of Boqun Feng
> Sent: Sunday, February 9, 2020 7:40 PM
> 
> 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. Let's move those definitions into x86's tlfs header file
> to support virtual PCI on non-x86 architectures in the future. Note that
> "__packed" attribute is added to these structures during the movement
> for the same reason as we use the attribute for other TLFS structures in
> the header file: make sure the structures meet the specification and
> avoid anything unexpected from the compilers.
> 
> Additionally, rename struct retarget_msi_interrupt to
> hv_retarget_msi_interrupt for the consistent naming convention, also
> mirroring the name in TLFS.
> 
> diff --git a/arch/x86/include/asm/hyperv-tlfs.h
> b/arch/x86/include/asm/hyperv-tlfs.h
> +
> +struct hv_device_interrupt_target {
> +	u32 vector;
> +	u32 flags;
> +	union {
> +		u64 vp_mask;
> +		struct hv_vpset vp_set;
> +	};
> +} __packed;
> +
> +/* HvRetargetDeviceInterrupt hypercall */

Reviewed-by: Dexuan Cui <decui@microsoft.com>

Just a small thing: would it be slightly better if we change the name 
in the above line to HVCALL_RETARGET_INTERRUPT ? 

HVCALL_RETARGET_INTERRUPT is a define, so it may help to locate the
actual value of the define here. And, HVCALL_RETARGET_INTERRUPT is
used several times in the patchset so IMO we'd better always use
the same name.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* RE: [PATCH v3 3/3] PCI: hv: Introduce hv_msi_entry
  2020-02-10  3:39 ` [PATCH v3 3/3] PCI: hv: Introduce hv_msi_entry Boqun Feng
@ 2020-02-13  4:18   ` Dexuan Cui
  2020-02-13  7:14     ` Boqun Feng
  0 siblings, 1 reply; 15+ messages in thread
From: Dexuan Cui @ 2020-02-13  4:18 UTC (permalink / raw)
  To: Boqun Feng, linux-pci, linux-hyperv, linux-kernel, linux-arm-kernel
  Cc: Michael Kelley, KY Srinivasan, Haiyang Zhang, Stephen Hemminger,
	Sasha Levin, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	H. Peter Anvin, x86, Lorenzo Pieralisi, Andrew Murray,
	Bjorn Helgaas, Andrew Murray

> From: linux-hyperv-owner@vger.kernel.org
> <linux-hyperv-owner@vger.kernel.org> On Behalf Of Boqun Feng
> diff --git a/arch/x86/include/asm/hyperv-tlfs.h
> b/arch/x86/include/asm/hyperv-tlfs.h
> 
> +union hv_msi_entry {
> +	u64 as_uint64;
> +	struct {
> +		u32 address;
> +		u32 data;
> +	} __packed;
> +};

Just a small thing: should we move the __packed to after the "}" of
the union hv_msi_entry ?

Reviewed-by: Dexuan Cui <decui@microsoft.com>

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v3 3/3] PCI: hv: Introduce hv_msi_entry
  2020-02-13  4:18   ` Dexuan Cui
@ 2020-02-13  7:14     ` Boqun Feng
  2020-02-13  8:05       ` Dexuan Cui
  0 siblings, 1 reply; 15+ messages in thread
From: Boqun Feng @ 2020-02-13  7:14 UTC (permalink / raw)
  To: Dexuan Cui
  Cc: linux-pci, linux-hyperv, linux-kernel, linux-arm-kernel,
	Michael Kelley, KY Srinivasan, Haiyang Zhang, Stephen Hemminger,
	Sasha Levin, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	H. Peter Anvin, x86, Lorenzo Pieralisi, Andrew Murray,
	Bjorn Helgaas, Andrew Murray

On Thu, Feb 13, 2020 at 04:18:01AM +0000, Dexuan Cui wrote:
> > From: linux-hyperv-owner@vger.kernel.org
> > <linux-hyperv-owner@vger.kernel.org> On Behalf Of Boqun Feng
> > diff --git a/arch/x86/include/asm/hyperv-tlfs.h
> > b/arch/x86/include/asm/hyperv-tlfs.h
> > 
> > +union hv_msi_entry {
> > +	u64 as_uint64;
> > +	struct {
> > +		u32 address;
> > +		u32 data;
> > +	} __packed;
> > +};
> 
> Just a small thing: should we move the __packed to after the "}" of
> the union hv_msi_entry ?
> 

Actually, in TLFS header, it's common to put the "__packed" inside the
union, rather than after the union. It makes sense because union is
different than struct: the alignment requirement of a union is already
decided by the "as_*" member, so no need for "__packed" attribute.

> Reviewed-by: Dexuan Cui <decui@microsoft.com>

Thanks!

Regards,
Boqun

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v3 2/3] PCI: hv: Move retarget related structures into tlfs header
  2020-02-13  4:17   ` Dexuan Cui
@ 2020-02-13  7:26     ` Boqun Feng
  2020-02-13  8:04       ` Dexuan Cui
  0 siblings, 1 reply; 15+ messages in thread
From: Boqun Feng @ 2020-02-13  7:26 UTC (permalink / raw)
  To: Dexuan Cui
  Cc: linux-pci, linux-hyperv, linux-kernel, linux-arm-kernel,
	Michael Kelley, KY Srinivasan, Haiyang Zhang, Stephen Hemminger,
	Sasha Levin, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	H. Peter Anvin, x86, Lorenzo Pieralisi, Andrew Murray,
	Bjorn Helgaas, Andrew Murray

On Thu, Feb 13, 2020 at 04:17:34AM +0000, Dexuan Cui wrote:
> > From: linux-hyperv-owner@vger.kernel.org
> > <linux-hyperv-owner@vger.kernel.org> On Behalf Of Boqun Feng
> > Sent: Sunday, February 9, 2020 7:40 PM
> > 
> > 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. Let's move those definitions into x86's tlfs header file
> > to support virtual PCI on non-x86 architectures in the future. Note that
> > "__packed" attribute is added to these structures during the movement
> > for the same reason as we use the attribute for other TLFS structures in
> > the header file: make sure the structures meet the specification and
> > avoid anything unexpected from the compilers.
> > 
> > Additionally, rename struct retarget_msi_interrupt to
> > hv_retarget_msi_interrupt for the consistent naming convention, also
> > mirroring the name in TLFS.
> > 
> > diff --git a/arch/x86/include/asm/hyperv-tlfs.h
> > b/arch/x86/include/asm/hyperv-tlfs.h
> > +
> > +struct hv_device_interrupt_target {
> > +	u32 vector;
> > +	u32 flags;
> > +	union {
> > +		u64 vp_mask;
> > +		struct hv_vpset vp_set;
> > +	};
> > +} __packed;
> > +
> > +/* HvRetargetDeviceInterrupt hypercall */
> 
> Reviewed-by: Dexuan Cui <decui@microsoft.com>
> 

Thanks!

> Just a small thing: would it be slightly better if we change the name 
> in the above line to HVCALL_RETARGET_INTERRUPT ? 
> 
> HVCALL_RETARGET_INTERRUPT is a define, so it may help to locate the
> actual value of the define here. And, HVCALL_RETARGET_INTERRUPT is
> used several times in the patchset so IMO we'd better always use
> the same name.

This might be a good suggestion, however, throughout the TLFS header,
camel case is more commonly used for referencing hypercall. For example:

	/* HvCallSendSyntheticClusterIpi hypercall */

So I think it's better to let it as it is for this patch, and later on,
if we reach a consensus, we can convert the names all together.

Thoughts?

Regards,
Boqun

^ permalink raw reply	[flat|nested] 15+ messages in thread

* RE: [PATCH v3 2/3] PCI: hv: Move retarget related structures into tlfs header
  2020-02-13  7:26     ` Boqun Feng
@ 2020-02-13  8:04       ` Dexuan Cui
  0 siblings, 0 replies; 15+ messages in thread
From: Dexuan Cui @ 2020-02-13  8:04 UTC (permalink / raw)
  To: Boqun Feng
  Cc: linux-pci, linux-hyperv, linux-kernel, linux-arm-kernel,
	Michael Kelley, KY Srinivasan, Haiyang Zhang, Stephen Hemminger,
	Sasha Levin, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	H. Peter Anvin, x86, Lorenzo Pieralisi, Andrew Murray,
	Bjorn Helgaas, Andrew Murray

> From: Boqun Feng <boqun.feng@gmail.com>
> Sent: Wednesday, February 12, 2020 11:26 PM
> 
> > Just a small thing: would it be slightly better if we change the name
> > in the above line to HVCALL_RETARGET_INTERRUPT ?
> >
> > HVCALL_RETARGET_INTERRUPT is a define, so it may help to locate the
> > actual value of the define here. And, HVCALL_RETARGET_INTERRUPT is
> > used several times in the patchset so IMO we'd better always use
> > the same name.
> 
> This might be a good suggestion, however, throughout the TLFS header,
> camel case is more commonly used for referencing hypercall. For example:
> 
> 	/* HvCallSendSyntheticClusterIpi hypercall */
> 
> So I think it's better to let it as it is for this patch, and later on,
> if we reach a consensus, we can convert the names all together.
> 
> Thoughts?
> 
> Regards,
> Boqun

Makes sense to me. Thanks for the explanation!

Thanks,
-- Dexuan

^ permalink raw reply	[flat|nested] 15+ messages in thread

* RE: [PATCH v3 3/3] PCI: hv: Introduce hv_msi_entry
  2020-02-13  7:14     ` Boqun Feng
@ 2020-02-13  8:05       ` Dexuan Cui
  0 siblings, 0 replies; 15+ messages in thread
From: Dexuan Cui @ 2020-02-13  8:05 UTC (permalink / raw)
  To: Boqun Feng
  Cc: linux-pci, linux-hyperv, linux-kernel, linux-arm-kernel,
	Michael Kelley, KY Srinivasan, Haiyang Zhang, Stephen Hemminger,
	Sasha Levin, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	H. Peter Anvin, x86, Lorenzo Pieralisi, Andrew Murray,
	Bjorn Helgaas, Andrew Murray

> From: Boqun Feng <boqun.feng@gmail.com>
> Sent: Wednesday, February 12, 2020 11:14 PM
> 
> On Thu, Feb 13, 2020 at 04:18:01AM +0000, Dexuan Cui wrote:
> > > From: linux-hyperv-owner@vger.kernel.org
> > > <linux-hyperv-owner@vger.kernel.org> On Behalf Of Boqun Feng
> > > diff --git a/arch/x86/include/asm/hyperv-tlfs.h
> > > b/arch/x86/include/asm/hyperv-tlfs.h
> > >
> > > +union hv_msi_entry {
> > > +	u64 as_uint64;
> > > +	struct {
> > > +		u32 address;
> > > +		u32 data;
> > > +	} __packed;
> > > +};
> >
> > Just a small thing: should we move the __packed to after the "}" of
> > the union hv_msi_entry ?
> >
> 
> Actually, in TLFS header, it's common to put the "__packed" inside the
> union, rather than after the union. It makes sense because union is
> different than struct: the alignment requirement of a union is already
> decided by the "as_*" member, so no need for "__packed" attribute.
> 
> Regards,
> Boqun

I see. Thanks for the explanation!

Thanks,
-- Dexuan

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v3 0/3] PCI: hv: Generify pci-hyperv.c
  2020-02-10  3:39 [PATCH v3 0/3] PCI: hv: Generify pci-hyperv.c Boqun Feng
                   ` (2 preceding siblings ...)
  2020-02-10  3:39 ` [PATCH v3 3/3] PCI: hv: Introduce hv_msi_entry Boqun Feng
@ 2020-02-21  2:33 ` Boqun Feng
  2020-02-21 10:44   ` Lorenzo Pieralisi
  3 siblings, 1 reply; 15+ messages in thread
From: Boqun Feng @ 2020-02-21  2:33 UTC (permalink / raw)
  To: linux-pci, linux-hyperv, linux-kernel, linux-arm-kernel
  Cc: Michael Kelley, K. Y. Srinivasan, Haiyang Zhang,
	Stephen Hemminger, Sasha Levin, Thomas Gleixner, Ingo Molnar,
	Borislav Petkov, H. Peter Anvin, x86, Lorenzo Pieralisi,
	Andrew Murray, Bjorn Helgaas

Ping ;-)

Any suggestion or plan on this patchset?

Thanks and Regards,
Boqun

On Mon, Feb 10, 2020 at 11:39:50AM +0800, Boqun Feng wrote:
> Hi,
> 
> This is the first part for virtual PCI support of Hyper-V guest on
> ARM64. The whole patchset doesn't have any functional change, but only
> refactors the pci-hyperv.c code to make it more arch-independent.
> 
> Previous version:
> v1: https://lore.kernel.org/lkml/20200121015713.69691-1-boqun.feng@gmail.com/
> v2: https://lore.kernel.org/linux-arm-kernel/20200203050313.69247-1-boqun.feng@gmail.com/
> 
> Changes since v2:
> 
> *	Rebased on 5.6-rc1
> 
> *	Reword commit logs as per Andrew's suggestion.
> 
> *	It makes more sense to have a generic interface to set the whole
> 	msi_entry rather than only the "address" field. So change
> 	hv_set_msi_address_from_desc() to hv_set_msi_entry_from_desc().
> 	Additionally, make it an inline function as per the suggestion
> 	of Andrew and Thomas.
> 
> *	Add the missing comment saying the partition_id of
> 	hv_retarget_device_interrupt must be self.
> 
> *	Add the explanation for why "__packed" is needed for TLFS
> 	structures.
> 
> I've done compile and boot test of this patchset, also done some tests
> with a pass-through NVMe device.
> 
> Suggestions and comments are welcome!
> 
> Regards,
> Boqun
> 
> Boqun Feng (3):
>   PCI: hv: Move hypercall related definitions into tlfs header
>   PCI: hv: Move retarget related structures into tlfs header
>   PCI: hv: Introduce hv_msi_entry
> 
>  arch/x86/include/asm/hyperv-tlfs.h  | 41 +++++++++++++++++++++++++++
>  arch/x86/include/asm/mshyperv.h     |  8 ++++++
>  drivers/pci/controller/pci-hyperv.c | 43 ++---------------------------
>  3 files changed, 52 insertions(+), 40 deletions(-)
> 
> -- 
> 2.24.1
> 

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v3 0/3] PCI: hv: Generify pci-hyperv.c
  2020-02-21  2:33 ` [PATCH v3 0/3] PCI: hv: Generify pci-hyperv.c Boqun Feng
@ 2020-02-21 10:44   ` Lorenzo Pieralisi
  2020-03-09 14:35     ` Boqun Feng
  0 siblings, 1 reply; 15+ messages in thread
From: Lorenzo Pieralisi @ 2020-02-21 10:44 UTC (permalink / raw)
  To: Boqun Feng
  Cc: linux-pci, linux-hyperv, linux-kernel, linux-arm-kernel,
	Michael Kelley, K. Y. Srinivasan, Haiyang Zhang,
	Stephen Hemminger, Sasha Levin, Thomas Gleixner, Ingo Molnar,
	Borislav Petkov, H. Peter Anvin, x86, Andrew Murray,
	Bjorn Helgaas

On Fri, Feb 21, 2020 at 10:33:44AM +0800, Boqun Feng wrote:
> Ping ;-)
> 
> Any suggestion or plan on this patchset?

Hi,

I shall have a look shortly, thanks.

Lorenzo

> Thanks and Regards,
> Boqun
> 
> On Mon, Feb 10, 2020 at 11:39:50AM +0800, Boqun Feng wrote:
> > Hi,
> > 
> > This is the first part for virtual PCI support of Hyper-V guest on
> > ARM64. The whole patchset doesn't have any functional change, but only
> > refactors the pci-hyperv.c code to make it more arch-independent.
> > 
> > Previous version:
> > v1: https://lore.kernel.org/lkml/20200121015713.69691-1-boqun.feng@gmail.com/
> > v2: https://lore.kernel.org/linux-arm-kernel/20200203050313.69247-1-boqun.feng@gmail.com/
> > 
> > Changes since v2:
> > 
> > *	Rebased on 5.6-rc1
> > 
> > *	Reword commit logs as per Andrew's suggestion.
> > 
> > *	It makes more sense to have a generic interface to set the whole
> > 	msi_entry rather than only the "address" field. So change
> > 	hv_set_msi_address_from_desc() to hv_set_msi_entry_from_desc().
> > 	Additionally, make it an inline function as per the suggestion
> > 	of Andrew and Thomas.
> > 
> > *	Add the missing comment saying the partition_id of
> > 	hv_retarget_device_interrupt must be self.
> > 
> > *	Add the explanation for why "__packed" is needed for TLFS
> > 	structures.
> > 
> > I've done compile and boot test of this patchset, also done some tests
> > with a pass-through NVMe device.
> > 
> > Suggestions and comments are welcome!
> > 
> > Regards,
> > Boqun
> > 
> > Boqun Feng (3):
> >   PCI: hv: Move hypercall related definitions into tlfs header
> >   PCI: hv: Move retarget related structures into tlfs header
> >   PCI: hv: Introduce hv_msi_entry
> > 
> >  arch/x86/include/asm/hyperv-tlfs.h  | 41 +++++++++++++++++++++++++++
> >  arch/x86/include/asm/mshyperv.h     |  8 ++++++
> >  drivers/pci/controller/pci-hyperv.c | 43 ++---------------------------
> >  3 files changed, 52 insertions(+), 40 deletions(-)
> > 
> > -- 
> > 2.24.1
> > 

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v3 0/3] PCI: hv: Generify pci-hyperv.c
  2020-02-21 10:44   ` Lorenzo Pieralisi
@ 2020-03-09 14:35     ` Boqun Feng
  2020-03-09 15:03       ` Lorenzo Pieralisi
  0 siblings, 1 reply; 15+ messages in thread
From: Boqun Feng @ 2020-03-09 14:35 UTC (permalink / raw)
  To: Lorenzo Pieralisi
  Cc: linux-pci, linux-hyperv, linux-kernel, linux-arm-kernel,
	Michael Kelley, K. Y. Srinivasan, Haiyang Zhang,
	Stephen Hemminger, Sasha Levin, Thomas Gleixner, Ingo Molnar,
	Borislav Petkov, H. Peter Anvin, x86, Andrew Murray,
	Bjorn Helgaas

Hi Lorenzo,

On Fri, Feb 21, 2020 at 10:44:54AM +0000, Lorenzo Pieralisi wrote:
> On Fri, Feb 21, 2020 at 10:33:44AM +0800, Boqun Feng wrote:
> > Ping ;-)
> > 
> > Any suggestion or plan on this patchset?
> 
> Hi,
> 
> I shall have a look shortly, thanks.
> 

Any chance you got some time to look into this?

Regards,
Boqun

> Lorenzo
> 
> > Thanks and Regards,
> > Boqun
> > 
> > On Mon, Feb 10, 2020 at 11:39:50AM +0800, Boqun Feng wrote:
> > > Hi,
> > > 
> > > This is the first part for virtual PCI support of Hyper-V guest on
> > > ARM64. The whole patchset doesn't have any functional change, but only
> > > refactors the pci-hyperv.c code to make it more arch-independent.
> > > 
> > > Previous version:
> > > v1: https://lore.kernel.org/lkml/20200121015713.69691-1-boqun.feng@gmail.com/
> > > v2: https://lore.kernel.org/linux-arm-kernel/20200203050313.69247-1-boqun.feng@gmail.com/
> > > 
> > > Changes since v2:
> > > 
> > > *	Rebased on 5.6-rc1
> > > 
> > > *	Reword commit logs as per Andrew's suggestion.
> > > 
> > > *	It makes more sense to have a generic interface to set the whole
> > > 	msi_entry rather than only the "address" field. So change
> > > 	hv_set_msi_address_from_desc() to hv_set_msi_entry_from_desc().
> > > 	Additionally, make it an inline function as per the suggestion
> > > 	of Andrew and Thomas.
> > > 
> > > *	Add the missing comment saying the partition_id of
> > > 	hv_retarget_device_interrupt must be self.
> > > 
> > > *	Add the explanation for why "__packed" is needed for TLFS
> > > 	structures.
> > > 
> > > I've done compile and boot test of this patchset, also done some tests
> > > with a pass-through NVMe device.
> > > 
> > > Suggestions and comments are welcome!
> > > 
> > > Regards,
> > > Boqun
> > > 
> > > Boqun Feng (3):
> > >   PCI: hv: Move hypercall related definitions into tlfs header
> > >   PCI: hv: Move retarget related structures into tlfs header
> > >   PCI: hv: Introduce hv_msi_entry
> > > 
> > >  arch/x86/include/asm/hyperv-tlfs.h  | 41 +++++++++++++++++++++++++++
> > >  arch/x86/include/asm/mshyperv.h     |  8 ++++++
> > >  drivers/pci/controller/pci-hyperv.c | 43 ++---------------------------
> > >  3 files changed, 52 insertions(+), 40 deletions(-)
> > > 
> > > -- 
> > > 2.24.1
> > > 

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v3 0/3] PCI: hv: Generify pci-hyperv.c
  2020-03-09 14:35     ` Boqun Feng
@ 2020-03-09 15:03       ` Lorenzo Pieralisi
  0 siblings, 0 replies; 15+ messages in thread
From: Lorenzo Pieralisi @ 2020-03-09 15:03 UTC (permalink / raw)
  To: Boqun Feng
  Cc: linux-pci, linux-hyperv, linux-kernel, linux-arm-kernel,
	Michael Kelley, K. Y. Srinivasan, Haiyang Zhang,
	Stephen Hemminger, Sasha Levin, Thomas Gleixner, Ingo Molnar,
	Borislav Petkov, H. Peter Anvin, x86, Andrew Murray,
	Bjorn Helgaas

On Mon, Mar 09, 2020 at 10:35:48PM +0800, Boqun Feng wrote:
> Hi Lorenzo,
> 
> On Fri, Feb 21, 2020 at 10:44:54AM +0000, Lorenzo Pieralisi wrote:
> > On Fri, Feb 21, 2020 at 10:33:44AM +0800, Boqun Feng wrote:
> > > Ping ;-)
> > > 
> > > Any suggestion or plan on this patchset?
> > 
> > Hi,
> > 
> > I shall have a look shortly, thanks.
> > 
> 
> Any chance you got some time to look into this?

I did last week and I applied it now, pci/hv for v5.7.

Thanks,
Lorenzo

> Regards,
> Boqun
> 
> > Lorenzo
> > 
> > > Thanks and Regards,
> > > Boqun
> > > 
> > > On Mon, Feb 10, 2020 at 11:39:50AM +0800, Boqun Feng wrote:
> > > > Hi,
> > > > 
> > > > This is the first part for virtual PCI support of Hyper-V guest on
> > > > ARM64. The whole patchset doesn't have any functional change, but only
> > > > refactors the pci-hyperv.c code to make it more arch-independent.
> > > > 
> > > > Previous version:
> > > > v1: https://lore.kernel.org/lkml/20200121015713.69691-1-boqun.feng@gmail.com/
> > > > v2: https://lore.kernel.org/linux-arm-kernel/20200203050313.69247-1-boqun.feng@gmail.com/
> > > > 
> > > > Changes since v2:
> > > > 
> > > > *	Rebased on 5.6-rc1
> > > > 
> > > > *	Reword commit logs as per Andrew's suggestion.
> > > > 
> > > > *	It makes more sense to have a generic interface to set the whole
> > > > 	msi_entry rather than only the "address" field. So change
> > > > 	hv_set_msi_address_from_desc() to hv_set_msi_entry_from_desc().
> > > > 	Additionally, make it an inline function as per the suggestion
> > > > 	of Andrew and Thomas.
> > > > 
> > > > *	Add the missing comment saying the partition_id of
> > > > 	hv_retarget_device_interrupt must be self.
> > > > 
> > > > *	Add the explanation for why "__packed" is needed for TLFS
> > > > 	structures.
> > > > 
> > > > I've done compile and boot test of this patchset, also done some tests
> > > > with a pass-through NVMe device.
> > > > 
> > > > Suggestions and comments are welcome!
> > > > 
> > > > Regards,
> > > > Boqun
> > > > 
> > > > Boqun Feng (3):
> > > >   PCI: hv: Move hypercall related definitions into tlfs header
> > > >   PCI: hv: Move retarget related structures into tlfs header
> > > >   PCI: hv: Introduce hv_msi_entry
> > > > 
> > > >  arch/x86/include/asm/hyperv-tlfs.h  | 41 +++++++++++++++++++++++++++
> > > >  arch/x86/include/asm/mshyperv.h     |  8 ++++++
> > > >  drivers/pci/controller/pci-hyperv.c | 43 ++---------------------------
> > > >  3 files changed, 52 insertions(+), 40 deletions(-)
> > > > 
> > > > -- 
> > > > 2.24.1
> > > > 

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2020-03-09 15:03 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-10  3:39 [PATCH v3 0/3] PCI: hv: Generify pci-hyperv.c Boqun Feng
2020-02-10  3:39 ` [PATCH v3 1/3] PCI: hv: Move hypercall related definitions into tlfs header Boqun Feng
2020-02-13  4:17   ` Dexuan Cui
2020-02-10  3:39 ` [PATCH v3 2/3] PCI: hv: Move retarget related structures " Boqun Feng
2020-02-13  4:17   ` Dexuan Cui
2020-02-13  7:26     ` Boqun Feng
2020-02-13  8:04       ` Dexuan Cui
2020-02-10  3:39 ` [PATCH v3 3/3] PCI: hv: Introduce hv_msi_entry Boqun Feng
2020-02-13  4:18   ` Dexuan Cui
2020-02-13  7:14     ` Boqun Feng
2020-02-13  8:05       ` Dexuan Cui
2020-02-21  2:33 ` [PATCH v3 0/3] PCI: hv: Generify pci-hyperv.c Boqun Feng
2020-02-21 10:44   ` Lorenzo Pieralisi
2020-03-09 14:35     ` Boqun Feng
2020-03-09 15:03       ` Lorenzo Pieralisi

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).