[PATCH v2 1/2] PCI: hv: Make the code arch neutral

[PATCH v2 1/2] PCI: hv: Make the code arch neutral

[Sunil Muthuswamy]

This patch makes the Hyper-V vPCI code architectural neutral by
introducing an irqchip that takes care of architectural
dependencies. This allows for the implementation of Hyper-V vPCI
for other architecture such as ARM64.

There are no functional changes expected from this patch.

Signed-off-by: Sunil Muthuswamy <sunilmut@microsoft.com>
---
In v2:
 v2 changes are described in the cover letter.

 MAINTAINERS                                 |  2 +
 arch/x86/include/asm/hyperv-tlfs.h          | 33 +++++++++++++
 arch/x86/include/asm/mshyperv.h             |  7 ---
 drivers/pci/controller/Makefile             |  2 +-
 drivers/pci/controller/pci-hyperv-irqchip.c | 51 ++++++++++++++++++++
 drivers/pci/controller/pci-hyperv-irqchip.h | 21 +++++++++
 drivers/pci/controller/pci-hyperv.c         | 52 +++++++++++++--------
 include/asm-generic/hyperv-tlfs.h           | 33 -------------
 8 files changed, 141 insertions(+), 60 deletions(-)
 create mode 100644 drivers/pci/controller/pci-hyperv-irqchip.c
 create mode 100644 drivers/pci/controller/pci-hyperv-irqchip.h

diff --git a/MAINTAINERS b/MAINTAINERS
index ca6d6fde85cf..ba8c979c17b2 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8688,6 +8688,8 @@ F:	drivers/iommu/hyperv-iommu.c
 F:	drivers/net/ethernet/microsoft/
 F:	drivers/net/hyperv/
 F:	drivers/pci/controller/pci-hyperv-intf.c
+F:	drivers/pci/controller/pci-hyperv-irqchip.c
+F:	drivers/pci/controller/pci-hyperv-irqchip.h
 F:	drivers/pci/controller/pci-hyperv.c
 F:	drivers/scsi/storvsc_drv.c
 F:	drivers/uio/uio_hv_generic.c
diff --git a/arch/x86/include/asm/hyperv-tlfs.h b/arch/x86/include/asm/hyperv-tlfs.h
index 2322d6bd5883..fdf3d28fbdd5 100644
--- a/arch/x86/include/asm/hyperv-tlfs.h
+++ b/arch/x86/include/asm/hyperv-tlfs.h
@@ -585,6 +585,39 @@ enum hv_interrupt_type {
 	HV_X64_INTERRUPT_TYPE_MAXIMUM           = 0x000A,
 };
 
+union hv_msi_address_register {
+	u32 as_uint32;
+	struct {
+		u32 reserved1:2;
+		u32 destination_mode:1;
+		u32 redirection_hint:1;
+		u32 reserved2:8;
+		u32 destination_id:8;
+		u32 msi_base:12;
+	};
+} __packed;
+
+union hv_msi_data_register {
+	u32 as_uint32;
+	struct {
+		u32 vector:8;
+		u32 delivery_mode:3;
+		u32 reserved1:3;
+		u32 level_assert:1;
+		u32 trigger_mode:1;
+		u32 reserved2:16;
+	};
+} __packed;
+
+/* HvRetargetDeviceInterrupt hypercall */
+union hv_msi_entry {
+	u64 as_uint64;
+	struct {
+		union hv_msi_address_register address;
+		union hv_msi_data_register data;
+	} __packed;
+};
+
 #include <asm-generic/hyperv-tlfs.h>
 
 #endif
diff --git a/arch/x86/include/asm/mshyperv.h b/arch/x86/include/asm/mshyperv.h
index adccbc209169..c2b9ab94408e 100644
--- a/arch/x86/include/asm/mshyperv.h
+++ b/arch/x86/include/asm/mshyperv.h
@@ -176,13 +176,6 @@ 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.as_uint32 = msi_desc->msg.address_lo;
-	msi_entry->data.as_uint32 = msi_desc->msg.data;
-}
-
 struct irq_domain *hv_create_pci_msi_domain(void);
 
 int hv_map_ioapic_interrupt(int ioapic_id, bool level, int vcpu, int vector,
diff --git a/drivers/pci/controller/Makefile b/drivers/pci/controller/Makefile
index aaf30b3dcc14..2c301d0fc23b 100644
--- a/drivers/pci/controller/Makefile
+++ b/drivers/pci/controller/Makefile
@@ -2,7 +2,7 @@
 obj-$(CONFIG_PCIE_CADENCE) += cadence/
 obj-$(CONFIG_PCI_FTPCI100) += pci-ftpci100.o
 obj-$(CONFIG_PCI_IXP4XX) += pci-ixp4xx.o
-obj-$(CONFIG_PCI_HYPERV) += pci-hyperv.o
+obj-$(CONFIG_PCI_HYPERV) += pci-hyperv.o pci-hyperv-irqchip.o
 obj-$(CONFIG_PCI_HYPERV_INTERFACE) += pci-hyperv-intf.o
 obj-$(CONFIG_PCI_MVEBU) += pci-mvebu.o
 obj-$(CONFIG_PCI_AARDVARK) += pci-aardvark.o
diff --git a/drivers/pci/controller/pci-hyperv-irqchip.c b/drivers/pci/controller/pci-hyperv-irqchip.c
new file mode 100644
index 000000000000..5f334f7d66cb
--- /dev/null
+++ b/drivers/pci/controller/pci-hyperv-irqchip.c
@@ -0,0 +1,51 @@
+// SPDX-License-Identifier: GPL-2.0
+
+/*
+ * Hyper-V vPCI irqchip.
+ *
+ * Copyright (C) 2021, Microsoft, Inc.
+ *
+ * Author : Sunil Muthuswamy <sunilmut@microsoft.com>
+ */
+
+#include <asm/mshyperv.h>
+#include <linux/acpi.h>
+#include <linux/irqdomain.h>
+#include <linux/irq.h>
+#include <linux/msi.h>
+
+#ifdef CONFIG_X86_64
+int hv_pci_irqchip_init(struct irq_domain **parent_domain,
+			bool *fasteoi_handler,
+			u8 *delivery_mode)
+{
+	*parent_domain = x86_vector_domain;
+	*fasteoi_handler = false;
+	*delivery_mode = APIC_DELIVERY_MODE_FIXED;
+
+	return 0;
+}
+
+void hv_pci_irqchip_free(void) {}
+
+unsigned int hv_msi_get_int_vector(struct irq_data *data)
+{
+	struct irq_cfg *cfg = irqd_cfg(data);
+
+	return cfg->vector;
+}
+
+void hv_set_msi_entry_from_desc(union hv_msi_entry *msi_entry,
+				struct msi_desc *msi_desc)
+{
+	msi_entry->address.as_uint32 = msi_desc->msg.address_lo;
+	msi_entry->data.as_uint32 = msi_desc->msg.data;
+}
+
+int hv_msi_prepare(struct irq_domain *domain, struct device *dev,
+		   int nvec, msi_alloc_info_t *info)
+{
+	return pci_msi_prepare(domain, dev, nvec, info);
+}
+
+#endif
diff --git a/drivers/pci/controller/pci-hyperv-irqchip.h b/drivers/pci/controller/pci-hyperv-irqchip.h
new file mode 100644
index 000000000000..8fbf17f03385
--- /dev/null
+++ b/drivers/pci/controller/pci-hyperv-irqchip.h
@@ -0,0 +1,21 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+/*
+ * Architecture specific vector management for the Hyper-V vPCI.
+ *
+ * Copyright (C) 2021, Microsoft, Inc.
+ *
+ * Author : Sunil Muthuswamy <sunilmut@microsoft.com>
+ */
+
+int hv_pci_irqchip_init(struct irq_domain **parent_domain,
+			bool *fasteoi_handler,
+			u8 *delivery_mode);
+
+void hv_pci_irqchip_free(void);
+unsigned int hv_msi_get_int_vector(struct irq_data *data);
+void hv_set_msi_entry_from_desc(union hv_msi_entry *msi_entry,
+				struct msi_desc *msi_desc);
+
+int hv_msi_prepare(struct irq_domain *domain, struct device *dev,
+		   int nvec, msi_alloc_info_t *info);
diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
index eaec915ffe62..2d3916206986 100644
--- a/drivers/pci/controller/pci-hyperv.c
+++ b/drivers/pci/controller/pci-hyperv.c
@@ -43,14 +43,12 @@
 #include <linux/pci-ecam.h>
 #include <linux/delay.h>
 #include <linux/semaphore.h>
-#include <linux/irqdomain.h>
-#include <asm/irqdomain.h>
-#include <asm/apic.h>
 #include <linux/irq.h>
 #include <linux/msi.h>
 #include <linux/hyperv.h>
 #include <linux/refcount.h>
 #include <asm/mshyperv.h>
+#include "pci-hyperv-irqchip.h"
 
 /*
  * Protocol versions. The low word is the minor version, the high word the
@@ -81,6 +79,10 @@ static enum pci_protocol_version_t pci_protocol_versions[] = {
 	PCI_PROTOCOL_VERSION_1_1,
 };
 
+static struct irq_domain *parent_domain;
+static bool fasteoi;
+static u8 delivery_mode;
+
 #define PCI_CONFIG_MMIO_LENGTH	0x2000
 #define CFG_PAGE_OFFSET 0x1000
 #define CFG_PAGE_SIZE (PCI_CONFIG_MMIO_LENGTH - CFG_PAGE_OFFSET)
@@ -1217,7 +1219,6 @@ static void hv_irq_mask(struct irq_data *data)
 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 hv_retarget_device_interrupt *params;
 	struct hv_pcibus_device *hbus;
 	struct cpumask *dest;
@@ -1246,11 +1247,12 @@ static void hv_irq_unmask(struct irq_data *data)
 			   (hbus->hdev->dev_instance.b[7] << 8) |
 			   (hbus->hdev->dev_instance.b[6] & 0xf8) |
 			   PCI_FUNC(pdev->devfn);
-	params->int_target.vector = cfg->vector;
+	params->int_target.vector = hv_msi_get_int_vector(data);
 
 	/*
-	 * Honoring apic->delivery_mode set to APIC_DELIVERY_MODE_FIXED by
-	 * setting the HV_DEVICE_INTERRUPT_TARGET_MULTICAST flag results in a
+	 * For x64, honoring apic->delivery_mode set to
+	 * APIC_DELIVERY_MODE_FIXED by setting the
+	 * HV_DEVICE_INTERRUPT_TARGET_MULTICAST flag results in a
 	 * spurious interrupt storm. Not doing so does not seem to have a
 	 * negative effect (yet?).
 	 */
@@ -1347,7 +1349,7 @@ static u32 hv_compose_msi_req_v1(
 	int_pkt->wslot.slot = slot;
 	int_pkt->int_desc.vector = vector;
 	int_pkt->int_desc.vector_count = 1;
-	int_pkt->int_desc.delivery_mode = APIC_DELIVERY_MODE_FIXED;
+	int_pkt->int_desc.delivery_mode = delivery_mode;
 
 	/*
 	 * Create MSI w/ dummy vCPU set, overwritten by subsequent retarget in
@@ -1377,7 +1379,7 @@ static u32 hv_compose_msi_req_v2(
 	int_pkt->wslot.slot = slot;
 	int_pkt->int_desc.vector = vector;
 	int_pkt->int_desc.vector_count = 1;
-	int_pkt->int_desc.delivery_mode = APIC_DELIVERY_MODE_FIXED;
+	int_pkt->int_desc.delivery_mode = delivery_mode;
 	cpu = hv_compose_msi_req_get_cpu(affinity);
 	int_pkt->int_desc.processor_array[0] =
 		hv_cpu_number_to_vp_number(cpu);
@@ -1397,7 +1399,7 @@ static u32 hv_compose_msi_req_v3(
 	int_pkt->int_desc.vector = vector;
 	int_pkt->int_desc.reserved = 0;
 	int_pkt->int_desc.vector_count = 1;
-	int_pkt->int_desc.delivery_mode = APIC_DELIVERY_MODE_FIXED;
+	int_pkt->int_desc.delivery_mode = delivery_mode;
 	cpu = hv_compose_msi_req_get_cpu(affinity);
 	int_pkt->int_desc.processor_array[0] =
 		hv_cpu_number_to_vp_number(cpu);
@@ -1419,7 +1421,6 @@ static u32 hv_compose_msi_req_v3(
  */
 static void hv_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
 {
-	struct irq_cfg *cfg = irqd_cfg(data);
 	struct hv_pcibus_device *hbus;
 	struct vmbus_channel *channel;
 	struct hv_pci_dev *hpdev;
@@ -1470,7 +1471,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,
-					cfg->vector);
+					hv_msi_get_int_vector(data));
 		break;
 
 	case PCI_PROTOCOL_VERSION_1_2:
@@ -1478,14 +1479,14 @@ 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,
-					cfg->vector);
+					hv_msi_get_int_vector(data));
 		break;
 
 	case PCI_PROTOCOL_VERSION_1_4:
 		size = hv_compose_msi_req_v3(&ctxt.int_pkts.v3,
 					dest,
 					hpdev->desc.win_slot.slot,
-					cfg->vector);
+					hv_msi_get_int_vector(data));
 		break;
 
 	default:
@@ -1601,7 +1602,7 @@ static struct irq_chip hv_msi_irq_chip = {
 };
 
 static struct msi_domain_ops hv_msi_ops = {
-	.msi_prepare	= pci_msi_prepare,
+	.msi_prepare	= hv_msi_prepare,
 	.msi_free	= hv_msi_free,
 };
 
@@ -1625,12 +1626,13 @@ static int hv_pcie_init_irq_domain(struct hv_pcibus_device *hbus)
 	hbus->msi_info.flags = (MSI_FLAG_USE_DEF_DOM_OPS |
 		MSI_FLAG_USE_DEF_CHIP_OPS | MSI_FLAG_MULTI_PCI_MSI |
 		MSI_FLAG_PCI_MSIX);
-	hbus->msi_info.handler = handle_edge_irq;
-	hbus->msi_info.handler_name = "edge";
+	hbus->msi_info.handler =
+		fasteoi ? handle_fasteoi_irq : handle_edge_irq;
+	hbus->msi_info.handler_name = fasteoi ? "fasteoi" : "edge";
 	hbus->msi_info.data = hbus;
 	hbus->irq_domain = pci_msi_create_irq_domain(hbus->fwnode,
 						     &hbus->msi_info,
-						     x86_vector_domain);
+						     parent_domain);
 	if (!hbus->irq_domain) {
 		dev_err(&hbus->hdev->device,
 			"Failed to build an MSI IRQ domain\n");
@@ -3531,13 +3533,21 @@ static void __exit exit_hv_pci_drv(void)
 	hvpci_block_ops.read_block = NULL;
 	hvpci_block_ops.write_block = NULL;
 	hvpci_block_ops.reg_blk_invalidate = NULL;
+
+	hv_pci_irqchip_free();
 }
 
 static int __init init_hv_pci_drv(void)
 {
+	int ret;
+
 	if (!hv_is_hyperv_initialized())
 		return -ENODEV;
 
+	ret = hv_pci_irqchip_init(&parent_domain, &fasteoi, &delivery_mode);
+	if (ret)
+		return ret;
+
 	/* Set the invalid domain number's bit, so it will not be used */
 	set_bit(HVPCI_DOM_INVALID, hvpci_dom_map);
 
@@ -3546,7 +3556,11 @@ static int __init init_hv_pci_drv(void)
 	hvpci_block_ops.write_block = hv_write_config_block;
 	hvpci_block_ops.reg_blk_invalidate = hv_register_block_invalidate;
 
-	return vmbus_driver_register(&hv_pci_drv);
+	ret = vmbus_driver_register(&hv_pci_drv);
+	if (ret)
+		hv_pci_irqchip_free();
+
+	return ret;
 }
 
 module_init(init_hv_pci_drv);
diff --git a/include/asm-generic/hyperv-tlfs.h b/include/asm-generic/hyperv-tlfs.h
index 56348a541c50..45cc0c3b8ed7 100644
--- a/include/asm-generic/hyperv-tlfs.h
+++ b/include/asm-generic/hyperv-tlfs.h
@@ -539,39 +539,6 @@ enum hv_interrupt_source {
 	HV_INTERRUPT_SOURCE_IOAPIC,
 };
 
-union hv_msi_address_register {
-	u32 as_uint32;
-	struct {
-		u32 reserved1:2;
-		u32 destination_mode:1;
-		u32 redirection_hint:1;
-		u32 reserved2:8;
-		u32 destination_id:8;
-		u32 msi_base:12;
-	};
-} __packed;
-
-union hv_msi_data_register {
-	u32 as_uint32;
-	struct {
-		u32 vector:8;
-		u32 delivery_mode:3;
-		u32 reserved1:3;
-		u32 level_assert:1;
-		u32 trigger_mode:1;
-		u32 reserved2:16;
-	};
-} __packed;
-
-/* HvRetargetDeviceInterrupt hypercall */
-union hv_msi_entry {
-	u64 as_uint64;
-	struct {
-		union hv_msi_address_register address;
-		union hv_msi_data_register data;
-	} __packed;
-};
-
 union hv_ioapic_rte {
 	u64 as_uint64;
 
-- 
2.25.1

Re: [PATCH v2 1/2] PCI: hv: Make the code arch neutral

[Bjorn Helgaas]

Can you put some specifics in the subject line, please?  If this patch
does several things, that might be an indication that it should be
split into several patches.

On Fri, Oct 08, 2021 at 05:20:35PM +0000, Sunil Muthuswamy wrote:
> This patch makes the Hyper-V vPCI code architectural neutral by
> introducing an irqchip that takes care of architectural
> dependencies. This allows for the implementation of Hyper-V vPCI
> for other architecture such as ARM64.

No need to include "This patch"; we already know we're talking about
this patch.

Write in "imperative mood", e.g., "Encapsulate arch dependencies in
X ..." instead of "This patch makes the code ...".  See
https://chris.beams.io/posts/git-commit/

Wrap the text to fill 75 columns.

You said this "introduces an irqchip", but I don't see a new
struct irq_chip or similar.

The important part about making this arch-neutral seems to be adding
these interfaces that will encapsulate arch dependencies:

  hv_pci_irqchip_init()
  hv_pci_irqchip_free()
  hv_msi_get_int_vector()
  hv_set_msi_entry_from_desc()
  hv_msi_prepare()

I'm not sure wrapping them in "#ifdef CONFIG_X86_64" is the best
approach, but the IRQ folks will know better.

> +++ b/drivers/pci/controller/pci-hyperv-irqchip.c
> @@ -0,0 +1,51 @@
> +// SPDX-License-Identifier: GPL-2.0
> +

Spurious blank line (follow style of nearby files).

> +/*
> + * Hyper-V vPCI irqchip.

> +++ b/drivers/pci/controller/pci-hyperv-irqchip.h
> @@ -0,0 +1,21 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +

Spurious blank line (follow style of nearby files).

> +/*
> + * Architecture specific vector management for the Hyper-V vPCI.

RE: [EXTERNAL] Re: [PATCH v2 1/2] PCI: hv: Make the code arch neutral

[Sunil Muthuswamy]

On Friday, October 8, 2021 12:17 PM
Bjorn Helgaas <helgaas@kernel.org> wrote:

> Can you put some specifics in the subject line, please?  If this patch
> does several things, that might be an indication that it should be
> split into several patches.
> 
Will do in v3. But, this patch only makes the current Hyper-V vPCI code
architectural neutral by adding those interfaces that you mention below.
I will update the wording to make this clear in v3. thanks for the suggestion.

> > This patch makes the Hyper-V vPCI code architectural neutral by
> > introducing an irqchip that takes care of architectural
> > dependencies. This allows for the implementation of Hyper-V vPCI
> > for other architecture such as ARM64.
> 
> No need to include "This patch"; we already know we're talking about
> this patch.
> 
> Write in "imperative mood", e.g., "Encapsulate arch dependencies in
> X ..." instead of "This patch makes the code ...".  See
> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fchris.be
> ams.io%2Fposts%2Fgit-
> commit%2F&amp;data=04%7C01%7Csunilmut%40microsoft.com%7Cb40962bf1
> 5614957781608d98a9030c8%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C
> 0%7C637693174187863024%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjA
> wMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sda
> ta=XlrM%2Bzg92v%2BXxXObWQbznaB%2F6ZW6u79NtDoG4FfyhXw%3D&amp;r
> eserved=0
> 
> Wrap the text to fill 75 columns.
> 
All of the above points noted; thanks. Will address in v3.

> You said this "introduces an irqchip", but I don't see a new
> struct irq_chip or similar.
> 
> The important part about making this arch-neutral seems to be adding
> these interfaces that will encapsulate arch dependencies:
> 
>   hv_pci_irqchip_init()
>   hv_pci_irqchip_free()
>   hv_msi_get_int_vector()
>   hv_set_msi_entry_from_desc()
>   hv_msi_prepare()
> 
Yes, the wording of 'irqchip' is a bit misleading for this patch as it gets
added in 2/2. Thanks for the suggestion; I will reword in v3.

> I'm not sure wrapping them in "#ifdef CONFIG_X86_64" is the best
> approach, but the IRQ folks will know better.
> 
Hyper-V vPCI is not firmware enumerated and so we cannot take a
dependency on the firmware to dictate whether or not to instantiate
an irqchip or not. I am open to suggestions though.

> > +++ b/drivers/pci/controller/pci-hyperv-irqchip.c
> > @@ -0,0 +1,51 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> 
> Spurious blank line (follow style of nearby files).
> 
Noted, thanks. Will fix in v3.

> > +/*
> > + * Hyper-V vPCI irqchip.
> 
> > +++ b/drivers/pci/controller/pci-hyperv-irqchip.h
> > @@ -0,0 +1,21 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +
> 
> Spurious blank line (follow style of nearby files).
> 
Noted, thanks. Will fix in v3.

- Sunil

Re: [PATCH v2 1/2] PCI: hv: Make the code arch neutral

[Boqun Feng]

Hi,

On Fri, Oct 08, 2021 at 05:20:35PM +0000, Sunil Muthuswamy wrote:
> This patch makes the Hyper-V vPCI code architectural neutral by
> introducing an irqchip that takes care of architectural
> dependencies. This allows for the implementation of Hyper-V vPCI
> for other architecture such as ARM64.
> 
> There are no functional changes expected from this patch.
> 

As reported by Baihua (I can also reproduce), compile errors are hit
when compiling with CONFIG_PCI_HYPERV=m:

ERROR: modpost: missing MODULE_LICENSE() in drivers/pci/controller/pci-hyperv-irqchip.o
ERROR: modpost: "hv_msi_prepare" [drivers/pci/controller/pci-hyperv.ko] undefined!
ERROR: modpost: "hv_pci_irqchip_init" [drivers/pci/controller/pci-hyperv.ko] undefined!
ERROR: modpost: "hv_pci_irqchip_free" [drivers/pci/controller/pci-hyperv.ko] undefined!
ERROR: modpost: "hv_msi_get_int_vector" [drivers/pci/controller/pci-hyperv.ko] undefined!
ERROR: modpost: "hv_set_msi_entry_from_desc" [drivers/pci/controller/pci-hyperv.ko] undefined!

It means that we MODULE_LICENSE() should be added in
pci-hyperv-irqchip.c, also these symbols should be exported.

Regards,
Boqun

> Signed-off-by: Sunil Muthuswamy <sunilmut@microsoft.com>
> ---
> In v2:
>  v2 changes are described in the cover letter.
> 
>  MAINTAINERS                                 |  2 +
>  arch/x86/include/asm/hyperv-tlfs.h          | 33 +++++++++++++
>  arch/x86/include/asm/mshyperv.h             |  7 ---
>  drivers/pci/controller/Makefile             |  2 +-
>  drivers/pci/controller/pci-hyperv-irqchip.c | 51 ++++++++++++++++++++
>  drivers/pci/controller/pci-hyperv-irqchip.h | 21 +++++++++
>  drivers/pci/controller/pci-hyperv.c         | 52 +++++++++++++--------
>  include/asm-generic/hyperv-tlfs.h           | 33 -------------
>  8 files changed, 141 insertions(+), 60 deletions(-)
>  create mode 100644 drivers/pci/controller/pci-hyperv-irqchip.c
>  create mode 100644 drivers/pci/controller/pci-hyperv-irqchip.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index ca6d6fde85cf..ba8c979c17b2 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -8688,6 +8688,8 @@ F:	drivers/iommu/hyperv-iommu.c
>  F:	drivers/net/ethernet/microsoft/
>  F:	drivers/net/hyperv/
>  F:	drivers/pci/controller/pci-hyperv-intf.c
> +F:	drivers/pci/controller/pci-hyperv-irqchip.c
> +F:	drivers/pci/controller/pci-hyperv-irqchip.h
>  F:	drivers/pci/controller/pci-hyperv.c
>  F:	drivers/scsi/storvsc_drv.c
>  F:	drivers/uio/uio_hv_generic.c
> diff --git a/arch/x86/include/asm/hyperv-tlfs.h b/arch/x86/include/asm/hyperv-tlfs.h
> index 2322d6bd5883..fdf3d28fbdd5 100644
> --- a/arch/x86/include/asm/hyperv-tlfs.h
> +++ b/arch/x86/include/asm/hyperv-tlfs.h
> @@ -585,6 +585,39 @@ enum hv_interrupt_type {
>  	HV_X64_INTERRUPT_TYPE_MAXIMUM           = 0x000A,
>  };
>  
> +union hv_msi_address_register {
> +	u32 as_uint32;
> +	struct {
> +		u32 reserved1:2;
> +		u32 destination_mode:1;
> +		u32 redirection_hint:1;
> +		u32 reserved2:8;
> +		u32 destination_id:8;
> +		u32 msi_base:12;
> +	};
> +} __packed;
> +
> +union hv_msi_data_register {
> +	u32 as_uint32;
> +	struct {
> +		u32 vector:8;
> +		u32 delivery_mode:3;
> +		u32 reserved1:3;
> +		u32 level_assert:1;
> +		u32 trigger_mode:1;
> +		u32 reserved2:16;
> +	};
> +} __packed;
> +
> +/* HvRetargetDeviceInterrupt hypercall */
> +union hv_msi_entry {
> +	u64 as_uint64;
> +	struct {
> +		union hv_msi_address_register address;
> +		union hv_msi_data_register data;
> +	} __packed;
> +};
> +
>  #include <asm-generic/hyperv-tlfs.h>
>  
>  #endif
> diff --git a/arch/x86/include/asm/mshyperv.h b/arch/x86/include/asm/mshyperv.h
> index adccbc209169..c2b9ab94408e 100644
> --- a/arch/x86/include/asm/mshyperv.h
> +++ b/arch/x86/include/asm/mshyperv.h
> @@ -176,13 +176,6 @@ 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.as_uint32 = msi_desc->msg.address_lo;
> -	msi_entry->data.as_uint32 = msi_desc->msg.data;
> -}
> -
>  struct irq_domain *hv_create_pci_msi_domain(void);
>  
>  int hv_map_ioapic_interrupt(int ioapic_id, bool level, int vcpu, int vector,
> diff --git a/drivers/pci/controller/Makefile b/drivers/pci/controller/Makefile
> index aaf30b3dcc14..2c301d0fc23b 100644
> --- a/drivers/pci/controller/Makefile
> +++ b/drivers/pci/controller/Makefile
> @@ -2,7 +2,7 @@
>  obj-$(CONFIG_PCIE_CADENCE) += cadence/
>  obj-$(CONFIG_PCI_FTPCI100) += pci-ftpci100.o
>  obj-$(CONFIG_PCI_IXP4XX) += pci-ixp4xx.o
> -obj-$(CONFIG_PCI_HYPERV) += pci-hyperv.o
> +obj-$(CONFIG_PCI_HYPERV) += pci-hyperv.o pci-hyperv-irqchip.o
>  obj-$(CONFIG_PCI_HYPERV_INTERFACE) += pci-hyperv-intf.o
>  obj-$(CONFIG_PCI_MVEBU) += pci-mvebu.o
>  obj-$(CONFIG_PCI_AARDVARK) += pci-aardvark.o
> diff --git a/drivers/pci/controller/pci-hyperv-irqchip.c b/drivers/pci/controller/pci-hyperv-irqchip.c
> new file mode 100644
> index 000000000000..5f334f7d66cb
> --- /dev/null
> +++ b/drivers/pci/controller/pci-hyperv-irqchip.c
> @@ -0,0 +1,51 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +/*
> + * Hyper-V vPCI irqchip.
> + *
> + * Copyright (C) 2021, Microsoft, Inc.
> + *
> + * Author : Sunil Muthuswamy <sunilmut@microsoft.com>
> + */
> +
> +#include <asm/mshyperv.h>
> +#include <linux/acpi.h>
> +#include <linux/irqdomain.h>
> +#include <linux/irq.h>
> +#include <linux/msi.h>
> +
> +#ifdef CONFIG_X86_64
> +int hv_pci_irqchip_init(struct irq_domain **parent_domain,
> +			bool *fasteoi_handler,
> +			u8 *delivery_mode)
> +{
> +	*parent_domain = x86_vector_domain;
> +	*fasteoi_handler = false;
> +	*delivery_mode = APIC_DELIVERY_MODE_FIXED;
> +
> +	return 0;
> +}
> +
> +void hv_pci_irqchip_free(void) {}
> +
> +unsigned int hv_msi_get_int_vector(struct irq_data *data)
> +{
> +	struct irq_cfg *cfg = irqd_cfg(data);
> +
> +	return cfg->vector;
> +}
> +
> +void hv_set_msi_entry_from_desc(union hv_msi_entry *msi_entry,
> +				struct msi_desc *msi_desc)
> +{
> +	msi_entry->address.as_uint32 = msi_desc->msg.address_lo;
> +	msi_entry->data.as_uint32 = msi_desc->msg.data;
> +}
> +
> +int hv_msi_prepare(struct irq_domain *domain, struct device *dev,
> +		   int nvec, msi_alloc_info_t *info)
> +{
> +	return pci_msi_prepare(domain, dev, nvec, info);
> +}
> +
> +#endif
> diff --git a/drivers/pci/controller/pci-hyperv-irqchip.h b/drivers/pci/controller/pci-hyperv-irqchip.h
> new file mode 100644
> index 000000000000..8fbf17f03385
> --- /dev/null
> +++ b/drivers/pci/controller/pci-hyperv-irqchip.h
> @@ -0,0 +1,21 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +/*
> + * Architecture specific vector management for the Hyper-V vPCI.
> + *
> + * Copyright (C) 2021, Microsoft, Inc.
> + *
> + * Author : Sunil Muthuswamy <sunilmut@microsoft.com>
> + */
> +
> +int hv_pci_irqchip_init(struct irq_domain **parent_domain,
> +			bool *fasteoi_handler,
> +			u8 *delivery_mode);
> +
> +void hv_pci_irqchip_free(void);
> +unsigned int hv_msi_get_int_vector(struct irq_data *data);
> +void hv_set_msi_entry_from_desc(union hv_msi_entry *msi_entry,
> +				struct msi_desc *msi_desc);
> +
> +int hv_msi_prepare(struct irq_domain *domain, struct device *dev,
> +		   int nvec, msi_alloc_info_t *info);
> diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
> index eaec915ffe62..2d3916206986 100644
> --- a/drivers/pci/controller/pci-hyperv.c
> +++ b/drivers/pci/controller/pci-hyperv.c
> @@ -43,14 +43,12 @@
>  #include <linux/pci-ecam.h>
>  #include <linux/delay.h>
>  #include <linux/semaphore.h>
> -#include <linux/irqdomain.h>
> -#include <asm/irqdomain.h>
> -#include <asm/apic.h>
>  #include <linux/irq.h>
>  #include <linux/msi.h>
>  #include <linux/hyperv.h>
>  #include <linux/refcount.h>
>  #include <asm/mshyperv.h>
> +#include "pci-hyperv-irqchip.h"
>  
>  /*
>   * Protocol versions. The low word is the minor version, the high word the
> @@ -81,6 +79,10 @@ static enum pci_protocol_version_t pci_protocol_versions[] = {
>  	PCI_PROTOCOL_VERSION_1_1,
>  };
>  
> +static struct irq_domain *parent_domain;
> +static bool fasteoi;
> +static u8 delivery_mode;
> +
>  #define PCI_CONFIG_MMIO_LENGTH	0x2000
>  #define CFG_PAGE_OFFSET 0x1000
>  #define CFG_PAGE_SIZE (PCI_CONFIG_MMIO_LENGTH - CFG_PAGE_OFFSET)
> @@ -1217,7 +1219,6 @@ static void hv_irq_mask(struct irq_data *data)
>  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 hv_retarget_device_interrupt *params;
>  	struct hv_pcibus_device *hbus;
>  	struct cpumask *dest;
> @@ -1246,11 +1247,12 @@ static void hv_irq_unmask(struct irq_data *data)
>  			   (hbus->hdev->dev_instance.b[7] << 8) |
>  			   (hbus->hdev->dev_instance.b[6] & 0xf8) |
>  			   PCI_FUNC(pdev->devfn);
> -	params->int_target.vector = cfg->vector;
> +	params->int_target.vector = hv_msi_get_int_vector(data);
>  
>  	/*
> -	 * Honoring apic->delivery_mode set to APIC_DELIVERY_MODE_FIXED by
> -	 * setting the HV_DEVICE_INTERRUPT_TARGET_MULTICAST flag results in a
> +	 * For x64, honoring apic->delivery_mode set to
> +	 * APIC_DELIVERY_MODE_FIXED by setting the
> +	 * HV_DEVICE_INTERRUPT_TARGET_MULTICAST flag results in a
>  	 * spurious interrupt storm. Not doing so does not seem to have a
>  	 * negative effect (yet?).
>  	 */
> @@ -1347,7 +1349,7 @@ static u32 hv_compose_msi_req_v1(
>  	int_pkt->wslot.slot = slot;
>  	int_pkt->int_desc.vector = vector;
>  	int_pkt->int_desc.vector_count = 1;
> -	int_pkt->int_desc.delivery_mode = APIC_DELIVERY_MODE_FIXED;
> +	int_pkt->int_desc.delivery_mode = delivery_mode;
>  
>  	/*
>  	 * Create MSI w/ dummy vCPU set, overwritten by subsequent retarget in
> @@ -1377,7 +1379,7 @@ static u32 hv_compose_msi_req_v2(
>  	int_pkt->wslot.slot = slot;
>  	int_pkt->int_desc.vector = vector;
>  	int_pkt->int_desc.vector_count = 1;
> -	int_pkt->int_desc.delivery_mode = APIC_DELIVERY_MODE_FIXED;
> +	int_pkt->int_desc.delivery_mode = delivery_mode;
>  	cpu = hv_compose_msi_req_get_cpu(affinity);
>  	int_pkt->int_desc.processor_array[0] =
>  		hv_cpu_number_to_vp_number(cpu);
> @@ -1397,7 +1399,7 @@ static u32 hv_compose_msi_req_v3(
>  	int_pkt->int_desc.vector = vector;
>  	int_pkt->int_desc.reserved = 0;
>  	int_pkt->int_desc.vector_count = 1;
> -	int_pkt->int_desc.delivery_mode = APIC_DELIVERY_MODE_FIXED;
> +	int_pkt->int_desc.delivery_mode = delivery_mode;
>  	cpu = hv_compose_msi_req_get_cpu(affinity);
>  	int_pkt->int_desc.processor_array[0] =
>  		hv_cpu_number_to_vp_number(cpu);
> @@ -1419,7 +1421,6 @@ static u32 hv_compose_msi_req_v3(
>   */
>  static void hv_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
>  {
> -	struct irq_cfg *cfg = irqd_cfg(data);
>  	struct hv_pcibus_device *hbus;
>  	struct vmbus_channel *channel;
>  	struct hv_pci_dev *hpdev;
> @@ -1470,7 +1471,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,
> -					cfg->vector);
> +					hv_msi_get_int_vector(data));
>  		break;
>  
>  	case PCI_PROTOCOL_VERSION_1_2:
> @@ -1478,14 +1479,14 @@ 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,
> -					cfg->vector);
> +					hv_msi_get_int_vector(data));
>  		break;
>  
>  	case PCI_PROTOCOL_VERSION_1_4:
>  		size = hv_compose_msi_req_v3(&ctxt.int_pkts.v3,
>  					dest,
>  					hpdev->desc.win_slot.slot,
> -					cfg->vector);
> +					hv_msi_get_int_vector(data));
>  		break;
>  
>  	default:
> @@ -1601,7 +1602,7 @@ static struct irq_chip hv_msi_irq_chip = {
>  };
>  
>  static struct msi_domain_ops hv_msi_ops = {
> -	.msi_prepare	= pci_msi_prepare,
> +	.msi_prepare	= hv_msi_prepare,
>  	.msi_free	= hv_msi_free,
>  };
>  
> @@ -1625,12 +1626,13 @@ static int hv_pcie_init_irq_domain(struct hv_pcibus_device *hbus)
>  	hbus->msi_info.flags = (MSI_FLAG_USE_DEF_DOM_OPS |
>  		MSI_FLAG_USE_DEF_CHIP_OPS | MSI_FLAG_MULTI_PCI_MSI |
>  		MSI_FLAG_PCI_MSIX);
> -	hbus->msi_info.handler = handle_edge_irq;
> -	hbus->msi_info.handler_name = "edge";
> +	hbus->msi_info.handler =
> +		fasteoi ? handle_fasteoi_irq : handle_edge_irq;
> +	hbus->msi_info.handler_name = fasteoi ? "fasteoi" : "edge";
>  	hbus->msi_info.data = hbus;
>  	hbus->irq_domain = pci_msi_create_irq_domain(hbus->fwnode,
>  						     &hbus->msi_info,
> -						     x86_vector_domain);
> +						     parent_domain);
>  	if (!hbus->irq_domain) {
>  		dev_err(&hbus->hdev->device,
>  			"Failed to build an MSI IRQ domain\n");
> @@ -3531,13 +3533,21 @@ static void __exit exit_hv_pci_drv(void)
>  	hvpci_block_ops.read_block = NULL;
>  	hvpci_block_ops.write_block = NULL;
>  	hvpci_block_ops.reg_blk_invalidate = NULL;
> +
> +	hv_pci_irqchip_free();
>  }
>  
>  static int __init init_hv_pci_drv(void)
>  {
> +	int ret;
> +
>  	if (!hv_is_hyperv_initialized())
>  		return -ENODEV;
>  
> +	ret = hv_pci_irqchip_init(&parent_domain, &fasteoi, &delivery_mode);
> +	if (ret)
> +		return ret;
> +
>  	/* Set the invalid domain number's bit, so it will not be used */
>  	set_bit(HVPCI_DOM_INVALID, hvpci_dom_map);
>  
> @@ -3546,7 +3556,11 @@ static int __init init_hv_pci_drv(void)
>  	hvpci_block_ops.write_block = hv_write_config_block;
>  	hvpci_block_ops.reg_blk_invalidate = hv_register_block_invalidate;
>  
> -	return vmbus_driver_register(&hv_pci_drv);
> +	ret = vmbus_driver_register(&hv_pci_drv);
> +	if (ret)
> +		hv_pci_irqchip_free();
> +
> +	return ret;
>  }
>  
>  module_init(init_hv_pci_drv);
> diff --git a/include/asm-generic/hyperv-tlfs.h b/include/asm-generic/hyperv-tlfs.h
> index 56348a541c50..45cc0c3b8ed7 100644
> --- a/include/asm-generic/hyperv-tlfs.h
> +++ b/include/asm-generic/hyperv-tlfs.h
> @@ -539,39 +539,6 @@ enum hv_interrupt_source {
>  	HV_INTERRUPT_SOURCE_IOAPIC,
>  };
>  
> -union hv_msi_address_register {
> -	u32 as_uint32;
> -	struct {
> -		u32 reserved1:2;
> -		u32 destination_mode:1;
> -		u32 redirection_hint:1;
> -		u32 reserved2:8;
> -		u32 destination_id:8;
> -		u32 msi_base:12;
> -	};
> -} __packed;
> -
> -union hv_msi_data_register {
> -	u32 as_uint32;
> -	struct {
> -		u32 vector:8;
> -		u32 delivery_mode:3;
> -		u32 reserved1:3;
> -		u32 level_assert:1;
> -		u32 trigger_mode:1;
> -		u32 reserved2:16;
> -	};
> -} __packed;
> -
> -/* HvRetargetDeviceInterrupt hypercall */
> -union hv_msi_entry {
> -	u64 as_uint64;
> -	struct {
> -		union hv_msi_address_register address;
> -		union hv_msi_data_register data;
> -	} __packed;
> -};
> -
>  union hv_ioapic_rte {
>  	u64 as_uint64;
>  
> -- 
> 2.25.1
> 

RE: [EXTERNAL] Re: [PATCH v2 1/2] PCI: hv: Make the code arch neutral

[Sunil Muthuswamy]

On Tuesday, October 12, 2021 11:06 PM,
Boqun Feng <boqun.feng@gmail.com> wrote:

> As reported by Baihua (I can also reproduce), compile errors are hit
> when compiling with CONFIG_PCI_HYPERV=m:
> 
> ERROR: modpost: missing MODULE_LICENSE() in drivers/pci/controller/pci-
> hyperv-irqchip.o
> ERROR: modpost: "hv_msi_prepare" [drivers/pci/controller/pci-hyperv.ko]
> undefined!
> ERROR: modpost: "hv_pci_irqchip_init" [drivers/pci/controller/pci-hyperv.ko]
> undefined!
> ERROR: modpost: "hv_pci_irqchip_free" [drivers/pci/controller/pci-hyperv.ko]
> undefined!
> ERROR: modpost: "hv_msi_get_int_vector" [drivers/pci/controller/pci-
> hyperv.ko] undefined!
> ERROR: modpost: "hv_set_msi_entry_from_desc" [drivers/pci/controller/pci-
> hyperv.ko] undefined!
> 
> It means that we MODULE_LICENSE() should be added in
> pci-hyperv-irqchip.c, also these symbols should be exported.
> 
Thanks for catching this. Will fix in v3.

- Sunil