LinuxLists.cc - [PATCH 0/5] X86: Hyper-V: APIC enlightenments

[permalink] [raw]

Subject: Re: [PATCH 1/5] X86: Hyper-V: Enlighten APIC access

On Wed, 25 Apr 2018, [email protected] wrote:
> --- /dev/null
> +++ b/arch/x86/hyperv/hv_apic.c
> @@ -0,0 +1,98 @@
> +// SPDX-License-Identifier: GPL-2.0

Thanks for putting the license identifier in.

> +
> +/*
> + * Hyper-V specific APIC code.
> + *
> + * Copyright (C) 2018, Microsoft, Inc.
> + *
> + * Author : K. Y. Srinivasan <[email protected]>

But can you please check with your lawyers whether you can avoid the
pointless boilerplate? The SPDX identifier should cover it.

> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published
> + * by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE, GOOD TITLE or
> + * NON INFRINGEMENT. See the GNU General Public License for more
> + * details.
> + *
> + */
> +
> +#include <linux/types.h>
> +#include <asm/hypervisor.h>
> +#include <asm/mshyperv.h>
> +#include <linux/version.h>
> +#include <linux/vmalloc.h>
> +#include <linux/mm.h>
> +#include <linux/clockchips.h>
> +#include <linux/hyperv.h>
> +#include <linux/slab.h>
> +#include <linux/cpuhotplug.h>

We usually order the includes

#include <linux/....>
...
#include <linux/....>

#include <asm/....>
#include <asm/....>

> -void hyperv_init(void);
> +void __init hyperv_init(void);
> void hyperv_setup_mmu_ops(void);
> void hyper_alloc_mmu(void);
> void hyperv_report_panic(struct pt_regs *regs, long err);
> @@ -269,14 +269,16 @@ void hyperv_reenlightenment_intr(struct pt_regs *regs);
> void set_hv_tscchange_cb(void (*cb)(void));
> void clear_hv_tscchange_cb(void);
> void hyperv_stop_tsc_emulation(void);
> +void hv_apic_init(void);
> #else /* CONFIG_HYPERV */
> -static inline void hyperv_init(void) {}
> +static __init inline void hyperv_init(void) {}

The __init on the empty inline function is pointless.

Other than the few nits. This looks well done!

Thanks,

tglx

2018-04-26 22:10:20

[permalink] [raw]

Subject: Re: [PATCH 2/5] X86: Hyper-V: Enable IPI enlightenments

On Wed, 25 Apr 2018, [email protected] wrote:
> +static int __send_ipi_mask(const struct cpumask *mask, int vector)
> +{
> + int cur_cpu, vcpu;
> + struct ipi_arg_non_ex **arg;
> + struct ipi_arg_non_ex *ipi_arg;
> + int ret = 1;

So this indicates whether __send_ipi_mask() can send to @mask or not. So
please make it a bool and let it return false when it does not work, true
otherwise. If you had used -Exxxx then it would have been more obvious, but
this is really a boolean decision.

> + unsigned long flags;
> +
> + if (cpumask_empty(mask))
> + return 0;
> +
> + if (!hv_hypercall_pg)
> + return ret;
> +
> + if ((vector < HV_IPI_LOW_VECTOR) || (vector > HV_IPI_HIGH_VECTOR))
> + return ret;
> +
> + local_irq_save(flags);
> + arg = (struct ipi_arg_non_ex **)this_cpu_ptr(hyperv_pcpu_input_arg);
> +
> + ipi_arg = *arg;
> + if (unlikely(!ipi_arg))
> + goto ipi_mask_done;
> +
> +

Stray newline

> + ipi_arg->vector = vector;
> + ipi_arg->reserved = 0;
> + ipi_arg->cpu_mask = 0;
> +
> + for_each_cpu(cur_cpu, mask) {
> + vcpu = hv_cpu_number_to_vp_number(cur_cpu);
> + if (vcpu >= 64)
> + goto ipi_mask_done;

This is completely magic and deserves a comment.

> +
> + __set_bit(vcpu, (unsigned long *)&ipi_arg->cpu_mask);
> + }
> +
> + ret = hv_do_hypercall(HVCALL_SEND_IPI, ipi_arg, NULL);
> +
> +ipi_mask_done:
> + local_irq_restore(flags);
> + return ret;
> +}

....

> static int hv_cpu_init(unsigned int cpu)
> {
> u64 msr_vp_index;
> struct hv_vp_assist_page **hvp = &hv_vp_assist_page[smp_processor_id()];
> + void **input_arg;
> +
> + input_arg = (void **)this_cpu_ptr(hyperv_pcpu_input_arg);
> + *input_arg = page_address(alloc_page(GFP_ATOMIC));

This is called from the cpu hotplug thread and there is no need for an
atomic allocation. Please use GFP_KERNEL.

> hv_get_vp_index(msr_vp_index);
>
> @@ -217,6 +224,10 @@ static int hv_cpu_die(unsigned int cpu)
> {
> struct hv_reenlightenment_control re_ctrl;
> unsigned int new_cpu;
> + void **input_arg;
> +
> + input_arg = (void **)this_cpu_ptr(hyperv_pcpu_input_arg);
> + free_page((unsigned long)*input_arg);

Hrm. Again this is called from the CPU hotplug thread when the cou is about
to go down. But you can be scheduled out after free() and before disabling
the assist thing below and the pointer persist. There is no guarantee that
nothing sends an IPI anymore after this point.

So you have two options here:

1) Disable interrupts, get the pointer, set the per cpu pointer to NULL,
reenable interruots and free the page

2) Keep the page around and check for it in the CPU UP path and avoid the
allocation when the CPU comes online again.

> if (hv_vp_assist_page && hv_vp_assist_page[cpu])
> wrmsrl(HV_X64_MSR_VP_ASSIST_PAGE, 0);
> @@ -260,6 +271,12 @@ void __init hyperv_init(void)
> if ((ms_hyperv.features & required_msrs) != required_msrs)
> return;
>
> + /* Allocate the per-CPU state for the hypercall input arg */
> + hyperv_pcpu_input_arg = alloc_percpu(void *);
> +
> + if (hyperv_pcpu_input_arg == NULL)
> + return;

Huch. When that allocation fails, you return and ignore the rest of the
function which has been there before. Weird decision.

Thanks,

tglx

2018-04-26 22:17:48

by Michael Kelley (EOSG)

[permalink] [raw]

Subject: RE: [PATCH 1/5] X86: Hyper-V: Enlighten APIC access

> -----Original Message-----
> From: [email protected] <[email protected]>
> Sent: Wednesday, April 25, 2018 11:13 AM
> To: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; Stephen Hemminger <[email protected]>;
> Michael Kelley (EOSG) <[email protected]>; [email protected]
> Cc: KY Srinivasan <[email protected]>
> Subject: [PATCH 1/5] X86: Hyper-V: Enlighten APIC access
>
> From: "K. Y. Srinivasan" <[email protected]>
>
> Hyper-V supports MSR based APIC access; implement
> the enlightenment.
>
> Signed-off-by: K. Y. Srinivasan <[email protected]>
> ---
> arch/x86/hyperv/Makefile | 2 +-
> arch/x86/hyperv/hv_apic.c | 98 +++++++++++++++++++++++++++++++++++++++++
> arch/x86/hyperv/hv_init.c | 5 ++-
> arch/x86/include/asm/mshyperv.h | 6 ++-
> 4 files changed, 107 insertions(+), 4 deletions(-)
> create mode 100644 arch/x86/hyperv/hv_apic.c
>
> diff --git a/arch/x86/hyperv/Makefile b/arch/x86/hyperv/Makefile
> index 367a8203cfcf..00ce4df01a09 100644
> --- a/arch/x86/hyperv/Makefile
> +++ b/arch/x86/hyperv/Makefile
> @@ -1 +1 @@
> -obj-y := hv_init.o mmu.o
> +obj-y := hv_init.o mmu.o hv_apic.o
> diff --git a/arch/x86/hyperv/hv_apic.c b/arch/x86/hyperv/hv_apic.c
> new file mode 100644
> index 000000000000..e0a5b36208fc
> --- /dev/null
> +++ b/arch/x86/hyperv/hv_apic.c
> @@ -0,0 +1,98 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +/*
> + * Hyper-V specific APIC code.
> + *
> + * Copyright (C) 2018, Microsoft, Inc.
> + *
> + * Author : K. Y. Srinivasan <[email protected]>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published
> + * by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE, GOOD TITLE or
> + * NON INFRINGEMENT. See the GNU General Public License for more
> + * details.
> + *
> + */
> +
> +#include <linux/types.h>
> +#include <asm/hypervisor.h>
> +#include <asm/mshyperv.h>
> +#include <linux/version.h>
> +#include <linux/vmalloc.h>
> +#include <linux/mm.h>
> +#include <linux/clockchips.h>
> +#include <linux/hyperv.h>
> +#include <linux/slab.h>
> +#include <linux/cpuhotplug.h>
> +
> +static u64 hv_apic_icr_read(void)
> +{
> + u64 reg_val;
> +
> + rdmsrl(HV_X64_MSR_ICR, reg_val);
> + return reg_val;
> +}
> +
> +static void hv_apic_icr_write(u32 low, u32 id)
> +{
> + u64 reg_val;
> +
> + reg_val = SET_APIC_DEST_FIELD(id);
> + reg_val = reg_val << 32;
> + reg_val |= low;
> +
> + wrmsrl(HV_X64_MSR_ICR, reg_val);
> +}
> +
> +static u32 hv_apic_read(u32 reg)
> +{
> + u32 reg_val, hi;
> +
> + switch (reg) {
> + case APIC_EOI:
> + rdmsr(HV_X64_MSR_EOI, reg_val, hi);
> + return reg_val;
> + case APIC_TASKPRI:
> + rdmsr(HV_X64_MSR_TPR, reg_val, hi);
> + return reg_val;
> +
> + default:
> + return native_apic_mem_read(reg);
> + }
> +}
> +
> +static void hv_apic_write(u32 reg, u32 val)
> +{
> + switch (reg) {
> + case APIC_EOI:
> + wrmsr(HV_X64_MSR_EOI, val, 0);
> + break;
> + case APIC_TASKPRI:
> + wrmsr(HV_X64_MSR_TPR, val, 0);
> + break;
> + default:
> + native_apic_mem_write(reg, val);
> + }
> +}
> +
> +static void hv_apic_eoi_write(u32 reg, u32 val)
> +{
> + wrmsr(HV_X64_MSR_EOI, val, 0);
> +}
> +
> +void __init hv_apic_init(void)
> +{
> + if (ms_hyperv.hints & HV_X64_APIC_ACCESS_RECOMMENDED) {
> + pr_info("Hyper-V: Using MSR ased APIC access\n");

Typo here. "ased" should be "based".

> + apic_set_eoi_write(hv_apic_eoi_write);
> + apic->read = hv_apic_read;
> + apic->write = hv_apic_write;
> + apic->icr_write = hv_apic_icr_write;
> + apic->icr_read = hv_apic_icr_read;
> + }
> +}
> diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
> index cfecc2272f2d..71e50fc2b7ef 100644
> --- a/arch/x86/hyperv/hv_init.c
> +++ b/arch/x86/hyperv/hv_init.c
> @@ -242,8 +242,9 @@ static int hv_cpu_die(unsigned int cpu)
> *
> * 1. Setup the hypercall page.
> * 2. Register Hyper-V specific clocksource.
> + * 3. Setup Hyper-V specific APIC entry points.
> */
> -void hyperv_init(void)
> +void __init hyperv_init(void)
> {
> u64 guest_id, required_msrs;
> union hv_x64_msr_hypercall_contents hypercall_msr;
> @@ -298,6 +299,8 @@ void hyperv_init(void)
>
> hyper_alloc_mmu();
>
> + hv_apic_init();
> +
> /*
> * Register Hyper-V specific clocksource.
> */
> diff --git a/arch/x86/include/asm/mshyperv.h b/arch/x86/include/asm/mshyperv.h
> index b90e79610cf7..bcced50037c1 100644
> --- a/arch/x86/include/asm/mshyperv.h
> +++ b/arch/x86/include/asm/mshyperv.h
> @@ -258,7 +258,7 @@ static inline int hv_cpu_number_to_vp_number(int cpu_number)
> return hv_vp_index[cpu_number];
> }
>
> -void hyperv_init(void);
> +void __init hyperv_init(void);
> void hyperv_setup_mmu_ops(void);
> void hyper_alloc_mmu(void);
> void hyperv_report_panic(struct pt_regs *regs, long err);
> @@ -269,14 +269,16 @@ void hyperv_reenlightenment_intr(struct pt_regs *regs);
> void set_hv_tscchange_cb(void (*cb)(void));
> void clear_hv_tscchange_cb(void);
> void hyperv_stop_tsc_emulation(void);
> +void hv_apic_init(void);
> #else /* CONFIG_HYPERV */
> -static inline void hyperv_init(void) {}
> +static __init inline void hyperv_init(void) {}
> static inline bool hv_is_hyperv_initialized(void) { return false; }
> static inline void hyperv_cleanup(void) {}
> static inline void hyperv_setup_mmu_ops(void) {}
> static inline void set_hv_tscchange_cb(void (*cb)(void)) {}
> static inline void clear_hv_tscchange_cb(void) {}
> static inline void hyperv_stop_tsc_emulation(void) {};
> +static inline void hv_apic_init(void) {}
> static inline struct hv_vp_assist_page *hv_get_vp_assist_page(unsigned int cpu)
> {
> return NULL;
> --
> 2.15.1

2018-04-26 22:18:23

[permalink] [raw]

Subject: Re: [PATCH 3/5] X86: Hyper-V: Enhanced IPI enlightenment

On Wed, 25 Apr 2018, [email protected] wrote:
>
> +struct ipi_arg_ex {
> + u32 vector;
> + u32 reserved;
> + struct hv_vpset vp_set;

Please align that in tabular fashion for easy of reading

u32 vector;
u32 reserved;
struct hv_vpset vp_set;

> +};
> +
> static struct apic orig_apic;
>
> static u64 hv_apic_icr_read(void)
> @@ -97,6 +103,40 @@ static void hv_apic_eoi_write(u32 reg, u32 val)
> * IPI implementation on Hyper-V.
> */
>
> +static int __send_ipi_mask_ex(const struct cpumask *mask, int vector)
> +{
> + int nr_bank = 0;
> + struct ipi_arg_ex **arg;
> + struct ipi_arg_ex *ipi_arg;
> + int ret = 1;
> + unsigned long flags;

This is really horrible to read.

struct ipi_arg_ex *ipi_arg;
struct ipi_arg_ex **arg;
unsigned long flags;
bool ret = false;
int nr_bank = 0;

is really more conveniant for quick reading.

So the other more limited function has a lot more sanity checks vs. vector
number and other things. Why are they not required here? Comment please.

> + local_irq_save(flags);
> + arg = (struct ipi_arg_ex **)this_cpu_ptr(hyperv_pcpu_input_arg);
> +
> + ipi_arg = *arg;
> + if (unlikely(!ipi_arg))
> + goto ipi_mask_ex_done;
> +
> + ipi_arg->vector = vector;
> + ipi_arg->reserved = 0;
> + ipi_arg->vp_set.valid_bank_mask = 0;
> +
> + if (!cpumask_equal(mask, cpu_present_mask)) {
> + ipi_arg->vp_set.format = HV_GENERIC_SET_SPARCE_4K;
> + nr_bank = cpumask_to_vpset(&(ipi_arg->vp_set), mask);

nr_bank really confused me. bank_nr is what you mean, not number of banks,
right?

> + }
> + if (!nr_bank)
> + ipi_arg->vp_set.format = HV_GENERIC_SET_ALL;
> +
> + ret = hv_do_rep_hypercall(HVCALL_SEND_IPI_EX, 0, nr_bank,
> + ipi_arg, NULL);
> +
> +ipi_mask_ex_done:
> + local_irq_restore(flags);
> + return ret;
> +}

Thanks,

tglx

2018-04-26 22:23:11

[permalink] [raw]

Subject: Re: [PATCH 4/5] X86: Hyper-V: Consolidate code for converting cpumask to vpset

On Wed, 25 Apr 2018, [email protected] wrote:
>
> if (!cpumask_equal(mask, cpu_present_mask)) {
> - ipi_arg->vp_set.format = HV_GENERIC_SET_SPARCE_4K;
> + ipi_arg->vp_set.format = HV_GENERIC_SET_SPARSE_4K;

Please move this patch before the others, so you can use SPARSE in the new
code right away.

Thanks,

tglx

2018-04-26 22:25:01

[permalink] [raw]

Subject: Re: [PATCH 5/5] X86: Hyper-V: Consolidate the allocation of the hypercall input page

On Wed, 25 Apr 2018, [email protected] wrote:

> From: "K. Y. Srinivasan" <[email protected]>
>
> Consolidate the allocation of the hypercall input page.

Again. You can provide the new way of allocation first, then you don't have
to add code first in order to remove it later again.

Thanks,

tglx

2018-04-26 22:55:57

by Michael Kelley (EOSG)

[permalink] [raw]

Subject: RE: [PATCH 2/5] X86: Hyper-V: Enable IPI enlightenments

> -----Original Message-----
> From: [email protected] <[email protected]>
> Sent: Wednesday, April 25, 2018 11:13 AM
> To: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; Stephen Hemminger <[email protected]>;
> Michael Kelley (EOSG) <[email protected]>; [email protected]
> Cc: KY Srinivasan <[email protected]>
> Subject: [PATCH 2/5] X86: Hyper-V: Enable IPI enlightenments
>
> From: "K. Y. Srinivasan" <[email protected]>
>
> Hyper-V supports hypercalls to implement IPI; use them.
>
> Signed-off-by: K. Y. Srinivasan <[email protected]>
> ---
> arch/x86/hyperv/hv_apic.c | 125 +++++++++++++++++++++++++++++++++++++
> arch/x86/hyperv/hv_init.c | 17 +++++
> arch/x86/include/asm/hyperv-tlfs.h | 9 +++
> arch/x86/include/asm/mshyperv.h | 1 +
> 4 files changed, 152 insertions(+)
>
> diff --git a/arch/x86/hyperv/hv_apic.c b/arch/x86/hyperv/hv_apic.c
> index e0a5b36208fc..7f3322ecfb01 100644
> --- a/arch/x86/hyperv/hv_apic.c
> +++ b/arch/x86/hyperv/hv_apic.c
> @@ -30,6 +30,14 @@
> #include <linux/slab.h>
> #include <linux/cpuhotplug.h>
>
> +struct ipi_arg_non_ex {
> + u32 vector;
> + u32 reserved;
> + u64 cpu_mask;
> +};

I think we'd like to put structures like this, which are defined in the
Hyper-V Top Level Functional Spec, in hyperv-tlfs.h. Also, the 5.0b
version of the TLFS, which is latest, shows this structure on page 100:

u32 vector;
u8 targetvtl;
u8 reserved[3];
u64 cpu_mask;

> +
> +static struct apic orig_apic;
> +
> static u64 hv_apic_icr_read(void)
> {
> u64 reg_val;
> @@ -85,8 +93,125 @@ static void hv_apic_eoi_write(u32 reg, u32 val)
> wrmsr(HV_X64_MSR_EOI, val, 0);
> }
>
> +/*
> + * IPI implementation on Hyper-V.
> + */
> +
> +static int __send_ipi_mask(const struct cpumask *mask, int vector)
> +{
> + int cur_cpu, vcpu;
> + struct ipi_arg_non_ex **arg;
> + struct ipi_arg_non_ex *ipi_arg;
> + int ret = 1;
> + unsigned long flags;
> +
> + if (cpumask_empty(mask))
> + return 0;
> +
> + if (!hv_hypercall_pg)
> + return ret;
> +
> + if ((vector < HV_IPI_LOW_VECTOR) || (vector > HV_IPI_HIGH_VECTOR))
> + return ret;
> +
> + local_irq_save(flags);
> + arg = (struct ipi_arg_non_ex **)this_cpu_ptr(hyperv_pcpu_input_arg);
> +
> + ipi_arg = *arg;
> + if (unlikely(!ipi_arg))
> + goto ipi_mask_done;
> +
> +
> + ipi_arg->vector = vector;
> + ipi_arg->reserved = 0;
> + ipi_arg->cpu_mask = 0;
> +
> + for_each_cpu(cur_cpu, mask) {
> + vcpu = hv_cpu_number_to_vp_number(cur_cpu);
> + if (vcpu >= 64)
> + goto ipi_mask_done;
> +
> + __set_bit(vcpu, (unsigned long *)&ipi_arg->cpu_mask);
> + }
> +
> + ret = hv_do_hypercall(HVCALL_SEND_IPI, ipi_arg, NULL);
> +
> +ipi_mask_done:
> + local_irq_restore(flags);
> + return ret;
> +}
> +
> +static int __send_ipi_one(int cpu, int vector)
> +{
> + struct cpumask mask = CPU_MASK_NONE;
> +
> + cpumask_set_cpu(cpu, &mask);
> + return __send_ipi_mask(&mask, vector);
> +}
> +
> +static void hv_send_ipi(int cpu, int vector)
> +{
> + if (__send_ipi_one(cpu, vector))
> + orig_apic.send_IPI(cpu, vector);
> +}
> +
> +static void hv_send_ipi_mask(const struct cpumask *mask, int vector)
> +{
> + if (__send_ipi_mask(mask, vector))
> + 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))
> + orig_apic.send_IPI_mask_allbutself(mask, vector);
> +}
> +
> +static void hv_send_ipi_allbutself(int vector)
> +{
> + hv_send_ipi_mask_allbutself(cpu_online_mask, vector);
> +}
> +
> +static void hv_send_ipi_all(int vector)
> +{
> + if (__send_ipi_mask(cpu_online_mask, vector))
> + orig_apic.send_IPI_all(vector);
> +}
> +
> +static void hv_send_ipi_self(int vector)
> +{
> + if (__send_ipi_one(smp_processor_id(), vector))
> + orig_apic.send_IPI_self(vector);
> +}
> +
> void __init hv_apic_init(void)
> {
> + if (ms_hyperv.hints & HV_X64_CLUSTER_IPI_RECOMMENDED) {
> + if (hyperv_pcpu_input_arg == NULL)
> + goto msr_based_access;
> +
> + pr_info("Hyper-V: Using IPI hypercalls\n");
> + /*
> + * Set the IPI entry points.
> + */
> + orig_apic = *apic;
> +
> + apic->send_IPI = hv_send_ipi;
> + apic->send_IPI_mask = hv_send_ipi_mask;
> + apic->send_IPI_mask_allbutself = hv_send_ipi_mask_allbutself;
> + apic->send_IPI_allbutself = hv_send_ipi_allbutself;
> + apic->send_IPI_all = hv_send_ipi_all;
> + apic->send_IPI_self = hv_send_ipi_self;
> + }
> +
> +msr_based_access:
> if (ms_hyperv.hints & HV_X64_APIC_ACCESS_RECOMMENDED) {
> pr_info("Hyper-V: Using MSR ased APIC access\n");
> apic_set_eoi_write(hv_apic_eoi_write);
> diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
> index 71e50fc2b7ef..a895662b6b4c 100644
> --- a/arch/x86/hyperv/hv_init.c
> +++ b/arch/x86/hyperv/hv_init.c
> @@ -91,12 +91,19 @@ EXPORT_SYMBOL_GPL(hv_vp_index);
> struct hv_vp_assist_page **hv_vp_assist_page;
> EXPORT_SYMBOL_GPL(hv_vp_assist_page);
>
> +void __percpu **hyperv_pcpu_input_arg;
> +EXPORT_SYMBOL_GPL(hyperv_pcpu_input_arg);
> +
> u32 hv_max_vp_index;
>
> static int hv_cpu_init(unsigned int cpu)
> {
> u64 msr_vp_index;
> struct hv_vp_assist_page **hvp = &hv_vp_assist_page[smp_processor_id()];
> + void **input_arg;
> +
> + input_arg = (void **)this_cpu_ptr(hyperv_pcpu_input_arg);
> + *input_arg = page_address(alloc_page(GFP_ATOMIC));
>
> hv_get_vp_index(msr_vp_index);
>
> @@ -217,6 +224,10 @@ static int hv_cpu_die(unsigned int cpu)
> {
> struct hv_reenlightenment_control re_ctrl;
> unsigned int new_cpu;
> + void **input_arg;
> +
> + input_arg = (void **)this_cpu_ptr(hyperv_pcpu_input_arg);
> + free_page((unsigned long)*input_arg);
>
> if (hv_vp_assist_page && hv_vp_assist_page[cpu])
> wrmsrl(HV_X64_MSR_VP_ASSIST_PAGE, 0);
> @@ -260,6 +271,12 @@ void __init hyperv_init(void)
> if ((ms_hyperv.features & required_msrs) != required_msrs)
> return;
>
> + /* Allocate the per-CPU state for the hypercall input arg */
> + hyperv_pcpu_input_arg = alloc_percpu(void *);
> +
> + if (hyperv_pcpu_input_arg == NULL)
> + return;
> +
> /* Allocate percpu VP index */
> hv_vp_index = kmalloc_array(num_possible_cpus(), sizeof(*hv_vp_index),
> GFP_KERNEL);
> diff --git a/arch/x86/include/asm/hyperv-tlfs.h b/arch/x86/include/asm/hyperv-tlfs.h
> index 416cb0e0c496..646cf2ca2aaa 100644
> --- a/arch/x86/include/asm/hyperv-tlfs.h
> +++ b/arch/x86/include/asm/hyperv-tlfs.h
> @@ -164,6 +164,11 @@
> */
> #define HV_X64_DEPRECATING_AEOI_RECOMMENDED (1 << 9)
>
> +/*
> + * Recommend using cluster IPI hypercalls.
> + */
> +#define HV_X64_CLUSTER_IPI_RECOMMENDED (1 << 10)
> +
> /* Recommend using the newer ExProcessorMasks interface */
> #define HV_X64_EX_PROCESSOR_MASKS_RECOMMENDED (1 << 11)
>
> @@ -329,10 +334,14 @@ struct hv_tsc_emulation_status {
> #define HV_X64_MSR_HYPERCALL_PAGE_ADDRESS_MASK \
> (~((1ull << HV_X64_MSR_HYPERCALL_PAGE_ADDRESS_SHIFT) - 1))
>
> +#define HV_IPI_LOW_VECTOR 0x10
> +#define HV_IPI_HIGH_VECTOR 0xff
> +
> /* Declare the various hypercall operations. */
> #define HVCALL_FLUSH_VIRTUAL_ADDRESS_SPACE 0x0002
> #define HVCALL_FLUSH_VIRTUAL_ADDRESS_LIST 0x0003
> #define HVCALL_NOTIFY_LONG_SPIN_WAIT 0x0008
> +#define HVCALL_SEND_IPI 0x000b
> #define HVCALL_FLUSH_VIRTUAL_ADDRESS_SPACE_EX 0x0013
> #define HVCALL_FLUSH_VIRTUAL_ADDRESS_LIST_EX 0x0014
> #define HVCALL_POST_MESSAGE 0x005c
> diff --git a/arch/x86/include/asm/mshyperv.h b/arch/x86/include/asm/mshyperv.h
> index bcced50037c1..f6045f3611de 100644
> --- a/arch/x86/include/asm/mshyperv.h
> +++ b/arch/x86/include/asm/mshyperv.h
> @@ -122,6 +122,7 @@ static inline void hv_disable_stimer0_percpu_irq(int irq) {}
> #if IS_ENABLED(CONFIG_HYPERV)
> extern struct clocksource *hyperv_cs;
> extern void *hv_hypercall_pg;
> +extern void __percpu **hyperv_pcpu_input_arg;
>
> static inline u64 hv_do_hypercall(u64 control, void *input, void *output)
> {
> --
> 2.15.1

2018-04-26 22:56:26

[permalink] [raw]

Subject: RE: [PATCH 1/5] X86: Hyper-V: Enlighten APIC access

Michael,

On Thu, 26 Apr 2018, Michael Kelley (EOSG) wrote:

> > -----Original Message-----
> > From: [email protected] <[email protected]>
> > Sent: Wednesday, April 25, 2018 11:13 AM
> > To: [email protected]; [email protected]; [email protected];
> > [email protected]; [email protected]; [email protected]; [email protected];
> > [email protected]; [email protected]; Stephen Hemminger <[email protected]>;
> > Michael Kelley (EOSG) <[email protected]>; [email protected]
> > Cc: KY Srinivasan <[email protected]>
> > Subject: [PATCH 1/5] X86: Hyper-V: Enlighten APIC access

Please fix your MUA not to pointlessly copy the whole mail header.

> > +void __init hv_apic_init(void)
> > +{
> > + if (ms_hyperv.hints & HV_X64_APIC_ACCESS_RECOMMENDED) {
> > + pr_info("Hyper-V: Using MSR ased APIC access\n");
>
> Typo here. "ased" should be "based".

And please trim the reply to the relevant point. It's annoying to find that
single line of review comment in the useless pile of quoted patch.

Thanks,

tglx

2018-04-26 23:26:57

by Michael Kelley (EOSG)

[permalink] [raw]

Subject: RE: [PATCH 3/5] X86: Hyper-V: Enhanced IPI enlightenment

On Wed, 25 Apr 2018, KY Srinivasan <[email protected]> wrote:
>
> +struct ipi_arg_ex {
> + u32 vector;
> + u32 reserved;
> + struct hv_vpset vp_set;
> +};

Again, suggest moving to hyperv-tlfs.h. And the 5.0b version
of the TLFS has:

u32 vector;
u8 targetvtl;
u8 reserved[3];
struct hv_vpset vp_set;

....

>
> +struct hv_vpset {
> + u64 format;
> + u64 valid_bank_mask;
> + u64 bank_contents[];
> +};

And this as well.

Michael

2018-04-27 05:46:16

[permalink] [raw]

Subject: Re: [PATCH 1/5] X86: Hyper-V: Enlighten APIC access

Attachments:

(No filename) (3.00 kB)
.config.gz (24.61 kB)
Download all attachments

2018-04-27 05:53:55

[permalink] [raw]

Subject: RE: [PATCH 1/5] X86: Hyper-V: Enlighten APIC access

> -----Original Message-----
> From: Thomas Gleixner <[email protected]>
> Sent: Thursday, April 26, 2018 2:49 PM
> To: KY Srinivasan <[email protected]>
> Cc: [email protected]; [email protected]; linux-
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected]; Stephen
> Hemminger <[email protected]>; Michael Kelley (EOSG)
> <[email protected]>; [email protected]
> Subject: Re: [PATCH 1/5] X86: Hyper-V: Enlighten APIC access
>
> On Wed, 25 Apr 2018, [email protected] wrote:
> > --- /dev/null
> > +++ b/arch/x86/hyperv/hv_apic.c
> > @@ -0,0 +1,98 @@
> > +// SPDX-License-Identifier: GPL-2.0
>
> Thanks for putting the license identifier in.
>
> > +
> > +/*
> > + * Hyper-V specific APIC code.
> > + *
> > + * Copyright (C) 2018, Microsoft, Inc.
> > + *
> > + * Author : K. Y. Srinivasan <[email protected]>
>
> But can you please check with your lawyers whether you can avoid the
> pointless boilerplate? The SPDX identifier should cover it.

I will consult with MSFT legal on this.
>
> > + * This program is free software; you can redistribute it and/or modify it
> > + * under the terms of the GNU General Public License version 2 as
> published
> > + * by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope that it will be useful, but
> > + * WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE, GOOD
> TITLE or
> > + * NON INFRINGEMENT. See the GNU General Public License for more
> > + * details.
> > + *
> > + */
> > +
> > +#include <linux/types.h>
> > +#include <asm/hypervisor.h>
> > +#include <asm/mshyperv.h>
> > +#include <linux/version.h>
> > +#include <linux/vmalloc.h>
> > +#include <linux/mm.h>
> > +#include <linux/clockchips.h>
> > +#include <linux/hyperv.h>
> > +#include <linux/slab.h>
> > +#include <linux/cpuhotplug.h>
>
> We usually order the includes
>
> #include <linux/....>
> ...
> #include <linux/....>
>
> #include <asm/....>
> #include <asm/....>
>
>
> > -void hyperv_init(void);
> > +void __init hyperv_init(void);
> > void hyperv_setup_mmu_ops(void);
> > void hyper_alloc_mmu(void);
> > void hyperv_report_panic(struct pt_regs *regs, long err);
> > @@ -269,14 +269,16 @@ void hyperv_reenlightenment_intr(struct pt_regs
> *regs);
> > void set_hv_tscchange_cb(void (*cb)(void));
> > void clear_hv_tscchange_cb(void);
> > void hyperv_stop_tsc_emulation(void);
> > +void hv_apic_init(void);
> > #else /* CONFIG_HYPERV */
> > -static inline void hyperv_init(void) {}
> > +static __init inline void hyperv_init(void) {}
>
> The __init on the empty inline function is pointless.
>
> Other than the few nits. This looks well done!

Thanks Thomas. I will address all the issues you have brought up in
the next version.

Regards,

K. Y

2018-04-27 06:13:06

[permalink] [raw]

Subject: RE: [PATCH 2/5] X86: Hyper-V: Enable IPI enlightenments

> -----Original Message-----
> From: Thomas Gleixner <[email protected]>
> Sent: Thursday, April 26, 2018 3:09 PM
> To: KY Srinivasan <[email protected]>
> Cc: [email protected]; [email protected]; linux-
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected]; Stephen
> Hemminger <[email protected]>; Michael Kelley (EOSG)
> <[email protected]>; [email protected]
> Subject: Re: [PATCH 2/5] X86: Hyper-V: Enable IPI enlightenments
>
> On Wed, 25 Apr 2018, [email protected] wrote:
> > +static int __send_ipi_mask(const struct cpumask *mask, int vector)
> > +{
> > + int cur_cpu, vcpu;
> > + struct ipi_arg_non_ex **arg;
> > + struct ipi_arg_non_ex *ipi_arg;
> > + int ret = 1;
>
> So this indicates whether __send_ipi_mask() can send to @mask or not. So
> please make it a bool and let it return false when it does not work, true
> otherwise. If you had used -Exxxx then it would have been more obvious,
> but
> this is really a boolean decision.

Agreed.
>
> > + unsigned long flags;
> > +
> > + if (cpumask_empty(mask))
> > + return 0;
> > +
> > + if (!hv_hypercall_pg)
> > + return ret;
> > +
> > + if ((vector < HV_IPI_LOW_VECTOR) || (vector >
> HV_IPI_HIGH_VECTOR))
> > + return ret;
> > +
> > + local_irq_save(flags);
> > + arg = (struct ipi_arg_non_ex
> **)this_cpu_ptr(hyperv_pcpu_input_arg);
> > +
> > + ipi_arg = *arg;
> > + if (unlikely(!ipi_arg))
> > + goto ipi_mask_done;
> > +
> > +
>
> Stray newline
>
> > + ipi_arg->vector = vector;
> > + ipi_arg->reserved = 0;
> > + ipi_arg->cpu_mask = 0;
> > +
> > + for_each_cpu(cur_cpu, mask) {
> > + vcpu = hv_cpu_number_to_vp_number(cur_cpu);
> > + if (vcpu >= 64)
> > + goto ipi_mask_done;
>
> This is completely magic and deserves a comment.
>
> > +
> > + __set_bit(vcpu, (unsigned long *)&ipi_arg->cpu_mask);
> > + }
> > +
> > + ret = hv_do_hypercall(HVCALL_SEND_IPI, ipi_arg, NULL);
> > +
> > +ipi_mask_done:
> > + local_irq_restore(flags);
> > + return ret;
> > +}
>
> ....
>
> > static int hv_cpu_init(unsigned int cpu)
> > {
> > u64 msr_vp_index;
> > struct hv_vp_assist_page **hvp =
> &hv_vp_assist_page[smp_processor_id()];
> > + void **input_arg;
> > +
> > + input_arg = (void **)this_cpu_ptr(hyperv_pcpu_input_arg);
> > + *input_arg = page_address(alloc_page(GFP_ATOMIC));
>
> This is called from the cpu hotplug thread and there is no need for an
> atomic allocation. Please use GFP_KERNEL.
>
> > hv_get_vp_index(msr_vp_index);
> >
> > @@ -217,6 +224,10 @@ static int hv_cpu_die(unsigned int cpu)
> > {
> > struct hv_reenlightenment_control re_ctrl;
> > unsigned int new_cpu;
> > + void **input_arg;
> > +
> > + input_arg = (void **)this_cpu_ptr(hyperv_pcpu_input_arg);
> > + free_page((unsigned long)*input_arg);
>
> Hrm. Again this is called from the CPU hotplug thread when the cou is about
> to go down. But you can be scheduled out after free() and before disabling
> the assist thing below and the pointer persist. There is no guarantee that
> nothing sends an IPI anymore after this point.
>
> So you have two options here:
>
> 1) Disable interrupts, get the pointer, set the per cpu pointer to NULL,
> reenable interruots and free the page
I will implement this approach.
>
> 2) Keep the page around and check for it in the CPU UP path and avoid the
> allocation when the CPU comes online again.
>
> > if (hv_vp_assist_page && hv_vp_assist_page[cpu])
> > wrmsrl(HV_X64_MSR_VP_ASSIST_PAGE, 0);
> > @@ -260,6 +271,12 @@ void __init hyperv_init(void)
> > if ((ms_hyperv.features & required_msrs) != required_msrs)
> > return;
> >
> > + /* Allocate the per-CPU state for the hypercall input arg */
> > + hyperv_pcpu_input_arg = alloc_percpu(void *);
> > +
> > + if (hyperv_pcpu_input_arg == NULL)
> > + return;
>
> Huch. When that allocation fails, you return and ignore the rest of the
> function which has been there before. Weird decision.
I should have explained this. Failure of this allocation means that we would not have the
per-cpu hypercall input page which in turn would mean that we would not be able to invoke
any hypercalls.

Regards,

K. Y

2018-04-27 06:25:46

[permalink] [raw]

Subject: RE: [PATCH 3/5] X86: Hyper-V: Enhanced IPI enlightenment

> -----Original Message-----
> From: Thomas Gleixner <[email protected]>
> Sent: Thursday, April 26, 2018 3:17 PM
> To: KY Srinivasan <[email protected]>
> Cc: [email protected]; [email protected]; linux-
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected]; Stephen
> Hemminger <[email protected]>; Michael Kelley (EOSG)
> <[email protected]>; [email protected]
> Subject: Re: [PATCH 3/5] X86: Hyper-V: Enhanced IPI enlightenment
>
> On Wed, 25 Apr 2018, [email protected] wrote:
> >
> > +struct ipi_arg_ex {
> > + u32 vector;
> > + u32 reserved;
> > + struct hv_vpset vp_set;
>
> Please align that in tabular fashion for easy of reading
>
> u32 vector;
> u32 reserved;
> struct hv_vpset vp_set;
>
> > +};
> > +
> > static struct apic orig_apic;
> >
> > static u64 hv_apic_icr_read(void)
> > @@ -97,6 +103,40 @@ static void hv_apic_eoi_write(u32 reg, u32 val)
> > * IPI implementation on Hyper-V.
> > */
> >
> > +static int __send_ipi_mask_ex(const struct cpumask *mask, int vector)
> > +{
> > + int nr_bank = 0;
> > + struct ipi_arg_ex **arg;
> > + struct ipi_arg_ex *ipi_arg;
> > + int ret = 1;
> > + unsigned long flags;
>
> This is really horrible to read.
>
> struct ipi_arg_ex *ipi_arg;
> struct ipi_arg_ex **arg;
> unsigned long flags;
> bool ret = false;
> int nr_bank = 0;
>
> is really more conveniant for quick reading.
>
> So the other more limited function has a lot more sanity checks vs. vector
> number and other things. Why are they not required here? Comment
> please.

Yes, I will add the comments. This function is called from the other function
after all the sanity checks have been done and hence are not replicated here.
>
> > + local_irq_save(flags);
> > + arg = (struct ipi_arg_ex **)this_cpu_ptr(hyperv_pcpu_input_arg);
> > +
> > + ipi_arg = *arg;
> > + if (unlikely(!ipi_arg))
> > + goto ipi_mask_ex_done;
> > +
> > + ipi_arg->vector = vector;
> > + ipi_arg->reserved = 0;
> > + ipi_arg->vp_set.valid_bank_mask = 0;
> > +
> > + if (!cpumask_equal(mask, cpu_present_mask)) {
> > + ipi_arg->vp_set.format = HV_GENERIC_SET_SPARCE_4K;
> > + nr_bank = cpumask_to_vpset(&(ipi_arg->vp_set), mask);
>
> nr_bank really confused me. bank_nr is what you mean, not number of
> banks,
> right?
It is the number of banks. The hypercall used here is a variable length
hypercall.

Regards,

K. Y

2018-04-27 06:26:41

[permalink] [raw]

Subject: Re: [PATCH 2/5] X86: Hyper-V: Enable IPI enlightenments

Hi Srinivasan,

I love your patch! Yet something to improve:

[auto build test ERROR on v4.17-rc2]
[also build test ERROR on next-20180426]
[cannot apply to tip/x86/core]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url: https://github.com/0day-ci/linux/commits/kys-linuxonhyperv-com/X86-Hyper-V-APIC-enlightenments/20180427-114416
config: i386-randconfig-b0-04270034 (attached as .config)
compiler: gcc-4.9 (Debian 4.9.4-2) 4.9.4
reproduce:
# save the attached .config to linux build tree
make ARCH=i386

All errors (new ones prefixed by >>):

arch/x86//hyperv/hv_apic.c: In function 'hv_apic_read':
arch/x86//hyperv/hv_apic.c:73:3: error: implicit declaration of function 'native_apic_mem_read' [-Werror=implicit-function-declaration]
return native_apic_mem_read(reg);
^
arch/x86//hyperv/hv_apic.c: In function 'hv_apic_write':
arch/x86//hyperv/hv_apic.c:87:3: error: implicit declaration of function 'native_apic_mem_write' [-Werror=implicit-function-declaration]
native_apic_mem_write(reg, val);
^
arch/x86//hyperv/hv_apic.c: In function 'hv_send_ipi':
>> arch/x86//hyperv/hv_apic.c:155:3: error: invalid use of undefined type 'struct apic'
orig_apic.send_IPI(cpu, vector);
^
arch/x86//hyperv/hv_apic.c: In function 'hv_send_ipi_mask':
arch/x86//hyperv/hv_apic.c:161:3: error: invalid use of undefined type 'struct apic'
orig_apic.send_IPI_mask(mask, vector);
^
arch/x86//hyperv/hv_apic.c: In function 'hv_send_ipi_mask_allbutself':
arch/x86//hyperv/hv_apic.c:174:3: error: invalid use of undefined type 'struct apic'
orig_apic.send_IPI_mask_allbutself(mask, vector);
^
arch/x86//hyperv/hv_apic.c: In function 'hv_send_ipi_all':
arch/x86//hyperv/hv_apic.c:185:3: error: invalid use of undefined type 'struct apic'
orig_apic.send_IPI_all(vector);
^
arch/x86//hyperv/hv_apic.c: In function 'hv_send_ipi_self':
arch/x86//hyperv/hv_apic.c:191:3: error: invalid use of undefined type 'struct apic'
orig_apic.send_IPI_self(vector);
^
arch/x86//hyperv/hv_apic.c: In function 'hv_apic_init':
arch/x86//hyperv/hv_apic.c:204:16: error: 'apic' undeclared (first use in this function)
orig_apic = *apic;
^
arch/x86//hyperv/hv_apic.c:204:16: note: each undeclared identifier is reported only once for each function it appears in
>> arch/x86//hyperv/hv_apic.c:204:3: error: 'orig_apic' has an incomplete type
orig_apic = *apic;
^
arch/x86//hyperv/hv_apic.c:217:3: error: implicit declaration of function 'apic_set_eoi_write' [-Werror=implicit-function-declaration]
apic_set_eoi_write(hv_apic_eoi_write);
^
cc1: some warnings being treated as errors

vim +/orig_apic +204 arch/x86//hyperv/hv_apic.c

59
60 static u32 hv_apic_read(u32 reg)
61 {
62 u32 reg_val, hi;
63
64 switch (reg) {
65 case APIC_EOI:
66 rdmsr(HV_X64_MSR_EOI, reg_val, hi);
67 return reg_val;
68 case APIC_TASKPRI:
69 rdmsr(HV_X64_MSR_TPR, reg_val, hi);
70 return reg_val;
71
72 default:
> 73 return native_apic_mem_read(reg);
74 }
75 }
76
77 static void hv_apic_write(u32 reg, u32 val)
78 {
79 switch (reg) {
80 case APIC_EOI:
81 wrmsr(HV_X64_MSR_EOI, val, 0);
82 break;
83 case APIC_TASKPRI:
84 wrmsr(HV_X64_MSR_TPR, val, 0);
85 break;
86 default:
87 native_apic_mem_write(reg, val);
88 }
89 }
90
91 static void hv_apic_eoi_write(u32 reg, u32 val)
92 {
93 wrmsr(HV_X64_MSR_EOI, val, 0);
94 }
95
96 /*
97 * IPI implementation on Hyper-V.
98 */
99
100 static int __send_ipi_mask(const struct cpumask *mask, int vector)
101 {
102 int cur_cpu, vcpu;
103 struct ipi_arg_non_ex **arg;
104 struct ipi_arg_non_ex *ipi_arg;
105 int ret = 1;
106 unsigned long flags;
107
108 if (cpumask_empty(mask))
109 return 0;
110
111 if (!hv_hypercall_pg)
112 return ret;
113
114 if ((vector < HV_IPI_LOW_VECTOR) || (vector > HV_IPI_HIGH_VECTOR))
115 return ret;
116
117 local_irq_save(flags);
118 arg = (struct ipi_arg_non_ex **)this_cpu_ptr(hyperv_pcpu_input_arg);
119
120 ipi_arg = *arg;
121 if (unlikely(!ipi_arg))
122 goto ipi_mask_done;
123
124
125 ipi_arg->vector = vector;
126 ipi_arg->reserved = 0;
127 ipi_arg->cpu_mask = 0;
128
129 for_each_cpu(cur_cpu, mask) {
130 vcpu = hv_cpu_number_to_vp_number(cur_cpu);
131 if (vcpu >= 64)
132 goto ipi_mask_done;
133
134 __set_bit(vcpu, (unsigned long *)&ipi_arg->cpu_mask);
135 }
136
137 ret = hv_do_hypercall(HVCALL_SEND_IPI, ipi_arg, NULL);
138
139 ipi_mask_done:
140 local_irq_restore(flags);
141 return ret;
142 }
143
144 static int __send_ipi_one(int cpu, int vector)
145 {
146 struct cpumask mask = CPU_MASK_NONE;
147
148 cpumask_set_cpu(cpu, &mask);
149 return __send_ipi_mask(&mask, vector);
150 }
151
152 static void hv_send_ipi(int cpu, int vector)
153 {
154 if (__send_ipi_one(cpu, vector))
> 155 orig_apic.send_IPI(cpu, vector);
156 }
157
158 static void hv_send_ipi_mask(const struct cpumask *mask, int vector)
159 {
160 if (__send_ipi_mask(mask, vector))
161 orig_apic.send_IPI_mask(mask, vector);
162 }
163
164 static void hv_send_ipi_mask_allbutself(const struct cpumask *mask, int vector)
165 {
166 unsigned int this_cpu = smp_processor_id();
167 struct cpumask new_mask;
168 const struct cpumask *local_mask;
169
170 cpumask_copy(&new_mask, mask);
171 cpumask_clear_cpu(this_cpu, &new_mask);
172 local_mask = &new_mask;
173 if (__send_ipi_mask(local_mask, vector))
174 orig_apic.send_IPI_mask_allbutself(mask, vector);
175 }
176
177 static void hv_send_ipi_allbutself(int vector)
178 {
179 hv_send_ipi_mask_allbutself(cpu_online_mask, vector);
180 }
181
182 static void hv_send_ipi_all(int vector)
183 {
184 if (__send_ipi_mask(cpu_online_mask, vector))
185 orig_apic.send_IPI_all(vector);
186 }
187
188 static void hv_send_ipi_self(int vector)
189 {
190 if (__send_ipi_one(smp_processor_id(), vector))
> 191 orig_apic.send_IPI_self(vector);
192 }
193
194 void __init hv_apic_init(void)
195 {
196 if (ms_hyperv.hints & HV_X64_CLUSTER_IPI_RECOMMENDED) {
197 if (hyperv_pcpu_input_arg == NULL)
198 goto msr_based_access;
199
200 pr_info("Hyper-V: Using IPI hypercalls\n");
201 /*
202 * Set the IPI entry points.
203 */
> 204 orig_apic = *apic;

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation

Attachments:

(No filename) (7.03 kB)
.config.gz (24.61 kB)
Download all attachments

2018-04-27 06:31:01

[permalink] [raw]

Subject: RE: [PATCH 5/5] X86: Hyper-V: Consolidate the allocation of the hypercall input page

> -----Original Message-----
> From: Thomas Gleixner <[email protected]>
> Sent: Thursday, April 26, 2018 3:24 PM
> To: KY Srinivasan <[email protected]>
> Cc: [email protected]; [email protected]; linux-
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected]; Stephen
> Hemminger <[email protected]>; Michael Kelley (EOSG)
> <[email protected]>; [email protected]
> Subject: Re: [PATCH 5/5] X86: Hyper-V: Consolidate the allocation of the
> hypercall input page
>
> On Wed, 25 Apr 2018, [email protected] wrote:
>
> > From: "K. Y. Srinivasan" <[email protected]>
> >
> > Consolidate the allocation of the hypercall input page.
>
> Again. You can provide the new way of allocation first, then you don't have
> to add code first in order to remove it later again.

I have implemented the new way upfront for the new code - the IPI code
[PATCH 2/5] X86: Hyper-V: Enable IPI enlightenments.
What I am doing here using that infrastructure for the TLB flush enlightenments
and getting rid of unnecessary code.

Regards,

K. Y

2018-04-27 06:32:53

[permalink] [raw]

Subject: RE: [PATCH 2/5] X86: Hyper-V: Enable IPI enlightenments

> -----Original Message-----
> From: Michael Kelley (EOSG)
> Sent: Thursday, April 26, 2018 3:55 PM
> To: KY Srinivasan <[email protected]>; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected]; Stephen
> Hemminger <[email protected]>; [email protected]
> Subject: RE: [PATCH 2/5] X86: Hyper-V: Enable IPI enlightenments
>
> > -----Original Message-----
> > From: [email protected] <[email protected]>
> > Sent: Wednesday, April 25, 2018 11:13 AM
> > To: [email protected]; [email protected]; linux-
> [email protected];
> > [email protected]; [email protected]; [email protected];
> [email protected];
> > [email protected]; [email protected]; Stephen Hemminger
> <[email protected]>;
> > Michael Kelley (EOSG) <[email protected]>;
> [email protected]
> > Cc: KY Srinivasan <[email protected]>
> > Subject: [PATCH 2/5] X86: Hyper-V: Enable IPI enlightenments
> >
> > From: "K. Y. Srinivasan" <[email protected]>
> >
> > Hyper-V supports hypercalls to implement IPI; use them.
> >
> > Signed-off-by: K. Y. Srinivasan <[email protected]>
> > ---
> > arch/x86/hyperv/hv_apic.c | 125
> +++++++++++++++++++++++++++++++++++++
> > arch/x86/hyperv/hv_init.c | 17 +++++
> > arch/x86/include/asm/hyperv-tlfs.h | 9 +++
> > arch/x86/include/asm/mshyperv.h | 1 +
> > 4 files changed, 152 insertions(+)
> >
> > diff --git a/arch/x86/hyperv/hv_apic.c b/arch/x86/hyperv/hv_apic.c
> > index e0a5b36208fc..7f3322ecfb01 100644
> > --- a/arch/x86/hyperv/hv_apic.c
> > +++ b/arch/x86/hyperv/hv_apic.c
> > @@ -30,6 +30,14 @@
> > #include <linux/slab.h>
> > #include <linux/cpuhotplug.h>
> >
> > +struct ipi_arg_non_ex {
> > + u32 vector;
> > + u32 reserved;
> > + u64 cpu_mask;
> > +};
>
> I think we'd like to put structures like this, which are defined in the
> Hyper-V Top Level Functional Spec, in hyperv-tlfs.h. Also, the 5.0b
> version of the TLFS, which is latest, shows this structure on page 100:
>
> u32 vector;
> u8 targetvtl;
> u8 reserved[3];
> u64 cpu_mask;
>

Good point. I will make the necessary adjustments.

Regards,

K. Y

2018-04-27 06:35:34

[permalink] [raw]

Subject: RE: [PATCH 2/5] X86: Hyper-V: Enable IPI enlightenments

> -----Original Message-----
> From: Dan Carpenter <[email protected]>
> Sent: Thursday, April 26, 2018 2:32 PM
> To: KY Srinivasan <[email protected]>
> Cc: [email protected]; [email protected]; linux-
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; Stephen Hemminger <[email protected]>; Michael
> Kelley (EOSG) <[email protected]>; [email protected]
> Subject: Re: [PATCH 2/5] X86: Hyper-V: Enable IPI enlightenments
>
> On Wed, Apr 25, 2018 at 11:12:47AM -0700, [email protected] wrote:
> > +/*
> > + * IPI implementation on Hyper-V.
> > + */
> > +
> > +static int __send_ipi_mask(const struct cpumask *mask, int vector)
> > +{
> > + int cur_cpu, vcpu;
> > + struct ipi_arg_non_ex **arg;
> > + struct ipi_arg_non_ex *ipi_arg;
> > + int ret = 1;
>
> Not specifically related to this patch, but hv code sometimes returns 1
> on error or U64_MAX. It's slightly magical. Maybe
> HV_STATUS_INVALID_HYPERCALL_INPUT (3) would be more appropriate?
> Or we
> could make a new more generic error code:
>
> #define HV_STATUS_INVALID 1

Good point. We will look at cleaning this up.

Regards,

K. Y

2018-04-27 11:00:07

[permalink] [raw]

Subject: RE: [PATCH 5/5] X86: Hyper-V: Consolidate the allocation of the hypercall input page

On Fri, 27 Apr 2018, KY Srinivasan wrote:

>
>
> > -----Original Message-----
> > From: Thomas Gleixner <[email protected]>
> > Sent: Thursday, April 26, 2018 3:24 PM
> > To: KY Srinivasan <[email protected]>
> > Cc: [email protected]; [email protected]; linux-
> > [email protected]; [email protected]; [email protected];
> > [email protected]; [email protected]; [email protected]; Stephen
> > Hemminger <[email protected]>; Michael Kelley (EOSG)
> > <[email protected]>; [email protected]
> > Subject: Re: [PATCH 5/5] X86: Hyper-V: Consolidate the allocation of the
> > hypercall input page
> >
> > On Wed, 25 Apr 2018, [email protected] wrote:
> >
> > > From: "K. Y. Srinivasan" <[email protected]>
> > >
> > > Consolidate the allocation of the hypercall input page.
> >
> > Again. You can provide the new way of allocation first, then you don't have
> > to add code first in order to remove it later again.
>
> I have implemented the new way upfront for the new code - the IPI code
> [PATCH 2/5] X86: Hyper-V: Enable IPI enlightenments.
> What I am doing here using that infrastructure for the TLB flush enlightenments
> and getting rid of unnecessary code.

Ok. I maybe misread it, but a bit more elaborate change log might help to
avoid that.

Thanks,

tglx

2018-04-27 11:23:23

[permalink] [raw]

Subject: Re: [PATCH 2/5] X86: Hyper-V: Enable IPI enlightenments

Hi Srinivasan,

I love your patch! Perhaps something to improve:

[auto build test WARNING on v4.17-rc2]
[also build test WARNING on next-20180426]
[cannot apply to tip/x86/core]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url: https://github.com/0day-ci/linux/commits/kys-linuxonhyperv-com/X86-Hyper-V-APIC-enlightenments/20180427-114416
reproduce:
# apt-get install sparse
make ARCH=x86_64 allmodconfig
make C=1 CF=-D__CHECK_ENDIAN__

sparse warnings: (new ones prefixed by >>)

>> arch/x86/hyperv/hv_init.c:105:30: sparse: incorrect type in initializer (different address spaces) @@ expected void const [noderef] <asn:3>*__vpp_verify @@ got const [noderef] <asn:3>*__vpp_verify @@
arch/x86/hyperv/hv_init.c:105:30: expected void const [noderef] <asn:3>*__vpp_verify
arch/x86/hyperv/hv_init.c:105:30: got void [noderef] <asn:3>**<noident>
arch/x86/hyperv/hv_init.c:229:30: sparse: incorrect type in initializer (different address spaces) @@ expected void const [noderef] <asn:3>*__vpp_verify @@ got const [noderef] <asn:3>*__vpp_verify @@
arch/x86/hyperv/hv_init.c:229:30: expected void const [noderef] <asn:3>*__vpp_verify
arch/x86/hyperv/hv_init.c:229:30: got void [noderef] <asn:3>**<noident>
>> arch/x86/hyperv/hv_init.c:275:31: sparse: incorrect type in assignment (different address spaces) @@ expected void [noderef] <asn:3>**extern [addressable] [toplevel] hyperv_pcpu_input_arg @@ got addressable] [toplevel] hyperv_pcpu_input_arg @@
arch/x86/hyperv/hv_init.c:275:31: expected void [noderef] <asn:3>**extern [addressable] [toplevel] hyperv_pcpu_input_arg
arch/x86/hyperv/hv_init.c:275:31: got void *[noderef] <asn:3>*<noident>
arch/x86/include/asm/paravirt.h:150:9: sparse: cast truncates bits from constant value (8000000000000000 becomes 0)
--
>> arch/x86/hyperv/hv_apic.c:118:41: sparse: incorrect type in initializer (different address spaces) @@ expected void const [noderef] <asn:3>*__vpp_verify @@ got const [noderef] <asn:3>*__vpp_verify @@
arch/x86/hyperv/hv_apic.c:118:41: expected void const [noderef] <asn:3>*__vpp_verify
arch/x86/hyperv/hv_apic.c:118:41: got void [noderef] <asn:3>**<noident>

vim +105 arch/x86/hyperv/hv_init.c

98
99 static int hv_cpu_init(unsigned int cpu)
100 {
101 u64 msr_vp_index;
102 struct hv_vp_assist_page **hvp = &hv_vp_assist_page[smp_processor_id()];
103 void **input_arg;
104
> 105 input_arg = (void **)this_cpu_ptr(hyperv_pcpu_input_arg);
106 *input_arg = page_address(alloc_page(GFP_ATOMIC));
107
108 hv_get_vp_index(msr_vp_index);
109
110 hv_vp_index[smp_processor_id()] = msr_vp_index;
111
112 if (msr_vp_index > hv_max_vp_index)
113 hv_max_vp_index = msr_vp_index;
114
115 if (!hv_vp_assist_page)
116 return 0;
117
118 if (!*hvp)
119 *hvp = __vmalloc(PAGE_SIZE, GFP_KERNEL, PAGE_KERNEL);
120
121 if (*hvp) {
122 u64 val;
123
124 val = vmalloc_to_pfn(*hvp);
125 val = (val << HV_X64_MSR_VP_ASSIST_PAGE_ADDRESS_SHIFT) |
126 HV_X64_MSR_VP_ASSIST_PAGE_ENABLE;
127
128 wrmsrl(HV_X64_MSR_VP_ASSIST_PAGE, val);
129 }
130
131 return 0;
132 }
133
134 static void (*hv_reenlightenment_cb)(void);
135
136 static void hv_reenlightenment_notify(struct work_struct *dummy)
137 {
138 struct hv_tsc_emulation_status emu_status;
139
140 rdmsrl(HV_X64_MSR_TSC_EMULATION_STATUS, *(u64 *)&emu_status);
141
142 /* Don't issue the callback if TSC accesses are not emulated */
143 if (hv_reenlightenment_cb && emu_status.inprogress)
144 hv_reenlightenment_cb();
145 }
146 static DECLARE_DELAYED_WORK(hv_reenlightenment_work, hv_reenlightenment_notify);
147
148 void hyperv_stop_tsc_emulation(void)
149 {
150 u64 freq;
151 struct hv_tsc_emulation_status emu_status;
152
153 rdmsrl(HV_X64_MSR_TSC_EMULATION_STATUS, *(u64 *)&emu_status);
154 emu_status.inprogress = 0;
155 wrmsrl(HV_X64_MSR_TSC_EMULATION_STATUS, *(u64 *)&emu_status);
156
157 rdmsrl(HV_X64_MSR_TSC_FREQUENCY, freq);
158 tsc_khz = div64_u64(freq, 1000);
159 }
160 EXPORT_SYMBOL_GPL(hyperv_stop_tsc_emulation);
161
162 static inline bool hv_reenlightenment_available(void)
163 {
164 /*
165 * Check for required features and priviliges to make TSC frequency
166 * change notifications work.
167 */
168 return ms_hyperv.features & HV_X64_ACCESS_FREQUENCY_MSRS &&
169 ms_hyperv.misc_features & HV_FEATURE_FREQUENCY_MSRS_AVAILABLE &&
170 ms_hyperv.features & HV_X64_ACCESS_REENLIGHTENMENT;
171 }
172
173 __visible void __irq_entry hyperv_reenlightenment_intr(struct pt_regs *regs)
174 {
175 entering_ack_irq();
176
177 inc_irq_stat(irq_hv_reenlightenment_count);
178
179 schedule_delayed_work(&hv_reenlightenment_work, HZ/10);
180
181 exiting_irq();
182 }
183
184 void set_hv_tscchange_cb(void (*cb)(void))
185 {
186 struct hv_reenlightenment_control re_ctrl = {
187 .vector = HYPERV_REENLIGHTENMENT_VECTOR,
188 .enabled = 1,
189 .target_vp = hv_vp_index[smp_processor_id()]
190 };
191 struct hv_tsc_emulation_control emu_ctrl = {.enabled = 1};
192
193 if (!hv_reenlightenment_available()) {
194 pr_warn("Hyper-V: reenlightenment support is unavailable\n");
195 return;
196 }
197
198 hv_reenlightenment_cb = cb;
199
200 /* Make sure callback is registered before we write to MSRs */
201 wmb();
202
203 wrmsrl(HV_X64_MSR_REENLIGHTENMENT_CONTROL, *((u64 *)&re_ctrl));
204 wrmsrl(HV_X64_MSR_TSC_EMULATION_CONTROL, *((u64 *)&emu_ctrl));
205 }
206 EXPORT_SYMBOL_GPL(set_hv_tscchange_cb);
207
208 void clear_hv_tscchange_cb(void)
209 {
210 struct hv_reenlightenment_control re_ctrl;
211
212 if (!hv_reenlightenment_available())
213 return;
214
215 rdmsrl(HV_X64_MSR_REENLIGHTENMENT_CONTROL, *(u64 *)&re_ctrl);
216 re_ctrl.enabled = 0;
217 wrmsrl(HV_X64_MSR_REENLIGHTENMENT_CONTROL, *(u64 *)&re_ctrl);
218
219 hv_reenlightenment_cb = NULL;
220 }
221 EXPORT_SYMBOL_GPL(clear_hv_tscchange_cb);
222
223 static int hv_cpu_die(unsigned int cpu)
224 {
225 struct hv_reenlightenment_control re_ctrl;
226 unsigned int new_cpu;
227 void **input_arg;
228
229 input_arg = (void **)this_cpu_ptr(hyperv_pcpu_input_arg);
230 free_page((unsigned long)*input_arg);
231
232 if (hv_vp_assist_page && hv_vp_assist_page[cpu])
233 wrmsrl(HV_X64_MSR_VP_ASSIST_PAGE, 0);
234
235 if (hv_reenlightenment_cb == NULL)
236 return 0;
237
238 rdmsrl(HV_X64_MSR_REENLIGHTENMENT_CONTROL, *((u64 *)&re_ctrl));
239 if (re_ctrl.target_vp == hv_vp_index[cpu]) {
240 /* Reassign to some other online CPU */
241 new_cpu = cpumask_any_but(cpu_online_mask, cpu);
242
243 re_ctrl.target_vp = hv_vp_index[new_cpu];
244 wrmsrl(HV_X64_MSR_REENLIGHTENMENT_CONTROL, *((u64 *)&re_ctrl));
245 }
246
247 return 0;
248 }
249
250 /*
251 * This function is to be invoked early in the boot sequence after the
252 * hypervisor has been detected.
253 *
254 * 1. Setup the hypercall page.
255 * 2. Register Hyper-V specific clocksource.
256 * 3. Setup Hyper-V specific APIC entry points.
257 */
258 void __init hyperv_init(void)
259 {
260 u64 guest_id, required_msrs;
261 union hv_x64_msr_hypercall_contents hypercall_msr;
262 int cpuhp;
263
264 if (x86_hyper_type != X86_HYPER_MS_HYPERV)
265 return;
266
267 /* Absolutely required MSRs */
268 required_msrs = HV_X64_MSR_HYPERCALL_AVAILABLE |
269 HV_X64_MSR_VP_INDEX_AVAILABLE;
270
271 if ((ms_hyperv.features & required_msrs) != required_msrs)
272 return;
273
274 /* Allocate the per-CPU state for the hypercall input arg */
> 275 hyperv_pcpu_input_arg = alloc_percpu(void *);
276
277 if (hyperv_pcpu_input_arg == NULL)
278 return;
279
280 /* Allocate percpu VP index */
281 hv_vp_index = kmalloc_array(num_possible_cpus(), sizeof(*hv_vp_index),
282 GFP_KERNEL);
283 if (!hv_vp_index)
284 return;
285
286 hv_vp_assist_page = kcalloc(num_possible_cpus(),
287 sizeof(*hv_vp_assist_page), GFP_KERNEL);
288 if (!hv_vp_assist_page) {
289 ms_hyperv.hints &= ~HV_X64_ENLIGHTENED_VMCS_RECOMMENDED;
290 goto free_vp_index;
291 }
292
293 cpuhp = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "x86/hyperv_init:online",
294 hv_cpu_init, hv_cpu_die);
295 if (cpuhp < 0)
296 goto free_vp_assist_page;
297
298 /*
299 * Setup the hypercall page and enable hypercalls.
300 * 1. Register the guest ID
301 * 2. Enable the hypercall and register the hypercall page
302 */
303 guest_id = generate_guest_id(0, LINUX_VERSION_CODE, 0);
304 wrmsrl(HV_X64_MSR_GUEST_OS_ID, guest_id);
305
306 hv_hypercall_pg = __vmalloc(PAGE_SIZE, GFP_KERNEL, PAGE_KERNEL_RX);
307 if (hv_hypercall_pg == NULL) {
308 wrmsrl(HV_X64_MSR_GUEST_OS_ID, 0);
309 goto remove_cpuhp_state;
310 }
311
312 rdmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
313 hypercall_msr.enable = 1;
314 hypercall_msr.guest_physical_address = vmalloc_to_pfn(hv_hypercall_pg);
315 wrmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
316
317 hyper_alloc_mmu();
318
319 hv_apic_init();
320
321 /*
322 * Register Hyper-V specific clocksource.
323 */
324 #ifdef CONFIG_HYPERV_TSCPAGE
325 if (ms_hyperv.features & HV_X64_MSR_REFERENCE_TSC_AVAILABLE) {
326 union hv_x64_msr_hypercall_contents tsc_msr;
327
328 tsc_pg = __vmalloc(PAGE_SIZE, GFP_KERNEL, PAGE_KERNEL);
329 if (!tsc_pg)
330 goto register_msr_cs;
331
332 hyperv_cs = &hyperv_cs_tsc;
333
334 rdmsrl(HV_X64_MSR_REFERENCE_TSC, tsc_msr.as_uint64);
335
336 tsc_msr.enable = 1;
337 tsc_msr.guest_physical_address = vmalloc_to_pfn(tsc_pg);
338
339 wrmsrl(HV_X64_MSR_REFERENCE_TSC, tsc_msr.as_uint64);
340
341 hyperv_cs_tsc.archdata.vclock_mode = VCLOCK_HVCLOCK;
342
343 clocksource_register_hz(&hyperv_cs_tsc, NSEC_PER_SEC/100);
344 return;
345 }
346 register_msr_cs:
347 #endif
348 /*
349 * For 32 bit guests just use the MSR based mechanism for reading
350 * the partition counter.
351 */
352
353 hyperv_cs = &hyperv_cs_msr;
354 if (ms_hyperv.features & HV_X64_MSR_TIME_REF_COUNT_AVAILABLE)
355 clocksource_register_hz(&hyperv_cs_msr, NSEC_PER_SEC/100);
356
357 return;
358
359 remove_cpuhp_state:
360 cpuhp_remove_state(cpuhp);
361 free_vp_assist_page:
362 kfree(hv_vp_assist_page);
363 hv_vp_assist_page = NULL;
364 free_vp_index:
365 kfree(hv_vp_index);
366 hv_vp_index = NULL;
367 }
368

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation

2018-04-27 11:54:04

[permalink] [raw]

Subject: Re: [PATCH 5/5] X86: Hyper-V: Consolidate the allocation of the hypercall input page

2018-04-27 11:56:42