[PATCH] x86/hyper-v: Zero out the VP assist page to fix CPU offlining

[PATCH] x86/hyper-v: Zero out the VP assist page to fix CPU offlining

[Dexuan Cui]

When a CPU is being offlined, the CPU usually still receives a few
interrupts (e.g. reschedule IPIs), after hv_cpu_die() disables the
HV_X64_MSR_VP_ASSIST_PAGE, so hv_apic_eoi_write() may not write the EOI
MSR, if the apic_assist field's bit0 happens to be 1; as a result, Hyper-V
may not be able to deliver all the interrupts to the CPU, and the CPU may
not be stopped, and the kernel will hang soon.

The VP ASSIST PAGE is an "overlay" page (see Hyper-V TLFS's Section
5.2.1 "GPA Overlay Pages"), so with this fix we're sure the apic_assist
field is still zero, after the VP ASSIST PAGE is disabled.

Fixes: ba696429d290 ("x86/hyper-v: Implement EOI assist")
Signed-off-by: Dexuan Cui <decui@microsoft.com>
---
 arch/x86/hyperv/hv_init.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
index 0e033ef11a9f..db51a301f759 100644
--- a/arch/x86/hyperv/hv_init.c
+++ b/arch/x86/hyperv/hv_init.c
@@ -60,8 +60,14 @@ static int hv_cpu_init(unsigned int cpu)
 	if (!hv_vp_assist_page)
 		return 0;
 
+	/*
+	 * The ZERO flag is necessary, because in the case of CPU offlining
+	 * the page can still be used by hv_apic_eoi_write() for a while,
+	 * after the VP ASSIST PAGE is disabled in hv_cpu_die().
+	 */
 	if (!*hvp)
-		*hvp = __vmalloc(PAGE_SIZE, GFP_KERNEL, PAGE_KERNEL);
+		*hvp = __vmalloc(PAGE_SIZE, GFP_KERNEL | __GFP_ZERO,
+				 PAGE_KERNEL);
 
 	if (*hvp) {
 		u64 val;
-- 
2.19.1

RE: [PATCH] x86/hyper-v: Zero out the VP assist page to fix CPU offlining

[Michael Kelley]

From: Dexuan Cui <decui@microsoft.com> Sent: Wednesday, July 3, 2019 6:46 PM
> 
> When a CPU is being offlined, the CPU usually still receives a few
> interrupts (e.g. reschedule IPIs), after hv_cpu_die() disables the
> HV_X64_MSR_VP_ASSIST_PAGE, so hv_apic_eoi_write() may not write the EOI
> MSR, if the apic_assist field's bit0 happens to be 1; as a result, Hyper-V
> may not be able to deliver all the interrupts to the CPU, and the CPU may
> not be stopped, and the kernel will hang soon.
> 
> The VP ASSIST PAGE is an "overlay" page (see Hyper-V TLFS's Section
> 5.2.1 "GPA Overlay Pages"), so with this fix we're sure the apic_assist
> field is still zero, after the VP ASSIST PAGE is disabled.
> 
> Fixes: ba696429d290 ("x86/hyper-v: Implement EOI assist")
> Signed-off-by: Dexuan Cui <decui@microsoft.com>
> ---
>  arch/x86/hyperv/hv_init.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
> index 0e033ef11a9f..db51a301f759 100644
> --- a/arch/x86/hyperv/hv_init.c
> +++ b/arch/x86/hyperv/hv_init.c
> @@ -60,8 +60,14 @@ static int hv_cpu_init(unsigned int cpu)
>  	if (!hv_vp_assist_page)
>  		return 0;
> 
> +	/*
> +	 * The ZERO flag is necessary, because in the case of CPU offlining
> +	 * the page can still be used by hv_apic_eoi_write() for a while,
> +	 * after the VP ASSIST PAGE is disabled in hv_cpu_die().
> +	 */
>  	if (!*hvp)
> -		*hvp = __vmalloc(PAGE_SIZE, GFP_KERNEL, PAGE_KERNEL);
> +		*hvp = __vmalloc(PAGE_SIZE, GFP_KERNEL | __GFP_ZERO,
> +				 PAGE_KERNEL);
> 
>  	if (*hvp) {
>  		u64 val;
> --
> 2.19.1

Reviewed-by:  Michael Kelley <mikelley@microsoft.com>

Re: [PATCH] x86/hyper-v: Zero out the VP assist page to fix CPU offlining

[Thomas Gleixner]

Dexuan,

On Thu, 4 Jul 2019, Dexuan Cui wrote:

> When a CPU is being offlined, the CPU usually still receives a few
> interrupts (e.g. reschedule IPIs), after hv_cpu_die() disables the
> HV_X64_MSR_VP_ASSIST_PAGE, so hv_apic_eoi_write() may not write the EOI
> MSR, if the apic_assist field's bit0 happens to be 1; as a result, Hyper-V
> may not be able to deliver all the interrupts to the CPU, and the CPU may
> not be stopped, and the kernel will hang soon.
> 
> The VP ASSIST PAGE is an "overlay" page (see Hyper-V TLFS's Section
> 5.2.1 "GPA Overlay Pages"), so with this fix we're sure the apic_assist
> field is still zero, after the VP ASSIST PAGE is disabled.
> 
> Fixes: ba696429d290 ("x86/hyper-v: Implement EOI assist")
> Signed-off-by: Dexuan Cui <decui@microsoft.com>
> ---
>  arch/x86/hyperv/hv_init.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
> index 0e033ef11a9f..db51a301f759 100644
> --- a/arch/x86/hyperv/hv_init.c
> +++ b/arch/x86/hyperv/hv_init.c
> @@ -60,8 +60,14 @@ static int hv_cpu_init(unsigned int cpu)
>  	if (!hv_vp_assist_page)
>  		return 0;
>  
> +	/*
> +	 * The ZERO flag is necessary, because in the case of CPU offlining
> +	 * the page can still be used by hv_apic_eoi_write() for a while,
> +	 * after the VP ASSIST PAGE is disabled in hv_cpu_die().
> +	 */
>  	if (!*hvp)
> -		*hvp = __vmalloc(PAGE_SIZE, GFP_KERNEL, PAGE_KERNEL);
> +		*hvp = __vmalloc(PAGE_SIZE, GFP_KERNEL | __GFP_ZERO,
> +				 PAGE_KERNEL);

This is the allocation when the CPU is brought online for the first
time. So what effect has zeroing at allocation time vs. offlining and
potentially receiving IPIs? That allocation is never freed.

Neither the comment nor the changelog make any sense to me.

Thanks,

	tglx

RE: [PATCH] x86/hyper-v: Zero out the VP assist page to fix CPU offlining

[Dexuan Cui]

> From: Thomas Gleixner <tglx@linutronix.de>
> Sent: Wednesday, July 17, 2019 4:04 PM
> To: Dexuan Cui <decui@microsoft.com>
> ...
> On Thu, 4 Jul 2019, Dexuan Cui wrote:
> > When a CPU is being offlined, the CPU usually still receives a few
> > interrupts (e.g. reschedule IPIs), after hv_cpu_die() disables the
> > HV_X64_MSR_VP_ASSIST_PAGE, so hv_apic_eoi_write() may not write 
> > the EOI MSR, if the apic_assist field's bit0 happens to be 1; as a result,
> > Hyper-V may not be able to deliver all the interrupts to the CPU, and the
> > CPU may not be stopped, and the kernel will hang soon.
> >
> > The VP ASSIST PAGE is an "overlay" page (see Hyper-V TLFS's Section
> > 5.2.1 "GPA Overlay Pages"), so with this fix we're sure the apic_assist
> > field is still zero, after the VP ASSIST PAGE is disabled.
> >
> > Fixes: ba696429d290 ("x86/hyper-v: Implement EOI assist")
> > Signed-off-by: Dexuan Cui <decui@microsoft.com>
> > 
> > diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
> > index 0e033ef11a9f..db51a301f759 100644
> > --- a/arch/x86/hyperv/hv_init.c
> > +++ b/arch/x86/hyperv/hv_init.c
> > @@ -60,8 +60,14 @@ static int hv_cpu_init(unsigned int cpu)
> >  	if (!hv_vp_assist_page)
> >  		return 0;
> >
> > +	/*
> > +	 * The ZERO flag is necessary, because in the case of CPU offlining
> > +	 * the page can still be used by hv_apic_eoi_write() for a while,
> > +	 * after the VP ASSIST PAGE is disabled in hv_cpu_die().
> > +	 */
> >  	if (!*hvp)
> > -		*hvp = __vmalloc(PAGE_SIZE, GFP_KERNEL, PAGE_KERNEL);
> > +		*hvp = __vmalloc(PAGE_SIZE, GFP_KERNEL | __GFP_ZERO,
> > +				 PAGE_KERNEL);
> 
> This is the allocation when the CPU is brought online for the first
> time. So what effect has zeroing at allocation time vs. offlining and
> potentially receiving IPIs? That allocation is never freed.
> 
> Neither the comment nor the changelog make any sense to me.
> 	tglx

That allocation was introduced by the commit
a46d15cc1ae5 ("x86/hyper-v: allocate and use Virtual Processor Assist Pages").

I think it's ok to not free the page when a CPU is offlined: every
CPU uses only 1 page and CPU offlining is not really a very usual
operation except for the scenario of hibernation (and suspend-to-memory), 
where the CPUs are quickly onlined again, when we resume from hibernation.
IMO Vitaly intentionally decided to not free the page for simplicity of the
code.

When a CPU (e.g. CPU1) is being onlined, in hv_cpu_init(), we allocate the
VP_ASSIST_PAGE page and enable the PV EOI optimization for this CPU by
writing the MSR HV_X64_MSR_VP_ASSIST_PAGE. From now on, this CPU
*always* uses hvp->apic_assist (which is updated by the hypervisor) to
decide if it needs to write the EOI MSR:

static void hv_apic_eoi_write(u32 reg, u32 val)
{
        struct hv_vp_assist_page *hvp = hv_vp_assist_page[smp_processor_id()];

        if (hvp && (xchg(&hvp->apic_assist, 0) & 0x1))
                return;

        wrmsr(HV_X64_MSR_EOI, val, 0);
}

When a CPU (e.g. CPU1) is being offlined, on this CPU, we do:
1. in hv_cpu_die(), we disable the PV EOI optimizaton for this CPU;
2. we finish the remaining work of stopping this CPU;
3. this CPU is completed stopped.

Between 1 and 3, this CPU can still receive interrupts (e.g. IPIs from CPU0,
and Local APIC timer interrupts), and this CPU *must* write the EOI MSR for
every interrupt received, otherwise the hypervisor may not deliver further
interrupts, which may be needed to stop this CPU completely.

So we need to make sure hvp->apic_assist.bit0 is zero, after we run the line
"wrmsrl(HV_X64_MSR_VP_ASSIST_PAGE, 0);" in hv_cpu_die(). The easiest
way is what I do in this patch. Alternatively, we can use the below patch:

@@ -188,8 +188,12 @@ static int hv_cpu_die(unsigned int cpu)
        local_irq_restore(flags);
        free_page((unsigned long)input_pg);

-       if (hv_vp_assist_page && hv_vp_assist_page[cpu])
+       if (hv_vp_assist_page && hv_vp_assist_page[cpu]) {
+               local_irq_save(flags);
                wrmsrl(HV_X64_MSR_VP_ASSIST_PAGE, 0);
+               hvp->apic_assist &= ~1;
+               local_irq_restore(flags);
+       }

        if (hv_reenlightenment_cb == NULL)
                return 0;

This second version needs 3+ lines, so I prefer the one-line version. :-)

Thanks,
-- Dexuan

RE: [PATCH] x86/hyper-v: Zero out the VP assist page to fix CPU offlining

[Thomas Gleixner]

On Thu, 18 Jul 2019, Dexuan Cui wrote:
> > On Thu, 4 Jul 2019, Dexuan Cui wrote:
> > This is the allocation when the CPU is brought online for the first
> > time. So what effect has zeroing at allocation time vs. offlining and
> > potentially receiving IPIs? That allocation is never freed.
> > 
> > Neither the comment nor the changelog make any sense to me.
> > 	tglx
> 
> That allocation was introduced by the commit
> a46d15cc1ae5 ("x86/hyper-v: allocate and use Virtual Processor Assist Pages").
> 
> I think it's ok to not free the page when a CPU is offlined: every
> CPU uses only 1 page and CPU offlining is not really a very usual
> operation except for the scenario of hibernation (and suspend-to-memory), 
> where the CPUs are quickly onlined again, when we resume from hibernation.
> IMO Vitaly intentionally decided to not free the page for simplicity of the
> code.
> 
> When a CPU (e.g. CPU1) is being onlined, in hv_cpu_init(), we allocate the
> VP_ASSIST_PAGE page and enable the PV EOI optimization for this CPU by
> writing the MSR HV_X64_MSR_VP_ASSIST_PAGE. From now on, this CPU
> *always* uses hvp->apic_assist (which is updated by the hypervisor) to
> decide if it needs to write the EOI MSR:
> 
> static void hv_apic_eoi_write(u32 reg, u32 val)
> {
>         struct hv_vp_assist_page *hvp = hv_vp_assist_page[smp_processor_id()];
> 
>         if (hvp && (xchg(&hvp->apic_assist, 0) & 0x1))
>                 return;
> 
>         wrmsr(HV_X64_MSR_EOI, val, 0);
> }
> 
> When a CPU (e.g. CPU1) is being offlined, on this CPU, we do:
> 1. in hv_cpu_die(), we disable the PV EOI optimizaton for this CPU;
> 2. we finish the remaining work of stopping this CPU;
> 3. this CPU is completed stopped.
> 
> Between 1 and 3, this CPU can still receive interrupts (e.g. IPIs from CPU0,
> and Local APIC timer interrupts), and this CPU *must* write the EOI MSR for
> every interrupt received, otherwise the hypervisor may not deliver further
> interrupts, which may be needed to stop this CPU completely.
> 
> So we need to make sure hvp->apic_assist.bit0 is zero, after we run the line
> "wrmsrl(HV_X64_MSR_VP_ASSIST_PAGE, 0);" in hv_cpu_die(). The easiest
> way is what I do in this patch. Alternatively, we can use the below patch:
> 
> @@ -188,8 +188,12 @@ static int hv_cpu_die(unsigned int cpu)
>         local_irq_restore(flags);
>         free_page((unsigned long)input_pg);
> 
> -       if (hv_vp_assist_page && hv_vp_assist_page[cpu])
> +       if (hv_vp_assist_page && hv_vp_assist_page[cpu]) {
> +               local_irq_save(flags);
>                 wrmsrl(HV_X64_MSR_VP_ASSIST_PAGE, 0);
> +               hvp->apic_assist &= ~1;
> +               local_irq_restore(flags);
> +       }
> 
>         if (hv_reenlightenment_cb == NULL)
>                 return 0;
> 
> This second version needs 3+ lines, so I prefer the one-line version. :-)

Those are two different things. The GPF_ZERO allocation makes sense on it's
own but it _cannot_ prevent the following scenario:

    cpu_init()
      if (!hvp)
      	 hvp = vmalloc(...., GFP_ZERO);
    ...

    hvp->apic_assist |= 1;

#1   cpu_die()
      if (....)
           wrmsrl(HV_X64_MSR_VP_ASSIST_PAGE, 0);

   ---> IPI
   	if (!(hvp->apic_assist & 1))	
	   wrmsr(APIC_EOI);    <- PATH not taken

#3   cpu is dead

    cpu_init()
       if (!hvp)
          hvp = vmalloc(....m, GFP_ZERO);  <- NOT TAKEN because hvp != NULL

So you have to come up with a better fairy tale why GFP_ZERO 'fixes' this.

Allocating hvp with GFP_ZERO makes sense on it's own so the allocated
memory has a defined state, but that's a different story.

The 3 liner patch above makes way more sense and you can spare the
local_irq_save/restore by moving the whole condition into the
irq_save/restore region above.

Thanks,

	tglx

RE: [PATCH] x86/hyper-v: Zero out the VP assist page to fix CPU offlining

[Dexuan Cui]

> From: Thomas Gleixner <tglx@linutronix.de>
> Sent: Thursday, July 18, 2019 12:01 AM
> ...
> Those are two different things. The GPF_ZERO allocation makes sense on its
> own but it _cannot_ prevent the following scenario:

Hi tglx,
The scenario can be prevented. 

The VP ASSIST PAGE is an "overlay" page (please see Hyper-V TLFS's Section
5.2.1 "GPA Overlay Pages", on page 38 of the spec). 

The spec can be downloaded from
https://docs.microsoft.com/en-us/virtualization/hyper-v-on-windows/reference/tlfs
(choose the v5.0c release)

Here is an excerpt of the section:

"
The hypervisor defines several special pages that "overlay" the guest's GPA
space. The hypercall code page is an example of an overlay page. Overlays
are addressed by Guest Physical Addresses (GPA) but are not included in the
normal GPA map maintained internally by the hypervisor. Conceptually,
they exist in a separate map that overlays the GPA map.

If a page within the GPA space is overlaid, any SPA page mapped to the
GPA page is effectively "obscured" and generally unreachable by the
virtual processor through processor memory accesses.
...
If an overlay page is disabled or is moved to a new location in the GPA
space, the underlying GPA page is "uncovered", and an existing
mapping becomes accessible to the guest. 
"

Here, SPA = System Physical Address = the final real physical address.

>     cpu_init()
>       if (!hvp)
>       	 hvp = vmalloc(...., GFP_ZERO);
>     ...
> 
>     hvp->apic_assist |= 1;

When the VP ASSIST PAGE feature is enabled and the hypervisor sets
the bit in hvp->apic_assist, the bit belongs to the special SPA page.

> #1   cpu_die()
>       if (....)
>            wrmsrl(HV_X64_MSR_VP_ASSIST_PAGE, 0);

After the VP ASSIST PAGE is disabled, hvp->apic_assist belongs to
the "normal" SPA page mapped to the GPA.

>    ---> IPI
>    	if (!(hvp->apic_assist & 1))
> 	   wrmsr(APIC_EOI);    <- PATH not taken

So, with the one-line patch or the three-line patch, here we're sure 
vp->apic_assist.bit0 must be 0.
 
> #3   cpu is dead
> 
>     cpu_init()
>        if (!hvp)
>           hvp = vmalloc(....m, GFP_ZERO);  <- NOT TAKEN because hvp !=
> NULL

It does not matter, because with the 1-line patch, the initial content of
the "normal" SPA page is filled with zeros; later, neither the hypervisor nor
the guest writes into the page, so the page always remains with zeros.

> So you have to come up with a better fairy tale why GFP_ZERO 'fixes' this.
> 
> Allocating hvp with GFP_ZERO makes sense on it's own so the allocated
> memory has a defined state, but that's a different story.
> 
> The 3 liner patch above makes way more sense and you can spare the
> local_irq_save/restore by moving the whole condition into the
> irq_save/restore region above.
> 
> 	tglx

The concept of the "overlay page" seems weird, and frankly speaking, 
I don't really understand why the Hyper-V guys invented it, but as far
as this patch here is concerned, I think the patch is safe and it can
indeed fix the CPU offlining issue I described.

Thanks,
-- Dexuan

RE: [PATCH] x86/hyper-v: Zero out the VP assist page to fix CPU offlining

[Thomas Gleixner]

On Thu, 18 Jul 2019, Dexuan Cui wrote:
> 
> The concept of the "overlay page" seems weird, and frankly speaking, 
> I don't really understand why the Hyper-V guys invented it, but as far
> as this patch here is concerned, I think the patch is safe and it can
> indeed fix the CPU offlining issue I described.

Then this needs some really good explanation and in the change log because
that's really obscure behaviour.

Thanks,

	tglx

RE: [PATCH] x86/hyper-v: Zero out the VP assist page to fix CPU offlining

[Dexuan Cui]

> From: Thomas Gleixner <tglx@linutronix.de>
> Sent: Thursday, July 18, 2019 12:56 AM
> To: Dexuan Cui <decui@microsoft.com>
> 
> On Thu, 18 Jul 2019, Dexuan Cui wrote:
> >
> > The concept of the "overlay page" seems weird, and frankly speaking,
> > I don't really understand why the Hyper-V guys invented it, but as far
> > as this patch here is concerned, I think the patch is safe and it can
> > indeed fix the CPU offlining issue I described.
> 
> Then this needs some really good explanation and in the change log because
> that's really obscure behaviour.
> 
> 	tglx

Agreed. I'll combine my replies into the changelog and post a v2 of
the one-line patch.

Thanks,
-- Dexuan