Re: [PATCH] hyperv: root partition faults writing to VP ASSIST MSR PAGE
From: Wei Liu <wei.liu@kernel.org>
Date: 2021-07-20 11:21:13
Also in:
lkml
The commit message needs a bit of work. On Tue, Jul 20, 2021 at 12:21:26AM +0530, Praveen Kumar wrote:
The root partition is not supposed to write to VP ASSIST PAGE as this MSR is specific to Guest VP, and thus below stack is observed.
Yes, root kernel is supposed to write to this MSR, but that's not because this MSR is specific to children (guest) partitions. It is just that for root this is read-only. You should mention VP assist pages for root are pre-determined by the hypervisor. Root kernel is not allowed to change them to different locations.
[ 2.778197] unchecked MSR access error: WRMSR to 0x40000073 (tried to write 0x0000000145ac5001) at rIP: 0xffffffff810c1084 (native_write_msr+0x4/0x30) [ 2.784867] Call Trace: [ 2.791507] hv_cpu_init+0xf1/0x1c0 [ 2.798144] ? hyperv_report_panic+0xd0/0xd0 [ 2.804806] cpuhp_invoke_callback+0x11a/0x440 [ 2.811465] ? hv_resume+0x90/0x90 [ 2.818137] cpuhp_issue_call+0x126/0x130 [ 2.824782] __cpuhp_setup_state_cpuslocked+0x102/0x2b0 [ 2.831427] ? hyperv_report_panic+0xd0/0xd0 [ 2.838075] ? hyperv_report_panic+0xd0/0xd0 [ 2.844723] ? hv_resume+0x90/0x90 [ 2.851375] __cpuhp_setup_state+0x3d/0x90 [ 2.858030] hyperv_init+0x14e/0x410 [ 2.864689] ? enable_IR_x2apic+0x190/0x1a0 [ 2.871349] apic_intr_mode_init+0x8b/0x100 [ 2.878017] x86_late_time_init+0x20/0x30 [ 2.884675] start_kernel+0x459/0x4fb [ 2.891329] secondary_startup_64_no_verify+0xb0/0xbb Root partition actually shares the VP ASSIST page with hypervisor, and
So do children partitions. This page is by design shared between hypervisor and any partitions that use it.
quoted hunk ↗ jump to hunk
thus as a solution, this patch memremaps the memory from hypervisor during hv_cpu_init and unmaps during hv_cpu_die calls. Further, this patch also resolve some error handling and checkpatch errors Signed-off-by: Praveen Kumar <redacted> --- arch/x86/hyperv/hv_init.c | 57 +++++++++++++++++++++++++++------------ 1 file changed, 40 insertions(+), 17 deletions(-)diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c index 6f247e7e07eb..292b17e0b173 100644 --- a/arch/x86/hyperv/hv_init.c +++ b/arch/x86/hyperv/hv_init.c@@ -44,7 +44,7 @@ EXPORT_SYMBOL_GPL(hv_vp_assist_page); static int hv_cpu_init(unsigned int cpu) { - struct hv_vp_assist_page **hvp = &hv_vp_assist_page[smp_processor_id()]; + struct hv_vp_assist_page **hvp = NULL; int ret; ret = hv_common_cpu_init(cpu);@@ -54,25 +54,43 @@ static int hv_cpu_init(unsigned int cpu) if (!hv_vp_assist_page) return 0; + hvp = &hv_vp_assist_page[smp_processor_id()]; +
Why is this needed? Is it because of checkpatch?
/*
- * The VP ASSIST PAGE is an "overlay" page (see Hyper-V TLFS's Section
- * 5.2.1 "GPA Overlay Pages"). Here it must be zeroed out to make sure
- * we always write the EOI MSR in hv_apic_eoi_write() *after* the
- * EOI optimization is disabled in hv_cpu_die(), otherwise a CPU may
- * not be stopped in the case of CPU offlining and the VM will hang.
+ * For Root partition we need to map the hypervisor VP ASSIST PAGE
+ * instead of allocating a new page.
*/
- if (!*hvp) {
- *hvp = __vmalloc(PAGE_SIZE, GFP_KERNEL | __GFP_ZERO);
- }This path suggests that it is possible to enter this function with *hvp already set. The new path for root is missing this check.
+ if (hv_root_partition &&
+ ms_hyperv.features & HV_MSR_APIC_ACCESS_AVAILABLE) {Is HV_MSR_APIC_ACCESS_AVAILABLE a root only flag? Shouldn't non-root kernel check this too? Wei.