Re: [PATCH v5 15/16] x86/hyperv: implement an MSI domain for root partition

On Wed, Jan 27, 2021 at 05:47:04AM +0000, Michael Kelley wrote:

From: Wei Liu <wei.liu@kernel.org> Sent: Wednesday, January 20, 2021 4:01 AM

quoted

When Linux runs as the root partition on Microsoft Hypervisor, its
interrupts are remapped.  Linux will need to explicitly map and unmap
interrupts for hardware.

Implement an MSI domain to issue the correct hypercalls. And initialize
this irqdomain as the default MSI irq domain.

Signed-off-by: Sunil Muthuswamy <redacted>
Co-Developed-by: Sunil Muthuswamy <redacted>
Signed-off-by: Wei Liu <wei.liu@kernel.org>
---
v4: Fix compilation issue when CONFIG_PCI_MSI is not set.
v3: build irqdomain.o for 32bit as well.

I'm not clear on the intent for 32-bit builds.  Given that hv_proc.c is built
only for 64-bit, I'm assuming running Linux in the root partition
is only functional for 64-bit builds.  So is the goal simply that 32-bit
builds will compile correctly?  Seems like maybe there should be
a CONFIG option for running Linux in the root partition, and that
option would force 64-bit.

To ensure 32 bit kernel builds and 32 bit guests still work.

The config option ROOT_API is to be introduced by Nuno's /dev/mshv
series. We can use that option to gate some objects when that's
available.

[...]

quoted

diff --git a/arch/x86/hyperv/irqdomain.c b/arch/x86/hyperv/irqdomain.c
new file mode 100644
index 000000000000..19637cd60231
--- /dev/null
+++ b/arch/x86/hyperv/irqdomain.c

@@ -0,0 +1,332 @@
+// SPDX-License-Identifier: GPL-2.0
+//
+// Irqdomain for Linux to run as the root partition on Microsoft Hypervisor.
+//
+// Authors:
+//   Sunil Muthuswamy <sunilmut@microsoft.com>
+//   Wei Liu <wei.liu@kernel.org>

I think the // comment style should only be used for the SPDX line.

Fixed.

quoted

+
+#include <linux/pci.h>
+#include <linux/irq.h>
+#include <asm/mshyperv.h>
+

[...]

quoted

+static int hv_map_msi_interrupt(struct pci_dev *dev, int vcpu, int vector,
+				struct hv_interrupt_entry *entry)
+{
+	struct hv_input_map_device_interrupt *input;
+	struct hv_output_map_device_interrupt *output;
+	struct hv_device_interrupt_descriptor *intr_desc;
+	unsigned long flags;
+	u16 status;
+
+	local_irq_save(flags);
+
+	input = *this_cpu_ptr(hyperv_pcpu_input_arg);
+	output = *this_cpu_ptr(hyperv_pcpu_output_arg);
+
+	intr_desc = &input->interrupt_descriptor;
+	memset(input, 0, sizeof(*input));
+	input->partition_id = hv_current_partition_id;
+	input->device_id = hv_build_pci_dev_id(dev).as_uint64;
+	intr_desc->interrupt_type = HV_X64_INTERRUPT_TYPE_FIXED;
+	intr_desc->trigger_mode = HV_INTERRUPT_TRIGGER_MODE_EDGE;
+	intr_desc->vector_count = 1;
+	intr_desc->target.vector = vector;
+	__set_bit(vcpu, (unsigned long*)&intr_desc->target.vp_mask);

This is using the CPU bitmap format that supports up to 64 vCPUs.  Any reason not
to use the format that supports a larger number of CPUs?   In either case, perhaps
a check for the value of vcpu against the max of 64 (or the larger number if you
change the bitmap format) would be appropriate.

This is mostly due to we didn't have a suitably large machine during
development.

I will see if this can use vpset instead.

quoted

+
+	status = hv_do_rep_hypercall(HVCALL_MAP_DEVICE_INTERRUPT, 0, 0, input, output) &
+			 HV_HYPERCALL_RESULT_MASK;
+	*entry = output->interrupt_entry;
+
+	local_irq_restore(flags);
+
+	if (status != HV_STATUS_SUCCESS)
+		pr_err("%s: hypercall failed, status %d\n", __func__, status);
+
+	return status;
+}
+

[...]

quoted

+static int hv_unmap_msi_interrupt(struct pci_dev *dev, struct hv_interrupt_entry
*old_entry)
+{
+	return hv_unmap_interrupt(hv_build_pci_dev_id(dev).as_uint64, old_entry)
+		& HV_HYPERCALL_RESULT_MASK;

The masking with HV_HYPERCALL_RESULT_MASK is already done in
hv_unmap_interrupt().

Fixed.

Wei.