* [PATCH 1/2] asm-generic/hyperv: provide cpumask_to_vpset_noself [not found] <20210908194243.238523-1-wei.liu@kernel.org> @ 2021-09-08 19:42 ` Wei Liu 2021-09-09 5:38 ` Vitaly Kuznetsov 2021-09-08 19:42 ` [PATCH 2/2] x86/hyperv: remove on-stack cpumask from hv_send_ipi_mask_allbutself Wei Liu 1 sibling, 1 reply; 6+ messages in thread From: Wei Liu @ 2021-09-08 19:42 UTC (permalink / raw) To: Linux on Hyper-V List Cc: Michael Kelley, kys, haiyangz, decui, sthemmin, Wei Liu, Arnd Bergmann, open list:GENERIC INCLUDE/ASM HEADER FILES, open list This is a new variant which removes `self' cpu from the vpset. It will be used in Hyper-V enlightened IPI code. Signed-off-by: Wei Liu <wei.liu@kernel.org> --- Provide a new variant instead of adding a new parameter because it makes it easier to backport -- we don't need to fix the users of cpumask_to_vpset. --- include/asm-generic/mshyperv.h | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/include/asm-generic/mshyperv.h b/include/asm-generic/mshyperv.h index 9a000ba2bb75..d89690ee95aa 100644 --- a/include/asm-generic/mshyperv.h +++ b/include/asm-generic/mshyperv.h @@ -184,10 +184,12 @@ static inline int hv_cpu_number_to_vp_number(int cpu_number) return hv_vp_index[cpu_number]; } -static inline int cpumask_to_vpset(struct hv_vpset *vpset, - const struct cpumask *cpus) +static inline int cpumask_to_vpset_ex(struct hv_vpset *vpset, + const struct cpumask *cpus, + bool exclude_self) { int cpu, vcpu, vcpu_bank, vcpu_offset, nr_bank = 1; + int this_cpu = smp_processor_id(); /* valid_bank_mask can represent up to 64 banks */ if (hv_max_vp_index / 64 >= 64) @@ -205,6 +207,8 @@ static inline int cpumask_to_vpset(struct hv_vpset *vpset, * Some banks may end up being empty but this is acceptable. */ for_each_cpu(cpu, cpus) { + if (exclude_self && cpu == this_cpu) + continue; vcpu = hv_cpu_number_to_vp_number(cpu); if (vcpu == VP_INVAL) return -1; @@ -219,6 +223,18 @@ static inline int cpumask_to_vpset(struct hv_vpset *vpset, return nr_bank; } +static inline int cpumask_to_vpset(struct hv_vpset *vpset, + const struct cpumask *cpus) +{ + return cpumask_to_vpset_ex(vpset, cpus, false); +} + +static inline int cpumask_to_vpset_noself(struct hv_vpset *vpset, + const struct cpumask *cpus) +{ + return cpumask_to_vpset_ex(vpset, cpus, true); +} + void hyperv_report_panic(struct pt_regs *regs, long err, bool in_die); bool hv_is_hyperv_initialized(void); bool hv_is_hibernation_supported(void); -- 2.30.2 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] asm-generic/hyperv: provide cpumask_to_vpset_noself 2021-09-08 19:42 ` [PATCH 1/2] asm-generic/hyperv: provide cpumask_to_vpset_noself Wei Liu @ 2021-09-09 5:38 ` Vitaly Kuznetsov 2021-09-09 10:25 ` Wei Liu 0 siblings, 1 reply; 6+ messages in thread From: Vitaly Kuznetsov @ 2021-09-09 5:38 UTC (permalink / raw) To: Wei Liu, Linux on Hyper-V List Cc: Michael Kelley, kys, haiyangz, decui, sthemmin, Wei Liu, Arnd Bergmann, open list:GENERIC INCLUDE/ASM HEADER FILES, open list Wei Liu <wei.liu@kernel.org> writes: > This is a new variant which removes `self' cpu from the vpset. It will > be used in Hyper-V enlightened IPI code. > > Signed-off-by: Wei Liu <wei.liu@kernel.org> > --- > Provide a new variant instead of adding a new parameter because it makes > it easier to backport -- we don't need to fix the users of > cpumask_to_vpset. > --- > include/asm-generic/mshyperv.h | 20 ++++++++++++++++++-- > 1 file changed, 18 insertions(+), 2 deletions(-) > > diff --git a/include/asm-generic/mshyperv.h b/include/asm-generic/mshyperv.h > index 9a000ba2bb75..d89690ee95aa 100644 > --- a/include/asm-generic/mshyperv.h > +++ b/include/asm-generic/mshyperv.h > @@ -184,10 +184,12 @@ static inline int hv_cpu_number_to_vp_number(int cpu_number) > return hv_vp_index[cpu_number]; > } > > -static inline int cpumask_to_vpset(struct hv_vpset *vpset, > - const struct cpumask *cpus) > +static inline int cpumask_to_vpset_ex(struct hv_vpset *vpset, I'd suggest to avoid '_ex' suffix as we use it for 'extended hypercalls' (e.g. __send_ipi_mask_ex). Assuming nobody needs to call cpumask_to_vpset_ex() directly, should we just go with __cpumask_to_vpset() instead? > + const struct cpumask *cpus, > + bool exclude_self) > { > int cpu, vcpu, vcpu_bank, vcpu_offset, nr_bank = 1; > + int this_cpu = smp_processor_id(); > > /* valid_bank_mask can represent up to 64 banks */ > if (hv_max_vp_index / 64 >= 64) > @@ -205,6 +207,8 @@ static inline int cpumask_to_vpset(struct hv_vpset *vpset, > * Some banks may end up being empty but this is acceptable. > */ > for_each_cpu(cpu, cpus) { > + if (exclude_self && cpu == this_cpu) > + continue; > vcpu = hv_cpu_number_to_vp_number(cpu); > if (vcpu == VP_INVAL) > return -1; > @@ -219,6 +223,18 @@ static inline int cpumask_to_vpset(struct hv_vpset *vpset, > return nr_bank; > } > > +static inline int cpumask_to_vpset(struct hv_vpset *vpset, > + const struct cpumask *cpus) > +{ > + return cpumask_to_vpset_ex(vpset, cpus, false); > +} > + > +static inline int cpumask_to_vpset_noself(struct hv_vpset *vpset, > + const struct cpumask *cpus) > +{ > + return cpumask_to_vpset_ex(vpset, cpus, true); We need to make sure this is called with preemption disabled. We could've just swapped smp_processor_id() for get_cpu() in cpumask_to_vpset_ex() but this is hardly a solution: we can get preempted right after put_cpu() so it's really the caller of this function which needs to be protected. TL;DR: I suggest we add 'WARN_ON_ONCE(preemptible());' or something like this here to catch wrong usage. > +} > + > void hyperv_report_panic(struct pt_regs *regs, long err, bool in_die); > bool hv_is_hyperv_initialized(void); > bool hv_is_hibernation_supported(void); -- Vitaly ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] asm-generic/hyperv: provide cpumask_to_vpset_noself 2021-09-09 5:38 ` Vitaly Kuznetsov @ 2021-09-09 10:25 ` Wei Liu 0 siblings, 0 replies; 6+ messages in thread From: Wei Liu @ 2021-09-09 10:25 UTC (permalink / raw) To: Vitaly Kuznetsov Cc: Wei Liu, Linux on Hyper-V List, Michael Kelley, kys, haiyangz, decui, sthemmin, Arnd Bergmann, open list:GENERIC INCLUDE/ASM HEADER FILES, open list On Thu, Sep 09, 2021 at 07:38:28AM +0200, Vitaly Kuznetsov wrote: > Wei Liu <wei.liu@kernel.org> writes: > > > This is a new variant which removes `self' cpu from the vpset. It will > > be used in Hyper-V enlightened IPI code. > > > > Signed-off-by: Wei Liu <wei.liu@kernel.org> > > --- > > Provide a new variant instead of adding a new parameter because it makes > > it easier to backport -- we don't need to fix the users of > > cpumask_to_vpset. > > --- > > include/asm-generic/mshyperv.h | 20 ++++++++++++++++++-- > > 1 file changed, 18 insertions(+), 2 deletions(-) > > > > diff --git a/include/asm-generic/mshyperv.h b/include/asm-generic/mshyperv.h > > index 9a000ba2bb75..d89690ee95aa 100644 > > --- a/include/asm-generic/mshyperv.h > > +++ b/include/asm-generic/mshyperv.h > > @@ -184,10 +184,12 @@ static inline int hv_cpu_number_to_vp_number(int cpu_number) > > return hv_vp_index[cpu_number]; > > } > > > > -static inline int cpumask_to_vpset(struct hv_vpset *vpset, > > - const struct cpumask *cpus) > > +static inline int cpumask_to_vpset_ex(struct hv_vpset *vpset, > > I'd suggest to avoid '_ex' suffix as we use it for 'extended hypercalls' > (e.g. __send_ipi_mask_ex). Assuming nobody needs to call > cpumask_to_vpset_ex() directly, should we just go with > __cpumask_to_vpset() instead? Sure. I'm not too fussed about the name. I will wait a bit for other people to express their opinions. > > > + const struct cpumask *cpus, > > + bool exclude_self) > > { > > int cpu, vcpu, vcpu_bank, vcpu_offset, nr_bank = 1; > > + int this_cpu = smp_processor_id(); > > > > /* valid_bank_mask can represent up to 64 banks */ > > if (hv_max_vp_index / 64 >= 64) > > @@ -205,6 +207,8 @@ static inline int cpumask_to_vpset(struct hv_vpset *vpset, > > * Some banks may end up being empty but this is acceptable. > > */ > > for_each_cpu(cpu, cpus) { > > + if (exclude_self && cpu == this_cpu) > > + continue; > > vcpu = hv_cpu_number_to_vp_number(cpu); > > if (vcpu == VP_INVAL) > > return -1; > > @@ -219,6 +223,18 @@ static inline int cpumask_to_vpset(struct hv_vpset *vpset, > > return nr_bank; > > } > > > > +static inline int cpumask_to_vpset(struct hv_vpset *vpset, > > + const struct cpumask *cpus) > > +{ > > + return cpumask_to_vpset_ex(vpset, cpus, false); > > +} > > + > > +static inline int cpumask_to_vpset_noself(struct hv_vpset *vpset, > > + const struct cpumask *cpus) > > +{ > > + return cpumask_to_vpset_ex(vpset, cpus, true); > > > We need to make sure this is called with preemption disabled. We > could've just swapped smp_processor_id() for get_cpu() in > cpumask_to_vpset_ex() but this is hardly a solution: we can get > preempted right after put_cpu() so it's really the caller of this > function which needs to be protected. > > TL;DR: I suggest we add 'WARN_ON_ONCE(preemptible());' or something like > this here to catch wrong usage. Good suggestion. I can add this check to the noself variant. Or if people prefer, this check can also be moved into the leaf helper. Wei. > > > +} > > + > > void hyperv_report_panic(struct pt_regs *regs, long err, bool in_die); > > bool hv_is_hyperv_initialized(void); > > bool hv_is_hibernation_supported(void); > > -- > Vitaly > ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/2] x86/hyperv: remove on-stack cpumask from hv_send_ipi_mask_allbutself [not found] <20210908194243.238523-1-wei.liu@kernel.org> 2021-09-08 19:42 ` [PATCH 1/2] asm-generic/hyperv: provide cpumask_to_vpset_noself Wei Liu @ 2021-09-08 19:42 ` Wei Liu 2021-09-10 17:25 ` Michael Kelley 1 sibling, 1 reply; 6+ messages in thread From: Wei Liu @ 2021-09-08 19:42 UTC (permalink / raw) To: Linux on Hyper-V List Cc: Michael Kelley, kys, haiyangz, decui, sthemmin, Wei Liu, Linus Torvalds, Thomas Gleixner, Ingo Molnar, Borislav Petkov, maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT), H. Peter Anvin, open list:X86 ARCHITECTURE (32-BIT AND 64-BIT) It is not a good practice to allocate a cpumask on stack, given it may consume up to 1 kilobytes of stack space if the kernel is configured to have 8192 cpus. The internal helper functions __send_ipi_mask{,_ex} need to loop over the provided mask anyway, so it is not too difficult to skip `self' there. We can thus do away with the on-stack cpumask in hv_send_ipi_mask_allbutself. Adjust call sites of __send_ipi_mask as needed. Reported-by: Linus Torvalds <torvalds@linux-foundation.org> Suggested-by: Michael Kelley <mikelley@microsoft.com> Suggested-by: Linus Torvalds <torvalds@linux-foundation.org> Fixes: 68bb7bfb7985d ("X86/Hyper-V: Enable IPI enlightenments") Signed-off-by: Wei Liu <wei.liu@kernel.org> --- arch/x86/hyperv/hv_apic.c | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/arch/x86/hyperv/hv_apic.c b/arch/x86/hyperv/hv_apic.c index 90e682a92820..911cd41d931c 100644 --- a/arch/x86/hyperv/hv_apic.c +++ b/arch/x86/hyperv/hv_apic.c @@ -99,7 +99,8 @@ static void hv_apic_eoi_write(u32 reg, u32 val) /* * IPI implementation on Hyper-V. */ -static bool __send_ipi_mask_ex(const struct cpumask *mask, int vector) +static bool __send_ipi_mask_ex(const struct cpumask *mask, int vector, + bool exclude_self) { struct hv_send_ipi_ex **arg; struct hv_send_ipi_ex *ipi_arg; @@ -123,7 +124,10 @@ static bool __send_ipi_mask_ex(const struct cpumask *mask, int vector) if (!cpumask_equal(mask, cpu_present_mask)) { ipi_arg->vp_set.format = HV_GENERIC_SET_SPARSE_4K; - nr_bank = cpumask_to_vpset(&(ipi_arg->vp_set), mask); + if (exclude_self) + nr_bank = cpumask_to_vpset_noself(&(ipi_arg->vp_set), mask); + else + nr_bank = cpumask_to_vpset(&(ipi_arg->vp_set), mask); } if (nr_bank < 0) goto ipi_mask_ex_done; @@ -138,9 +142,10 @@ static bool __send_ipi_mask_ex(const struct cpumask *mask, int vector) return hv_result_success(status); } -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); } static bool __send_ipi_one(int cpu, int vector) @@ -208,7 +215,7 @@ static bool __send_ipi_one(int cpu, int vector) return false; if (vp >= 64) - return __send_ipi_mask_ex(cpumask_of(cpu), vector); + return __send_ipi_mask_ex(cpumask_of(cpu), vector, false); status = hv_do_fast_hypercall16(HVCALL_SEND_IPI, vector, BIT_ULL(vp)); return hv_result_success(status); @@ -222,20 +229,13 @@ static void hv_send_ipi(int cpu, int vector) static void hv_send_ipi_mask(const struct cpumask *mask, int vector) { - if (!__send_ipi_mask(mask, vector)) + if (!__send_ipi_mask(mask, vector, false)) orig_apic.send_IPI_mask(mask, vector); } static void hv_send_ipi_mask_allbutself(const struct cpumask *mask, int vector) { - unsigned int this_cpu = smp_processor_id(); - struct cpumask new_mask; - const struct cpumask *local_mask; - - cpumask_copy(&new_mask, mask); - cpumask_clear_cpu(this_cpu, &new_mask); - local_mask = &new_mask; - if (!__send_ipi_mask(local_mask, vector)) + if (!__send_ipi_mask(mask, vector, true)) orig_apic.send_IPI_mask_allbutself(mask, vector); } @@ -246,7 +246,7 @@ static void hv_send_ipi_allbutself(int vector) static void hv_send_ipi_all(int vector) { - if (!__send_ipi_mask(cpu_online_mask, vector)) + if (!__send_ipi_mask(cpu_online_mask, vector, false)) orig_apic.send_IPI_all(vector); } -- 2.30.2 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* RE: [PATCH 2/2] x86/hyperv: remove on-stack cpumask from hv_send_ipi_mask_allbutself 2021-09-08 19:42 ` [PATCH 2/2] x86/hyperv: remove on-stack cpumask from hv_send_ipi_mask_allbutself Wei Liu @ 2021-09-10 17:25 ` Michael Kelley 2021-09-10 18:36 ` Wei Liu 0 siblings, 1 reply; 6+ messages in thread From: Michael Kelley @ 2021-09-10 17:25 UTC (permalink / raw) To: Wei Liu, Linux on Hyper-V List Cc: KY Srinivasan, Haiyang Zhang, Dexuan Cui, Stephen Hemminger, Linus Torvalds, Thomas Gleixner, Ingo Molnar, Borislav Petkov, maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT), H. Peter Anvin, open list:X86 ARCHITECTURE (32-BIT AND 64-BIT) From: Wei Liu <wei.liu@kernel.org> Sent: Wednesday, September 8, 2021 12:43 PM > > It is not a good practice to allocate a cpumask on stack, given it may > consume up to 1 kilobytes of stack space if the kernel is configured to > have 8192 cpus. > > The internal helper functions __send_ipi_mask{,_ex} need to loop over > the provided mask anyway, so it is not too difficult to skip `self' > there. We can thus do away with the on-stack cpumask in > hv_send_ipi_mask_allbutself. > > Adjust call sites of __send_ipi_mask as needed. > > Reported-by: Linus Torvalds <torvalds@linux-foundation.org> > Suggested-by: Michael Kelley <mikelley@microsoft.com> > Suggested-by: Linus Torvalds <torvalds@linux-foundation.org> > Fixes: 68bb7bfb7985d ("X86/Hyper-V: Enable IPI enlightenments") > Signed-off-by: Wei Liu <wei.liu@kernel.org> > --- > arch/x86/hyperv/hv_apic.c | 32 ++++++++++++++++---------------- > 1 file changed, 16 insertions(+), 16 deletions(-) > > diff --git a/arch/x86/hyperv/hv_apic.c b/arch/x86/hyperv/hv_apic.c > index 90e682a92820..911cd41d931c 100644 > --- a/arch/x86/hyperv/hv_apic.c > +++ b/arch/x86/hyperv/hv_apic.c > @@ -99,7 +99,8 @@ static void hv_apic_eoi_write(u32 reg, u32 val) > /* > * IPI implementation on Hyper-V. > */ > -static bool __send_ipi_mask_ex(const struct cpumask *mask, int vector) > +static bool __send_ipi_mask_ex(const struct cpumask *mask, int vector, > + bool exclude_self) > { > struct hv_send_ipi_ex **arg; > struct hv_send_ipi_ex *ipi_arg; > @@ -123,7 +124,10 @@ static bool __send_ipi_mask_ex(const struct cpumask *mask, int vector) > > if (!cpumask_equal(mask, cpu_present_mask)) { > ipi_arg->vp_set.format = HV_GENERIC_SET_SPARSE_4K; > - nr_bank = cpumask_to_vpset(&(ipi_arg->vp_set), mask); > + if (exclude_self) > + nr_bank = cpumask_to_vpset_noself(&(ipi_arg->vp_set), mask); > + else > + nr_bank = cpumask_to_vpset(&(ipi_arg->vp_set), mask); > } > if (nr_bank < 0) > goto ipi_mask_ex_done; > @@ -138,9 +142,10 @@ static bool __send_ipi_mask_ex(const struct cpumask *mask, int vector) > return hv_result_success(status); > } > > -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. The check for VP number >= 64 could also give a false positive since "self" isn't filtered out yet, but that's OK as all that will happen is using the _ex version where the non-ex version would have worked. > > static bool __send_ipi_one(int cpu, int vector) > @@ -208,7 +215,7 @@ static bool __send_ipi_one(int cpu, int vector) > return false; > > if (vp >= 64) > - return __send_ipi_mask_ex(cpumask_of(cpu), vector); > + return __send_ipi_mask_ex(cpumask_of(cpu), vector, false); > > status = hv_do_fast_hypercall16(HVCALL_SEND_IPI, vector, BIT_ULL(vp)); > return hv_result_success(status); > @@ -222,20 +229,13 @@ static void hv_send_ipi(int cpu, int vector) > > static void hv_send_ipi_mask(const struct cpumask *mask, int vector) > { > - if (!__send_ipi_mask(mask, vector)) > + if (!__send_ipi_mask(mask, vector, false)) > orig_apic.send_IPI_mask(mask, vector); > } > > static void hv_send_ipi_mask_allbutself(const struct cpumask *mask, int vector) > { > - unsigned int this_cpu = smp_processor_id(); > - struct cpumask new_mask; > - const struct cpumask *local_mask; > - > - cpumask_copy(&new_mask, mask); > - cpumask_clear_cpu(this_cpu, &new_mask); > - local_mask = &new_mask; > - if (!__send_ipi_mask(local_mask, vector)) > + if (!__send_ipi_mask(mask, vector, true)) > orig_apic.send_IPI_mask_allbutself(mask, vector); > } > > @@ -246,7 +246,7 @@ static void hv_send_ipi_allbutself(int vector) > > static void hv_send_ipi_all(int vector) > { > - if (!__send_ipi_mask(cpu_online_mask, vector)) > + if (!__send_ipi_mask(cpu_online_mask, vector, false)) > orig_apic.send_IPI_all(vector); > } > > -- > 2.30.2 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] x86/hyperv: remove on-stack cpumask from hv_send_ipi_mask_allbutself 2021-09-10 17:25 ` Michael Kelley @ 2021-09-10 18:36 ` Wei Liu 0 siblings, 0 replies; 6+ messages in thread From: Wei Liu @ 2021-09-10 18:36 UTC (permalink / raw) To: Michael Kelley Cc: Wei Liu, Linux on Hyper-V List, KY Srinivasan, Haiyang Zhang, Dexuan Cui, Stephen Hemminger, Linus Torvalds, Thomas Gleixner, Ingo Molnar, Borislav Petkov, maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT), H. Peter Anvin, open list:X86 ARCHITECTURE (32-BIT AND 64-BIT) On Fri, Sep 10, 2021 at 05:25:15PM +0000, Michael Kelley wrote: > From: Wei Liu <wei.liu@kernel.org> 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-09-10 18:36 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20210908194243.238523-1-wei.liu@kernel.org>
2021-09-08 19:42 ` [PATCH 1/2] asm-generic/hyperv: provide cpumask_to_vpset_noself Wei Liu
2021-09-09 5:38 ` Vitaly Kuznetsov
2021-09-09 10:25 ` Wei Liu
2021-09-08 19:42 ` [PATCH 2/2] x86/hyperv: remove on-stack cpumask from hv_send_ipi_mask_allbutself Wei Liu
2021-09-10 17:25 ` Michael Kelley
2021-09-10 18:36 ` Wei Liu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).