Re: [PATCH 2/2] x86/hyperv: remove on-stack cpumask from hv_send_ipi_mask_allbutself

[Wei Liu]

On Fri, Sep 10, 2021 at 05:25:15PM +0000, Michael Kelley wrote:
> From: Wei Liu <wei.liu@xxxxxxxxxx> Sent: Wednesday, September 8, 2021 12:43 PM
> > 
[...]
> > -static bool __send_ipi_mask(const struct cpumask *mask, int vector)
> > +static bool __send_ipi_mask(const struct cpumask *mask, int vector,
> > +		bool exclude_self)
> >  {
> > -	int cur_cpu, vcpu;
> > +	int cur_cpu, vcpu, this_cpu = smp_processor_id();
> >  	struct hv_send_ipi ipi_arg;
> >  	u64 status;
> > 
> > @@ -172,6 +177,8 @@ static bool __send_ipi_mask(const struct cpumask *mask, int vector)
> >  	ipi_arg.cpu_mask = 0;
> > 
> >  	for_each_cpu(cur_cpu, mask) {
> > +		if (exclude_self && cur_cpu == this_cpu)
> > +			continue;
> >  		vcpu = hv_cpu_number_to_vp_number(cur_cpu);
> >  		if (vcpu == VP_INVAL)
> >  			return false;
> > @@ -191,7 +198,7 @@ static bool __send_ipi_mask(const struct cpumask *mask, int vector)
> >  	return hv_result_success(status);
> > 
> >  do_ex_hypercall:
> > -	return __send_ipi_mask_ex(mask, vector);
> > +	return __send_ipi_mask_ex(mask, vector, exclude_self);
> >  }
> 
> This all looks correct to me, except for one difference compared with the
> current code.  In the current code, if the cpumask passed to
> hv_send_ipi_mask_allbutself() indicates only a single CPU that is "self",
> __send_ipi_mask() will detect that the cpumask is now empty, and 
> correctly return success without making the hypercall.  But
> the new code will make the hypercall with an empty input mask (both
> in the SEND_IPI and SEND_IPI_EX cases).   The Hyper-V TLFS is silent
> on whether such a hypercall is a no-op that returns success or is an
> error.  We'll have a problem if it is an error.  I think the safest thing
> is to enhance the cpumask_empty() test at the beginning of
> __send_ipi_mask() to also detect the case where only a single CPU
> is specified, and it is "self".   This could be done using cpumask_weight()
> and checking for zero as the "empty" case.   Then check for "1", and if
> exclude_self is set, check if it is the "self" CPU.

Sure. Making this change should not be too difficult.

Wei.