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

[Michael Kelley]

From: Wei Liu <wei.liu@xxxxxxxxxx> Sent: Thursday, February 4, 2021 9:57 AM
> 
> On Thu, Feb 04, 2021 at 05:43:16PM +0000, Michael Kelley wrote:
> [...]
> > >  remove_cpuhp_state:
> > > diff --git a/arch/x86/hyperv/irqdomain.c b/arch/x86/hyperv/irqdomain.c
> > > new file mode 100644
> > > index 000000000000..117f17e8c88a
> > > --- /dev/null
> > > +++ b/arch/x86/hyperv/irqdomain.c
> > > @@ -0,0 +1,362 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +
> > > +/*
> > > + * for Linux to run as the root partition on Microsoft Hypervisor.
> >
> > Nit:  Looks like the initial word "Irqdomain" got dropped from the above
> > comment line.  But don't respin just for this.
> >
> 
> I've added it back. Thanks.
> 
> > > +static int hv_map_interrupt(union hv_device_id device_id, bool level,
> > > +		int cpu, 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;
> > > +	u64 status;
> > > +	cpumask_t mask = CPU_MASK_NONE;
> > > +	int nr_bank, var_size;
> > > +
> > > +	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 = device_id.as_uint64;
> > > +	intr_desc->interrupt_type = HV_X64_INTERRUPT_TYPE_FIXED;
> > > +	intr_desc->vector_count = 1;
> > > +	intr_desc->target.vector = vector;
> > > +
> > > +	if (level)
> > > +		intr_desc->trigger_mode = HV_INTERRUPT_TRIGGER_MODE_LEVEL;
> > > +	else
> > > +		intr_desc->trigger_mode = HV_INTERRUPT_TRIGGER_MODE_EDGE;
> > > +
> > > +	cpumask_set_cpu(cpu, &mask);
> > > +	intr_desc->target.vp_set.valid_bank_mask = 0;
> > > +	intr_desc->target.vp_set.format = HV_GENERIC_SET_SPARSE_4K;
> > > +	nr_bank = cpumask_to_vpset(&(intr_desc->target.vp_set), &mask);
> >
> > There's a function get_cpu_mask() that returns a pointer to a cpumask with only
> > the specified cpu set in the mask.  It returns a const pointer to the correct entry
> > in a pre-allocated array of all such cpumasks, so it's a lot more efficient than
> > allocating and initializing a local cpumask instance on the stack.
> >
> 
> That's nice.
> 
> I've got the following diff to fix both issues. If you're happy with the
> changes, can you give your Reviewed-by? That saves a round of posting.
> 
> diff --git a/arch/x86/hyperv/irqdomain.c b/arch/x86/hyperv/irqdomain.c
> index 0cabc9aece38..fa71db798465 100644
> --- a/arch/x86/hyperv/irqdomain.c
> +++ b/arch/x86/hyperv/irqdomain.c
> @@ -1,7 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0
> 
>  /*
> - * for Linux to run as the root partition on Microsoft Hypervisor.
> + * Irqdomain for Linux to run as the root partition on Microsoft Hypervisor.
>   *
>   * Authors:
>   *  Sunil Muthuswamy <sunilmut@xxxxxxxxxxxxx>
> @@ -20,7 +20,7 @@ static int hv_map_interrupt(union hv_device_id device_id, bool level,
>         struct hv_device_interrupt_descriptor *intr_desc;
>         unsigned long flags;
>         u64 status;
> -       cpumask_t mask = CPU_MASK_NONE;
> +       const cpumask_t *mask;
>         int nr_bank, var_size;
> 
>         local_irq_save(flags);
> @@ -41,10 +41,10 @@ static int hv_map_interrupt(union hv_device_id device_id, bool
> level,
>         else
>                 intr_desc->trigger_mode = HV_INTERRUPT_TRIGGER_MODE_EDGE;
> 
> -       cpumask_set_cpu(cpu, &mask);
> +       mask = cpumask_of(cpu);
>         intr_desc->target.vp_set.valid_bank_mask = 0;
>         intr_desc->target.vp_set.format = HV_GENERIC_SET_SPARSE_4K;
> -       nr_bank = cpumask_to_vpset(&(intr_desc->target.vp_set), &mask);
> +       nr_bank = cpumask_to_vpset(&(intr_desc->target.vp_set), mask);

Can you just do the following and get rid of the 'mask' local entirely?

nr_bank = cpumask_to_vpset(&(intr_desc->target.vp_set), cpumask_of(cpu));

Either way,

Reviewed-by: Michael Kelley <mikelley@xxxxxxxxxxxxx>

>         if (nr_bank < 0) {
>                 local_irq_restore(flags);
>                 pr_err("%s: unable to generate VP set\n", __func__);