[PATCH] hyperv: root partition faults writing to VP ASSIST MSR PAGE

Subsystems: the rest, x86 architecture (32-bit and 64-bit)

STALE1851d

11 messages, 3 authors, 2021-07-21 · open the first message on its own page

[PATCH] hyperv: root partition faults writing to VP ASSIST MSR PAGE

From: Praveen Kumar <hidden>
Date: 2021-07-19 19:34:50 

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.

[    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
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()];
+
 	/*
-	 * 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);
-	}
+	if (hv_root_partition &&
+	    ms_hyperv.features & HV_MSR_APIC_ACCESS_AVAILABLE) {
+		union hv_x64_msr_hypercall_contents hypercall_msr;
+
+		rdmsrl(HV_X64_MSR_VP_ASSIST_PAGE, hypercall_msr.as_uint64);
+		/* remapping to root partition address space */
+		*hvp = memremap(hypercall_msr.guest_physical_address <<
+					HV_X64_MSR_VP_ASSIST_PAGE_ADDRESS_SHIFT,
+				PAGE_SIZE, MEMREMAP_WB);
+		WARN_ON(!(*hvp));
+	} else {
+		/*
+		 * 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.
+		 */
+		if (!*hvp)
+			*hvp = __vmalloc(PAGE_SIZE, GFP_KERNEL | __GFP_ZERO);
 
-	if (*hvp) {
-		u64 val;
+		if (*hvp) {
+			u64 val;
 
-		val = vmalloc_to_pfn(*hvp);
-		val = (val << HV_X64_MSR_VP_ASSIST_PAGE_ADDRESS_SHIFT) |
-			HV_X64_MSR_VP_ASSIST_PAGE_ENABLE;
+			val = vmalloc_to_pfn(*hvp);
+			val = (val << HV_X64_MSR_VP_ASSIST_PAGE_ADDRESS_SHIFT) |
+				HV_X64_MSR_VP_ASSIST_PAGE_ENABLE;
 
-		wrmsrl(HV_X64_MSR_VP_ASSIST_PAGE, val);
+			wrmsrl(HV_X64_MSR_VP_ASSIST_PAGE, val);
+		}
 	}
 
 	return 0;
@@ -170,8 +188,13 @@ static int hv_cpu_die(unsigned int cpu)
 
 	hv_common_cpu_die(cpu);
 
-	if (hv_vp_assist_page && hv_vp_assist_page[cpu])
-		wrmsrl(HV_X64_MSR_VP_ASSIST_PAGE, 0);
+	if (hv_vp_assist_page && hv_vp_assist_page[cpu]) {
+		if (hv_root_partition &&
+		    ms_hyperv.features & HV_MSR_APIC_ACCESS_AVAILABLE)
+			memunmap(hv_vp_assist_page[cpu]);
+		else
+			wrmsrl(HV_X64_MSR_VP_ASSIST_PAGE, 0);
+	}
 
 	if (hv_reenlightenment_cb == NULL)
 		return 0;
-- 
2.23.4

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 

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
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.

Re: [PATCH] hyperv: root partition faults writing to VP ASSIST MSR PAGE

From: Praveen Kumar <hidden>
Date: 2021-07-20 13:34:04 

On 20-07-2021 16:50, Wei Liu wrote:
The commit message needs a bit of work.

On Tue, Jul 20, 2021 at 12:21:26AM +0530, Praveen Kumar wrote:
quoted
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.
quoted
[    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.
Sure, will send updated commit log with these information in V2
quoted
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?
I think this is not required.

quoted
 	/*
-	 * 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.
Will add similar check for root partition as well. thanks.

quoted
+	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?
Yes, you are right. Will update this in v2. thanks.

Wei.
Regards,

~Praveen.





  

    
Re: [PATCH] hyperv: root partition faults writing to VP ASSIST MSR PAGE

    

      From: Wei Liu <wei.liu@kernel.org>

      Date: 2021-07-20 13:39:00 
    

  

  
On Tue, Jul 20, 2021 at 06:55:56PM +0530, Praveen Kumar wrote:
[...]
quoted
quoted
+	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?
Yes, you are right. Will update this in v2. thanks.
Please split adding this check to its own patch.

Ideally one patch only does one thing.

Wei.

quoted
Wei.
Regards,

~Praveen.





  

    
RE: [PATCH] hyperv: root partition faults writing to VP ASSIST MSR PAGE

    

      From: Michael Kelley <hidden>

      Date: 2021-07-20 16:26:52 
    

  

  
From: Wei Liu <wei.liu@kernel.org> Sent: Tuesday, July 20, 2021 6:35 AM
On Tue, Jul 20, 2021 at 06:55:56PM +0530, Praveen Kumar wrote:
[...]
quoted
quoted
quoted
+	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?
Yes, you are right. Will update this in v2. thanks.
Please split adding this check to its own patch.

Ideally one patch only does one thing.

Wei.
I was just looking around in the Hyper-V TLFS, and I didn't see
anywhere that the ability to set up a VP Assist page is dependent
on HV_MSR_APIC_ACCESS_AVAILABLE.  Or did I just miss it?

Maybe the VP Assist page is not useful for the APIC EOI optimization
Purposes if !HV_MSR_APIC_ACCESS_AVAILABLE, but the VP Assist
page has other uses, such as for nested enlightenments.  So I
wonder if the VP Assist page setup really should be gated on
HV_MSR_APIC_ACCESS_AVAILABLE.

Michael





  

    
Re: [PATCH] hyperv: root partition faults writing to VP ASSIST MSR PAGE

    

      From: Wei Liu <wei.liu@kernel.org>

      Date: 2021-07-20 16:34:04 
    

  

  
On Tue, Jul 20, 2021 at 04:20:44PM +0000, Michael Kelley wrote:
From: Wei Liu <wei.liu@kernel.org> Sent: Tuesday, July 20, 2021 6:35 AM
quoted
On Tue, Jul 20, 2021 at 06:55:56PM +0530, Praveen Kumar wrote:
[...]
quoted
quoted
quoted
+	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?
Yes, you are right. Will update this in v2. thanks.
Please split adding this check to its own patch.

Ideally one patch only does one thing.

Wei.
I was just looking around in the Hyper-V TLFS, and I didn't see
anywhere that the ability to set up a VP Assist page is dependent
on HV_MSR_APIC_ACCESS_AVAILABLE.  Or did I just miss it?
The feature bit Praveen used is wrong and should be fixed.

Per internal discussion this is gated by the AccessIntrCtrlRegs bit.

Wei.

Maybe the VP Assist page is not useful for the APIC EOI optimization
Purposes if !HV_MSR_APIC_ACCESS_AVAILABLE, but the VP Assist
page has other uses, such as for nested enlightenments.  So I
wonder if the VP Assist page setup really should be gated on
HV_MSR_APIC_ACCESS_AVAILABLE.

Michael





  

    
RE: [PATCH] hyperv: root partition faults writing to VP ASSIST MSR PAGE

    

      From: Michael Kelley <hidden>

      Date: 2021-07-21 04:10:45 
    

  

  
From: Wei Liu <wei.liu@kernel.org> Sent: Tuesday, July 20, 2021 9:29 AM
On Tue, Jul 20, 2021 at 04:20:44PM +0000, Michael Kelley wrote:
quoted
From: Wei Liu <wei.liu@kernel.org> Sent: Tuesday, July 20, 2021 6:35 AM
quoted
On Tue, Jul 20, 2021 at 06:55:56PM +0530, Praveen Kumar wrote:
[...]
quoted
quoted
quoted
+	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?
Yes, you are right. Will update this in v2. thanks.
Please split adding this check to its own patch.

Ideally one patch only does one thing.

Wei.
I was just looking around in the Hyper-V TLFS, and I didn't see
anywhere that the ability to set up a VP Assist page is dependent
on HV_MSR_APIC_ACCESS_AVAILABLE.  Or did I just miss it?
The feature bit Praveen used is wrong and should be fixed.

Per internal discussion this is gated by the AccessIntrCtrlRegs bit.

Wei.
The AccessIntrCtrlRegs bit *is* HV_MSR_APIC_ACCESS_AVAILABLE.
Both are defined as bit 4 of the Partition Privilege flags.  :-)   I don't
know why the names don't line up.   Even so, it's not clear to me that
AccessIntrCtrlRegs has any bearing on the VP Assist page.  I see this
description of AccessIntrCtrlRegs:

The partition has access to the synthetic MSRs associated with the
APIC (HV_X64_MSR_EOI, HV_X64_MSR_ICR and HV_X64_MSR_TPR).
If this flag is cleared, accesses to these MSRs results in a #GP fault if
the MSR intercept is not installed.

But maybe you have additional info that applies to the root
partition that is not in the TLFS.

Michael





  

    
Re: [PATCH] hyperv: root partition faults writing to VP ASSIST MSR PAGE

    

      From: Praveen Kumar <hidden>

      Date: 2021-07-21 07:17:45 
    

  

  
On 21-07-2021 09:40, Michael Kelley wrote:
From: Wei Liu <wei.liu@kernel.org> Sent: Tuesday, July 20, 2021 9:29 AM
quoted
On Tue, Jul 20, 2021 at 04:20:44PM +0000, Michael Kelley wrote:
quoted
From: Wei Liu <wei.liu@kernel.org> Sent: Tuesday, July 20, 2021 6:35 AM
quoted
On Tue, Jul 20, 2021 at 06:55:56PM +0530, Praveen Kumar wrote:
[...]
quoted
quoted
quoted
+	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?
Yes, you are right. Will update this in v2. thanks.
Please split adding this check to its own patch.

Ideally one patch only does one thing.

Wei.
I was just looking around in the Hyper-V TLFS, and I didn't see
anywhere that the ability to set up a VP Assist page is dependent
on HV_MSR_APIC_ACCESS_AVAILABLE.  Or did I just miss it?
The feature bit Praveen used is wrong and should be fixed.

Per internal discussion this is gated by the AccessIntrCtrlRegs bit.

Wei.
The AccessIntrCtrlRegs bit *is* HV_MSR_APIC_ACCESS_AVAILABLE.
Both are defined as bit 4 of the Partition Privilege flags.  :-)   I don't
know why the names don't line up.   Even so, it's not clear to me that
AccessIntrCtrlRegs has any bearing on the VP Assist page.  I see this
description of AccessIntrCtrlRegs:
Yup, what I understood as well, this is the one required one for Partition Privilege Flags (4th bit), however, cannot comment on the naming convention.

     5 /* Virtual APIC assist and VP assist page registers available */
     4 #define HV_MSR_APIC_ACCESS_AVAILABLE            BIT(4)

The partition has access to the synthetic MSRs associated with the
APIC (HV_X64_MSR_EOI, HV_X64_MSR_ICR and HV_X64_MSR_TPR).
If this flag is cleared, accesses to these MSRs results in a #GP fault if
the MSR intercept is not installed.
As per what I also understood from the TLFS doc,that we let partition access the MSR and do a fault.
However, the point is, does it make sense to allocate page for vp assist and perform action which is meant to fail when the flag is cleared ?

But maybe you have additional info that applies to the root
partition that is not in the TLFS.
As per what discussed internally and I understood, the root partition shares the vp assist page provided by hypervisor and its read only for Root kernel.

Michael
Regards,

~Praveen.





  

    
Re: [PATCH] hyperv: root partition faults writing to VP ASSIST MSR PAGE

    

      From: Wei Liu <wei.liu@kernel.org>

      Date: 2021-07-21 10:22:48 
    

  

  
On Wed, Jul 21, 2021 at 12:42:52PM +0530, Praveen Kumar wrote:
On 21-07-2021 09:40, Michael Kelley wrote:
quoted
From: Wei Liu <wei.liu@kernel.org> Sent: Tuesday, July 20, 2021 9:29 AM
quoted
On Tue, Jul 20, 2021 at 04:20:44PM +0000, Michael Kelley wrote:
quoted
From: Wei Liu <wei.liu@kernel.org> Sent: Tuesday, July 20, 2021 6:35 AM
quoted
On Tue, Jul 20, 2021 at 06:55:56PM +0530, Praveen Kumar wrote:
[...]
quoted
quoted
quoted
+	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?
Yes, you are right. Will update this in v2. thanks.
Please split adding this check to its own patch.

Ideally one patch only does one thing.

Wei.
I was just looking around in the Hyper-V TLFS, and I didn't see
anywhere that the ability to set up a VP Assist page is dependent
on HV_MSR_APIC_ACCESS_AVAILABLE.  Or did I just miss it?
The feature bit Praveen used is wrong and should be fixed.

Per internal discussion this is gated by the AccessIntrCtrlRegs bit.

Wei.
The AccessIntrCtrlRegs bit *is* HV_MSR_APIC_ACCESS_AVAILABLE.
Both are defined as bit 4 of the Partition Privilege flags.  :-)   I don't
know why the names don't line up.   Even so, it's not clear to me that
AccessIntrCtrlRegs has any bearing on the VP Assist page.  I see this
description of AccessIntrCtrlRegs:
Yup, what I understood as well, this is the one required one for Partition Privilege Flags (4th bit), however, cannot comment on the naming convention.

     5 /* Virtual APIC assist and VP assist page registers available */
     4 #define HV_MSR_APIC_ACCESS_AVAILABLE            BIT(4)
Urgh, okay. It is my fault for not reading the code closely. Sorry for
the confusion.

quoted
The partition has access to the synthetic MSRs associated with the
APIC (HV_X64_MSR_EOI, HV_X64_MSR_ICR and HV_X64_MSR_TPR).
If this flag is cleared, accesses to these MSRs results in a #GP fault if
the MSR intercept is not installed.
As per what I also understood from the TLFS doc,that we let partition
access the MSR and do a fault.  However, the point is, does it make
sense to allocate page for vp assist and perform action which is meant
to fail when the flag is cleared ?
Like Michael said, there are some other things that are not tied to that
particular bit. We should get more clarity on what gates what.  Perhaps
that privilege bit only controls access to the EOI assist bit and the
other things in the VP assist page are gated by other privilege bits.
This basically means we should setup the page when there is at least one
thing in that page can be used.

This is mostly an orthogonal issue from the one we want to fix. In
the interest of making progress we can drop the new check for now and
just add a root specific path for setting up and tearing down the VP
assist pages.

How does that sound?

Wei.

quoted
But maybe you have additional info that applies to the root
partition that is not in the TLFS.
As per what discussed internally and I understood, the root partition
shares the vp assist page provided by hypervisor and its read only for
Root kernel.

quoted
Michael
Regards,

~Praveen.





  

    
Re: [PATCH] hyperv: root partition faults writing to VP ASSIST MSR PAGE

    

      From: Praveen Kumar <hidden>

      Date: 2021-07-21 11:41:53 
    

  

  
On 21-07-2021 15:40, Wei Liu wrote:
On Wed, Jul 21, 2021 at 12:42:52PM +0530, Praveen Kumar wrote:
quoted
On 21-07-2021 09:40, Michael Kelley wrote:
quoted
From: Wei Liu <wei.liu@kernel.org> Sent: Tuesday, July 20, 2021 9:29 AM
quoted
On Tue, Jul 20, 2021 at 04:20:44PM +0000, Michael Kelley wrote:
quoted
From: Wei Liu <wei.liu@kernel.org> Sent: Tuesday, July 20, 2021 6:35 AM
quoted
On Tue, Jul 20, 2021 at 06:55:56PM +0530, Praveen Kumar wrote:
[...]
quoted
quoted
quoted
+	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?
Yes, you are right. Will update this in v2. thanks.
Please split adding this check to its own patch.

Ideally one patch only does one thing.

Wei.
I was just looking around in the Hyper-V TLFS, and I didn't see
anywhere that the ability to set up a VP Assist page is dependent
on HV_MSR_APIC_ACCESS_AVAILABLE.  Or did I just miss it?
The feature bit Praveen used is wrong and should be fixed.

Per internal discussion this is gated by the AccessIntrCtrlRegs bit.

Wei.
The AccessIntrCtrlRegs bit *is* HV_MSR_APIC_ACCESS_AVAILABLE.
Both are defined as bit 4 of the Partition Privilege flags.  :-)   I don't
know why the names don't line up.   Even so, it's not clear to me that
AccessIntrCtrlRegs has any bearing on the VP Assist page.  I see this
description of AccessIntrCtrlRegs:
Yup, what I understood as well, this is the one required one for Partition Privilege Flags (4th bit), however, cannot comment on the naming convention.

     5 /* Virtual APIC assist and VP assist page registers available */
     4 #define HV_MSR_APIC_ACCESS_AVAILABLE            BIT(4)
Urgh, okay. It is my fault for not reading the code closely. Sorry for
the confusion.

quoted
quoted
The partition has access to the synthetic MSRs associated with the
APIC (HV_X64_MSR_EOI, HV_X64_MSR_ICR and HV_X64_MSR_TPR).
If this flag is cleared, accesses to these MSRs results in a #GP fault if
the MSR intercept is not installed.
As per what I also understood from the TLFS doc,that we let partition
access the MSR and do a fault.  However, the point is, does it make
sense to allocate page for vp assist and perform action which is meant
to fail when the flag is cleared ?
Like Michael said, there are some other things that are not tied to that
particular bit. We should get more clarity on what gates what.  Perhaps
that privilege bit only controls access to the EOI assist bit and the
other things in the VP assist page are gated by other privilege bits.
This basically means we should setup the page when there is at least one
thing in that page can be used.

This is mostly an orthogonal issue from the one we want to fix. In
the interest of making progress we can drop the new check for now and
just add a root specific path for setting up and tearing down the VP
assist pages.

How does that sound?
Sounds good to me. Thanks Wei.

Wei.

quoted
quoted
But maybe you have additional info that applies to the root
partition that is not in the TLFS.
As per what discussed internally and I understood, the root partition
shares the vp assist page provided by hypervisor and its read only for
Root kernel.

quoted
Michael
Regards,

~Praveen.






  

    
RE: [PATCH] hyperv: root partition faults writing to VP ASSIST MSR PAGE

    

      From: Michael Kelley <hidden>

      Date: 2021-07-21 15:07:35 
    

  

  
From: Praveen Kumar <redacted> Sent: Wednesday, July 21, 2021 4:32 AM
On 21-07-2021 15:40, Wei Liu wrote:
quoted
On Wed, Jul 21, 2021 at 12:42:52PM +0530, Praveen Kumar wrote:
quoted
On 21-07-2021 09:40, Michael Kelley wrote:
quoted
From: Wei Liu <wei.liu@kernel.org> Sent: Tuesday, July 20, 2021 9:29 AM
quoted
On Tue, Jul 20, 2021 at 04:20:44PM +0000, Michael Kelley wrote:
quoted
From: Wei Liu <wei.liu@kernel.org> Sent: Tuesday, July 20, 2021 6:35 AM
quoted
On Tue, Jul 20, 2021 at 06:55:56PM +0530, Praveen Kumar wrote:
[...]
quoted
quoted
quoted
+	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?
Yes, you are right. Will update this in v2. thanks.
Please split adding this check to its own patch.

Ideally one patch only does one thing.

Wei.
I was just looking around in the Hyper-V TLFS, and I didn't see
anywhere that the ability to set up a VP Assist page is dependent
on HV_MSR_APIC_ACCESS_AVAILABLE.  Or did I just miss it?
The feature bit Praveen used is wrong and should be fixed.

Per internal discussion this is gated by the AccessIntrCtrlRegs bit.

Wei.
The AccessIntrCtrlRegs bit *is* HV_MSR_APIC_ACCESS_AVAILABLE.
Both are defined as bit 4 of the Partition Privilege flags.  :-)   I don't
know why the names don't line up.   Even so, it's not clear to me that
AccessIntrCtrlRegs has any bearing on the VP Assist page.  I see this
description of AccessIntrCtrlRegs:
Yup, what I understood as well, this is the one required one for Partition Privilege Flags (4th bit), however, cannot
comment on the naming convention.
quoted
quoted
     5 /* Virtual APIC assist and VP assist page registers available */
     4 #define HV_MSR_APIC_ACCESS_AVAILABLE            BIT(4)
Urgh, okay. It is my fault for not reading the code closely. Sorry for
the confusion.

quoted
quoted
The partition has access to the synthetic MSRs associated with the
APIC (HV_X64_MSR_EOI, HV_X64_MSR_ICR and HV_X64_MSR_TPR).
If this flag is cleared, accesses to these MSRs results in a #GP fault if
the MSR intercept is not installed.
As per what I also understood from the TLFS doc,that we let partition
access the MSR and do a fault.  However, the point is, does it make
sense to allocate page for vp assist and perform action which is meant
to fail when the flag is cleared ?
Like Michael said, there are some other things that are not tied to that
particular bit. We should get more clarity on what gates what.  Perhaps
that privilege bit only controls access to the EOI assist bit and the
other things in the VP assist page are gated by other privilege bits.
This basically means we should setup the page when there is at least one
thing in that page can be used.

This is mostly an orthogonal issue from the one we want to fix. In
the interest of making progress we can drop the new check for now and
just add a root specific path for setting up and tearing down the VP
assist pages.

How does that sound?
Sounds good to me. Thanks Wei.
Work for me as well.  Praveen -- The inconsistency in the name is
historical, and not something that needs to be changed now.  My
comment was just musing, not something actionable. :-)

Michael




    
  

  
    

      

        Keyboard shortcuts
      

      

        
          
hback out one level

          
jnext message in thread

          
kprevious message in thread

          
ldrill in

          
Escclose help / fold thread tree

          
?toggle this help