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

[Wei Liu <wei.liu@kernel.org>]

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
> > 
> > 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 <sunilmut@microsoft.com>
> > Co-Developed-by: Sunil Muthuswamy <sunilmut@microsoft.com>
> > 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.

> 
[...]
> > 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.

> 
> > +
> > +#include <linux/pci.h>
> > +#include <linux/irq.h>
> > +#include <asm/mshyperv.h>
> > +
[...]
> > +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.

> > +
> > +	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;
> > +}
> > +
[...]
> > +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.