* [PATCH v2 0/2] Fix MSR access errors during kexec in root partition
@ 2022-10-27 9:57 Anirudh Rayabharam
2022-10-27 9:57 ` [PATCH v2 1/2] clocksource/drivers/hyperv: add data structure for reference TSC MSR Anirudh Rayabharam
2022-10-27 9:57 ` [PATCH v2 2/2] x86/hyperv: fix invalid writes to MSRs during root partition kexec Anirudh Rayabharam
0 siblings, 2 replies; 10+ messages in thread
From: Anirudh Rayabharam @ 2022-10-27 9:57 UTC (permalink / raw)
To: kys, haiyangz, sthemmin, wei.liu, decui, tglx, mingo, bp,
dave.hansen, x86, hpa, daniel.lezcano, Arnd Bergmann,
linux-hyperv, linux-kernel, linux-arch
Cc: stanislav.kinsburskiy, Anirudh Rayabharam, kumarpraveen, mail
Fixes a couple of MSR access errors seen during kexec in root partition
and opportunistically introduces a data structure for the reference TSC
MSR in order to simplify the code that updates that MSR.
New in v2:
1. Reverse the patch order as suggested by Michael. First introduce the
new structure for reference tsc MSR and then use it in the hv_cleanup
fix.
2. Use hv_{get,set}_register functions in hv_cleanup().
Anirudh Rayabharam (2):
clocksource/drivers/hyperv: add data structure for reference TSC MSR
x86/hyperv: fix invalid writes to MSRs during root partition kexec
arch/x86/hyperv/hv_init.c | 11 +++++++----
drivers/clocksource/hyperv_timer.c | 28 ++++++++++++++--------------
include/asm-generic/hyperv-tlfs.h | 9 +++++++++
3 files changed, 30 insertions(+), 18 deletions(-)
--
2.34.1
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 1/2] clocksource/drivers/hyperv: add data structure for reference TSC MSR
2022-10-27 9:57 [PATCH v2 0/2] Fix MSR access errors during kexec in root partition Anirudh Rayabharam
@ 2022-10-27 9:57 ` Anirudh Rayabharam
2022-10-27 13:42 ` Michael Kelley (LINUX)
2022-10-27 9:57 ` [PATCH v2 2/2] x86/hyperv: fix invalid writes to MSRs during root partition kexec Anirudh Rayabharam
1 sibling, 1 reply; 10+ messages in thread
From: Anirudh Rayabharam @ 2022-10-27 9:57 UTC (permalink / raw)
To: kys, haiyangz, sthemmin, wei.liu, decui, tglx, mingo, bp,
dave.hansen, x86, hpa, daniel.lezcano, Arnd Bergmann,
linux-hyperv, linux-kernel, linux-arch
Cc: stanislav.kinsburskiy, Anirudh Rayabharam, kumarpraveen, mail
Add a data structure to represent the reference TSC MSR similar to
other MSRs. This simplifies the code for updating the MSR.
Signed-off-by: Anirudh Rayabharam <anrayabh@linux.microsoft.com>
---
drivers/clocksource/hyperv_timer.c | 28 ++++++++++++++--------------
include/asm-generic/hyperv-tlfs.h | 9 +++++++++
2 files changed, 23 insertions(+), 14 deletions(-)
diff --git a/drivers/clocksource/hyperv_timer.c b/drivers/clocksource/hyperv_timer.c
index bb47610bbd1c..11332c82d1af 100644
--- a/drivers/clocksource/hyperv_timer.c
+++ b/drivers/clocksource/hyperv_timer.c
@@ -395,25 +395,25 @@ static u64 notrace read_hv_sched_clock_tsc(void)
static void suspend_hv_clock_tsc(struct clocksource *arg)
{
- u64 tsc_msr;
+ union hv_reference_tsc_msr tsc_msr;
/* Disable the TSC page */
- tsc_msr = hv_get_register(HV_REGISTER_REFERENCE_TSC);
- tsc_msr &= ~BIT_ULL(0);
- hv_set_register(HV_REGISTER_REFERENCE_TSC, tsc_msr);
+ tsc_msr.as_uint64 = hv_get_register(HV_REGISTER_REFERENCE_TSC);
+ tsc_msr.enable = 0;
+ hv_set_register(HV_REGISTER_REFERENCE_TSC, tsc_msr.as_uint64);
}
static void resume_hv_clock_tsc(struct clocksource *arg)
{
phys_addr_t phys_addr = virt_to_phys(&tsc_pg);
- u64 tsc_msr;
+ union hv_reference_tsc_msr tsc_msr;
/* Re-enable the TSC page */
- tsc_msr = hv_get_register(HV_REGISTER_REFERENCE_TSC);
- tsc_msr &= GENMASK_ULL(11, 0);
- tsc_msr |= BIT_ULL(0) | (u64)phys_addr;
- hv_set_register(HV_REGISTER_REFERENCE_TSC, tsc_msr);
+ tsc_msr.as_uint64 = hv_get_register(HV_REGISTER_REFERENCE_TSC);
+ tsc_msr.enable = 1;
+ tsc_msr.pfn = __phys_to_pfn(phys_addr);
+ hv_set_register(HV_REGISTER_REFERENCE_TSC, tsc_msr.as_uint64);
}
#ifdef HAVE_VDSO_CLOCKMODE_HVCLOCK
@@ -495,7 +495,7 @@ static __always_inline void hv_setup_sched_clock(void *sched_clock) {}
static bool __init hv_init_tsc_clocksource(void)
{
- u64 tsc_msr;
+ union hv_reference_tsc_msr tsc_msr;
phys_addr_t phys_addr;
if (!(ms_hyperv.features & HV_MSR_REFERENCE_TSC_AVAILABLE))
@@ -530,10 +530,10 @@ static bool __init hv_init_tsc_clocksource(void)
* (which already has at least the low 12 bits set to zero since
* it is page aligned). Also set the "enable" bit, which is bit 0.
*/
- tsc_msr = hv_get_register(HV_REGISTER_REFERENCE_TSC);
- tsc_msr &= GENMASK_ULL(11, 0);
- tsc_msr = tsc_msr | 0x1 | (u64)phys_addr;
- hv_set_register(HV_REGISTER_REFERENCE_TSC, tsc_msr);
+ tsc_msr.as_uint64 = hv_get_register(HV_REGISTER_REFERENCE_TSC);
+ tsc_msr.enable = 1;
+ tsc_msr.pfn = __phys_to_pfn(phys_addr);
+ hv_set_register(HV_REGISTER_REFERENCE_TSC, tsc_msr.as_uint64);
clocksource_register_hz(&hyperv_cs_tsc, NSEC_PER_SEC/100);
diff --git a/include/asm-generic/hyperv-tlfs.h b/include/asm-generic/hyperv-tlfs.h
index fdce7a4cfc6f..b17c6eeb9afa 100644
--- a/include/asm-generic/hyperv-tlfs.h
+++ b/include/asm-generic/hyperv-tlfs.h
@@ -102,6 +102,15 @@ struct ms_hyperv_tsc_page {
volatile s64 tsc_offset;
} __packed;
+union hv_reference_tsc_msr {
+ u64 as_uint64;
+ struct {
+ u64 enable:1;
+ u64 reserved:11;
+ u64 pfn:52;
+ } __packed;
+};
+
/*
* The guest OS needs to register the guest ID with the hypervisor.
* The guest ID is a 64 bit entity and the structure of this ID is
--
2.34.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v2 2/2] x86/hyperv: fix invalid writes to MSRs during root partition kexec
2022-10-27 9:57 [PATCH v2 0/2] Fix MSR access errors during kexec in root partition Anirudh Rayabharam
2022-10-27 9:57 ` [PATCH v2 1/2] clocksource/drivers/hyperv: add data structure for reference TSC MSR Anirudh Rayabharam
@ 2022-10-27 9:57 ` Anirudh Rayabharam
2022-10-27 13:44 ` Michael Kelley (LINUX)
1 sibling, 1 reply; 10+ messages in thread
From: Anirudh Rayabharam @ 2022-10-27 9:57 UTC (permalink / raw)
To: kys, haiyangz, sthemmin, wei.liu, decui, tglx, mingo, bp,
dave.hansen, x86, hpa, daniel.lezcano, Arnd Bergmann,
linux-hyperv, linux-kernel, linux-arch
Cc: stanislav.kinsburskiy, Anirudh Rayabharam, kumarpraveen, mail
hv_cleanup resets the hypercall page by setting the MSR to 0. However,
the root partition is not allowed to write to the GPA bits of the MSR.
Instead, it uses the hypercall page provided by the MSR. Similar is the
case with the reference TSC MSR.
Clear only the enable bit instead of zeroing the entire MSR to make
the code valid for root partition too.
Signed-off-by: Anirudh Rayabharam <anrayabh@linux.microsoft.com>
---
arch/x86/hyperv/hv_init.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
index 29774126e931..80fdfff9266c 100644
--- a/arch/x86/hyperv/hv_init.c
+++ b/arch/x86/hyperv/hv_init.c
@@ -537,6 +537,7 @@ void __init hyperv_init(void)
void hyperv_cleanup(void)
{
union hv_x64_msr_hypercall_contents hypercall_msr;
+ union hv_reference_tsc_msr tsc_msr;
unregister_syscore_ops(&hv_syscore_ops);
@@ -552,12 +553,14 @@ void hyperv_cleanup(void)
hv_hypercall_pg = NULL;
/* Reset the hypercall page */
- hypercall_msr.as_uint64 = 0;
- wrmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
+ hypercall_msr.as_uint64 = hv_get_register(HV_X64_MSR_HYPERCALL);
+ hypercall_msr.enable = 0;
+ hv_set_register(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
/* Reset the TSC page */
- hypercall_msr.as_uint64 = 0;
- wrmsrl(HV_X64_MSR_REFERENCE_TSC, hypercall_msr.as_uint64);
+ tsc_msr.as_uint64 = hv_get_register(HV_X64_MSR_REFERENCE_TSC);
+ tsc_msr.enable = 0;
+ hv_set_register(HV_X64_MSR_REFERENCE_TSC, tsc_msr.as_uint64);
}
void hyperv_report_panic(struct pt_regs *regs, long err, bool in_die)
--
2.34.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* RE: [PATCH v2 1/2] clocksource/drivers/hyperv: add data structure for reference TSC MSR
2022-10-27 9:57 ` [PATCH v2 1/2] clocksource/drivers/hyperv: add data structure for reference TSC MSR Anirudh Rayabharam
@ 2022-10-27 13:42 ` Michael Kelley (LINUX)
2022-11-02 20:33 ` Michael Kelley (LINUX)
0 siblings, 1 reply; 10+ messages in thread
From: Michael Kelley (LINUX) @ 2022-10-27 13:42 UTC (permalink / raw)
To: Anirudh Rayabharam, KY Srinivasan, Haiyang Zhang,
Stephen Hemminger, wei.liu, Dexuan Cui, tglx, mingo, bp,
dave.hansen, x86, hpa, daniel.lezcano, Arnd Bergmann,
linux-hyperv, linux-kernel, linux-arch
Cc: stanislav.kinsburskiy, kumarpraveen, mail
From: Anirudh Rayabharam <anrayabh@linux.microsoft.com> Sent: Thursday, October 27, 2022 2:57 AM
>
> Add a data structure to represent the reference TSC MSR similar to
> other MSRs. This simplifies the code for updating the MSR.
>
> Signed-off-by: Anirudh Rayabharam <anrayabh@linux.microsoft.com>
> ---
> drivers/clocksource/hyperv_timer.c | 28 ++++++++++++++--------------
> include/asm-generic/hyperv-tlfs.h | 9 +++++++++
> 2 files changed, 23 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/clocksource/hyperv_timer.c b/drivers/clocksource/hyperv_timer.c
> index bb47610bbd1c..11332c82d1af 100644
> --- a/drivers/clocksource/hyperv_timer.c
> +++ b/drivers/clocksource/hyperv_timer.c
> @@ -395,25 +395,25 @@ static u64 notrace read_hv_sched_clock_tsc(void)
>
> static void suspend_hv_clock_tsc(struct clocksource *arg)
> {
> - u64 tsc_msr;
> + union hv_reference_tsc_msr tsc_msr;
>
> /* Disable the TSC page */
> - tsc_msr = hv_get_register(HV_REGISTER_REFERENCE_TSC);
> - tsc_msr &= ~BIT_ULL(0);
> - hv_set_register(HV_REGISTER_REFERENCE_TSC, tsc_msr);
> + tsc_msr.as_uint64 = hv_get_register(HV_REGISTER_REFERENCE_TSC);
> + tsc_msr.enable = 0;
> + hv_set_register(HV_REGISTER_REFERENCE_TSC, tsc_msr.as_uint64);
> }
>
>
> static void resume_hv_clock_tsc(struct clocksource *arg)
> {
> phys_addr_t phys_addr = virt_to_phys(&tsc_pg);
> - u64 tsc_msr;
> + union hv_reference_tsc_msr tsc_msr;
>
> /* Re-enable the TSC page */
> - tsc_msr = hv_get_register(HV_REGISTER_REFERENCE_TSC);
> - tsc_msr &= GENMASK_ULL(11, 0);
> - tsc_msr |= BIT_ULL(0) | (u64)phys_addr;
> - hv_set_register(HV_REGISTER_REFERENCE_TSC, tsc_msr);
> + tsc_msr.as_uint64 = hv_get_register(HV_REGISTER_REFERENCE_TSC);
> + tsc_msr.enable = 1;
> + tsc_msr.pfn = __phys_to_pfn(phys_addr);
> + hv_set_register(HV_REGISTER_REFERENCE_TSC, tsc_msr.as_uint64);
> }
>
> #ifdef HAVE_VDSO_CLOCKMODE_HVCLOCK
> @@ -495,7 +495,7 @@ static __always_inline void hv_setup_sched_clock(void
> *sched_clock) {}
>
> static bool __init hv_init_tsc_clocksource(void)
> {
> - u64 tsc_msr;
> + union hv_reference_tsc_msr tsc_msr;
> phys_addr_t phys_addr;
>
> if (!(ms_hyperv.features & HV_MSR_REFERENCE_TSC_AVAILABLE))
> @@ -530,10 +530,10 @@ static bool __init hv_init_tsc_clocksource(void)
> * (which already has at least the low 12 bits set to zero since
> * it is page aligned). Also set the "enable" bit, which is bit 0.
> */
> - tsc_msr = hv_get_register(HV_REGISTER_REFERENCE_TSC);
> - tsc_msr &= GENMASK_ULL(11, 0);
> - tsc_msr = tsc_msr | 0x1 | (u64)phys_addr;
> - hv_set_register(HV_REGISTER_REFERENCE_TSC, tsc_msr);
> + tsc_msr.as_uint64 = hv_get_register(HV_REGISTER_REFERENCE_TSC);
> + tsc_msr.enable = 1;
> + tsc_msr.pfn = __phys_to_pfn(phys_addr);
> + hv_set_register(HV_REGISTER_REFERENCE_TSC, tsc_msr.as_uint64);
>
> clocksource_register_hz(&hyperv_cs_tsc, NSEC_PER_SEC/100);
>
> diff --git a/include/asm-generic/hyperv-tlfs.h b/include/asm-generic/hyperv-tlfs.h
> index fdce7a4cfc6f..b17c6eeb9afa 100644
> --- a/include/asm-generic/hyperv-tlfs.h
> +++ b/include/asm-generic/hyperv-tlfs.h
> @@ -102,6 +102,15 @@ struct ms_hyperv_tsc_page {
> volatile s64 tsc_offset;
> } __packed;
>
> +union hv_reference_tsc_msr {
> + u64 as_uint64;
> + struct {
> + u64 enable:1;
> + u64 reserved:11;
> + u64 pfn:52;
> + } __packed;
> +};
> +
> /*
> * The guest OS needs to register the guest ID with the hypervisor.
> * The guest ID is a 64 bit entity and the structure of this ID is
> --
> 2.34.1
Reviewed-by: Michael Kelley <mikelley@microsoft.com>
^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH v2 2/2] x86/hyperv: fix invalid writes to MSRs during root partition kexec
2022-10-27 9:57 ` [PATCH v2 2/2] x86/hyperv: fix invalid writes to MSRs during root partition kexec Anirudh Rayabharam
@ 2022-10-27 13:44 ` Michael Kelley (LINUX)
2022-10-27 19:16 ` Wei Liu
0 siblings, 1 reply; 10+ messages in thread
From: Michael Kelley (LINUX) @ 2022-10-27 13:44 UTC (permalink / raw)
To: Anirudh Rayabharam, KY Srinivasan, Haiyang Zhang,
Stephen Hemminger, wei.liu, Dexuan Cui, tglx, mingo, bp,
dave.hansen, x86, hpa, daniel.lezcano, Arnd Bergmann,
linux-hyperv, linux-kernel, linux-arch
Cc: stanislav.kinsburskiy, kumarpraveen, mail
From: Anirudh Rayabharam <anrayabh@linux.microsoft.com> Sent: Thursday, October 27, 2022 2:57 AM
>
> hv_cleanup resets the hypercall page by setting the MSR to 0. However,
The function name is hyperv_cleanup(), not hv_cleanup().
> the root partition is not allowed to write to the GPA bits of the MSR.
> Instead, it uses the hypercall page provided by the MSR. Similar is the
> case with the reference TSC MSR.
>
> Clear only the enable bit instead of zeroing the entire MSR to make
> the code valid for root partition too.
>
> Signed-off-by: Anirudh Rayabharam <anrayabh@linux.microsoft.com>
> ---
> arch/x86/hyperv/hv_init.c | 11 +++++++----
> 1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
> index 29774126e931..80fdfff9266c 100644
> --- a/arch/x86/hyperv/hv_init.c
> +++ b/arch/x86/hyperv/hv_init.c
> @@ -537,6 +537,7 @@ void __init hyperv_init(void)
> void hyperv_cleanup(void)
> {
> union hv_x64_msr_hypercall_contents hypercall_msr;
> + union hv_reference_tsc_msr tsc_msr;
>
> unregister_syscore_ops(&hv_syscore_ops);
>
> @@ -552,12 +553,14 @@ void hyperv_cleanup(void)
> hv_hypercall_pg = NULL;
>
> /* Reset the hypercall page */
> - hypercall_msr.as_uint64 = 0;
> - wrmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
> + hypercall_msr.as_uint64 = hv_get_register(HV_X64_MSR_HYPERCALL);
> + hypercall_msr.enable = 0;
> + hv_set_register(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
>
> /* Reset the TSC page */
> - hypercall_msr.as_uint64 = 0;
> - wrmsrl(HV_X64_MSR_REFERENCE_TSC, hypercall_msr.as_uint64);
> + tsc_msr.as_uint64 = hv_get_register(HV_X64_MSR_REFERENCE_TSC);
> + tsc_msr.enable = 0;
> + hv_set_register(HV_X64_MSR_REFERENCE_TSC, tsc_msr.as_uint64);
> }
>
> void hyperv_report_panic(struct pt_regs *regs, long err, bool in_die)
> --
> 2.34.1
Modulo the nit in the commit message,
Reviewed-by: Michael Kelley <mikelley@microsoft.com>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/2] x86/hyperv: fix invalid writes to MSRs during root partition kexec
2022-10-27 13:44 ` Michael Kelley (LINUX)
@ 2022-10-27 19:16 ` Wei Liu
2022-10-28 12:34 ` Anirudh Rayabharam
0 siblings, 1 reply; 10+ messages in thread
From: Wei Liu @ 2022-10-27 19:16 UTC (permalink / raw)
To: Michael Kelley (LINUX)
Cc: Anirudh Rayabharam, KY Srinivasan, Haiyang Zhang,
Stephen Hemminger, wei.liu, Dexuan Cui, tglx, mingo, bp,
dave.hansen, x86, hpa, daniel.lezcano, Arnd Bergmann,
linux-hyperv, linux-kernel, linux-arch, stanislav.kinsburskiy,
kumarpraveen, mail
On Thu, Oct 27, 2022 at 01:44:40PM +0000, Michael Kelley (LINUX) wrote:
> From: Anirudh Rayabharam <anrayabh@linux.microsoft.com> Sent: Thursday, October 27, 2022 2:57 AM
> >
> > hv_cleanup resets the hypercall page by setting the MSR to 0. However,
>
> The function name is hyperv_cleanup(), not hv_cleanup().
I fixed this and applied both patches to hyperv-fixes. Thank you both.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/2] x86/hyperv: fix invalid writes to MSRs during root partition kexec
2022-10-27 19:16 ` Wei Liu
@ 2022-10-28 12:34 ` Anirudh Rayabharam
0 siblings, 0 replies; 10+ messages in thread
From: Anirudh Rayabharam @ 2022-10-28 12:34 UTC (permalink / raw)
To: Wei Liu
Cc: Michael Kelley (LINUX),
KY Srinivasan, Haiyang Zhang, Stephen Hemminger, Dexuan Cui,
tglx, mingo, bp, dave.hansen, x86, hpa, daniel.lezcano,
Arnd Bergmann, linux-hyperv, linux-kernel, linux-arch,
stanislav.kinsburskiy, kumarpraveen, mail
On Thu, Oct 27, 2022 at 07:16:49PM +0000, Wei Liu wrote:
> On Thu, Oct 27, 2022 at 01:44:40PM +0000, Michael Kelley (LINUX) wrote:
> > From: Anirudh Rayabharam <anrayabh@linux.microsoft.com> Sent: Thursday, October 27, 2022 2:57 AM
> > >
> > > hv_cleanup resets the hypercall page by setting the MSR to 0. However,
> >
> > The function name is hyperv_cleanup(), not hv_cleanup().
>
> I fixed this and applied both patches to hyperv-fixes. Thank you both.
Thank you!
Anirudh.
^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH v2 1/2] clocksource/drivers/hyperv: add data structure for reference TSC MSR
2022-10-27 13:42 ` Michael Kelley (LINUX)
@ 2022-11-02 20:33 ` Michael Kelley (LINUX)
2022-11-03 14:24 ` Wei Liu
0 siblings, 1 reply; 10+ messages in thread
From: Michael Kelley (LINUX) @ 2022-11-02 20:33 UTC (permalink / raw)
To: Michael Kelley (LINUX),
Anirudh Rayabharam, KY Srinivasan, Haiyang Zhang,
Stephen Hemminger, wei.liu, Dexuan Cui, tglx, mingo, bp,
dave.hansen, x86, hpa, daniel.lezcano, Arnd Bergmann,
linux-hyperv, linux-kernel, linux-arch
Cc: stanislav.kinsburskiy, kumarpraveen, mail
From: Michael Kelley (LINUX) <mikelley@microsoft.com> Sent: Thursday, October 27, 2022 6:43 AM
> From: Anirudh Rayabharam <anrayabh@linux.microsoft.com> Sent: Thursday,
> October 27, 2022 2:57 AM
> >
> > Add a data structure to represent the reference TSC MSR similar to
> > other MSRs. This simplifies the code for updating the MSR.
> >
> > Signed-off-by: Anirudh Rayabharam <anrayabh@linux.microsoft.com>
> > ---
> > drivers/clocksource/hyperv_timer.c | 28 ++++++++++++++--------------
> > include/asm-generic/hyperv-tlfs.h | 9 +++++++++
> > 2 files changed, 23 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/clocksource/hyperv_timer.c
> b/drivers/clocksource/hyperv_timer.c
> > index bb47610bbd1c..11332c82d1af 100644
> > --- a/drivers/clocksource/hyperv_timer.c
> > +++ b/drivers/clocksource/hyperv_timer.c
> > @@ -395,25 +395,25 @@ static u64 notrace read_hv_sched_clock_tsc(void)
> >
> > static void suspend_hv_clock_tsc(struct clocksource *arg)
> > {
> > - u64 tsc_msr;
> > + union hv_reference_tsc_msr tsc_msr;
> >
> > /* Disable the TSC page */
> > - tsc_msr = hv_get_register(HV_REGISTER_REFERENCE_TSC);
> > - tsc_msr &= ~BIT_ULL(0);
> > - hv_set_register(HV_REGISTER_REFERENCE_TSC, tsc_msr);
> > + tsc_msr.as_uint64 = hv_get_register(HV_REGISTER_REFERENCE_TSC);
> > + tsc_msr.enable = 0;
> > + hv_set_register(HV_REGISTER_REFERENCE_TSC, tsc_msr.as_uint64);
> > }
> >
> >
> > static void resume_hv_clock_tsc(struct clocksource *arg)
> > {
> > phys_addr_t phys_addr = virt_to_phys(&tsc_pg);
> > - u64 tsc_msr;
> > + union hv_reference_tsc_msr tsc_msr;
> >
> > /* Re-enable the TSC page */
> > - tsc_msr = hv_get_register(HV_REGISTER_REFERENCE_TSC);
> > - tsc_msr &= GENMASK_ULL(11, 0);
> > - tsc_msr |= BIT_ULL(0) | (u64)phys_addr;
> > - hv_set_register(HV_REGISTER_REFERENCE_TSC, tsc_msr);
> > + tsc_msr.as_uint64 = hv_get_register(HV_REGISTER_REFERENCE_TSC);
> > + tsc_msr.enable = 1;
> > + tsc_msr.pfn = __phys_to_pfn(phys_addr);
My previous review missed a problem here (and in the similar line below).
__phys_to_pfn() will return a PFN based on the guest page size, which might
be different from Hyper-V's page size that is always 4K. This needs to be a
Hyper-V PFN, and we have virt_to_hvpfn() available to do just that, assuming
that function is safe to use here and in the case below.
Michael
> > + hv_set_register(HV_REGISTER_REFERENCE_TSC, tsc_msr.as_uint64);
> > }
> >
> > #ifdef HAVE_VDSO_CLOCKMODE_HVCLOCK
> > @@ -495,7 +495,7 @@ static __always_inline void hv_setup_sched_clock(void
> > *sched_clock) {}
> >
> > static bool __init hv_init_tsc_clocksource(void)
> > {
> > - u64 tsc_msr;
> > + union hv_reference_tsc_msr tsc_msr;
> > phys_addr_t phys_addr;
> >
> > if (!(ms_hyperv.features & HV_MSR_REFERENCE_TSC_AVAILABLE))
> > @@ -530,10 +530,10 @@ static bool __init hv_init_tsc_clocksource(void)
> > * (which already has at least the low 12 bits set to zero since
> > * it is page aligned). Also set the "enable" bit, which is bit 0.
> > */
> > - tsc_msr = hv_get_register(HV_REGISTER_REFERENCE_TSC);
> > - tsc_msr &= GENMASK_ULL(11, 0);
> > - tsc_msr = tsc_msr | 0x1 | (u64)phys_addr;
> > - hv_set_register(HV_REGISTER_REFERENCE_TSC, tsc_msr);
> > + tsc_msr.as_uint64 = hv_get_register(HV_REGISTER_REFERENCE_TSC);
> > + tsc_msr.enable = 1;
> > + tsc_msr.pfn = __phys_to_pfn(phys_addr);
> > + hv_set_register(HV_REGISTER_REFERENCE_TSC, tsc_msr.as_uint64);
> >
> > clocksource_register_hz(&hyperv_cs_tsc, NSEC_PER_SEC/100);
> >
> > diff --git a/include/asm-generic/hyperv-tlfs.h b/include/asm-generic/hyperv-tlfs.h
> > index fdce7a4cfc6f..b17c6eeb9afa 100644
> > --- a/include/asm-generic/hyperv-tlfs.h
> > +++ b/include/asm-generic/hyperv-tlfs.h
> > @@ -102,6 +102,15 @@ struct ms_hyperv_tsc_page {
> > volatile s64 tsc_offset;
> > } __packed;
> >
> > +union hv_reference_tsc_msr {
> > + u64 as_uint64;
> > + struct {
> > + u64 enable:1;
> > + u64 reserved:11;
> > + u64 pfn:52;
> > + } __packed;
> > +};
> > +
> > /*
> > * The guest OS needs to register the guest ID with the hypervisor.
> > * The guest ID is a 64 bit entity and the structure of this ID is
> > --
> > 2.34.1
>
> Reviewed-by: Michael Kelley <mikelley@microsoft.com>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/2] clocksource/drivers/hyperv: add data structure for reference TSC MSR
2022-11-02 20:33 ` Michael Kelley (LINUX)
@ 2022-11-03 14:24 ` Wei Liu
2022-11-03 15:30 ` Anirudh Rayabharam
0 siblings, 1 reply; 10+ messages in thread
From: Wei Liu @ 2022-11-03 14:24 UTC (permalink / raw)
To: Michael Kelley (LINUX)
Cc: Anirudh Rayabharam, KY Srinivasan, Haiyang Zhang,
Stephen Hemminger, wei.liu, Dexuan Cui, tglx, mingo, bp,
dave.hansen, x86, hpa, daniel.lezcano, Arnd Bergmann,
linux-hyperv, linux-kernel, linux-arch, stanislav.kinsburskiy,
kumarpraveen, mail
On Wed, Nov 02, 2022 at 08:33:31PM +0000, Michael Kelley (LINUX) wrote:
> From: Michael Kelley (LINUX) <mikelley@microsoft.com> Sent: Thursday, October 27, 2022 6:43 AM
> > From: Anirudh Rayabharam <anrayabh@linux.microsoft.com> Sent: Thursday,
> > October 27, 2022 2:57 AM
> > >
> > > Add a data structure to represent the reference TSC MSR similar to
> > > other MSRs. This simplifies the code for updating the MSR.
> > >
> > > Signed-off-by: Anirudh Rayabharam <anrayabh@linux.microsoft.com>
> > > ---
> > > drivers/clocksource/hyperv_timer.c | 28 ++++++++++++++--------------
> > > include/asm-generic/hyperv-tlfs.h | 9 +++++++++
> > > 2 files changed, 23 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/drivers/clocksource/hyperv_timer.c
> > b/drivers/clocksource/hyperv_timer.c
> > > index bb47610bbd1c..11332c82d1af 100644
> > > --- a/drivers/clocksource/hyperv_timer.c
> > > +++ b/drivers/clocksource/hyperv_timer.c
> > > @@ -395,25 +395,25 @@ static u64 notrace read_hv_sched_clock_tsc(void)
> > >
> > > static void suspend_hv_clock_tsc(struct clocksource *arg)
> > > {
> > > - u64 tsc_msr;
> > > + union hv_reference_tsc_msr tsc_msr;
> > >
> > > /* Disable the TSC page */
> > > - tsc_msr = hv_get_register(HV_REGISTER_REFERENCE_TSC);
> > > - tsc_msr &= ~BIT_ULL(0);
> > > - hv_set_register(HV_REGISTER_REFERENCE_TSC, tsc_msr);
> > > + tsc_msr.as_uint64 = hv_get_register(HV_REGISTER_REFERENCE_TSC);
> > > + tsc_msr.enable = 0;
> > > + hv_set_register(HV_REGISTER_REFERENCE_TSC, tsc_msr.as_uint64);
> > > }
> > >
> > >
> > > static void resume_hv_clock_tsc(struct clocksource *arg)
> > > {
> > > phys_addr_t phys_addr = virt_to_phys(&tsc_pg);
> > > - u64 tsc_msr;
> > > + union hv_reference_tsc_msr tsc_msr;
> > >
> > > /* Re-enable the TSC page */
> > > - tsc_msr = hv_get_register(HV_REGISTER_REFERENCE_TSC);
> > > - tsc_msr &= GENMASK_ULL(11, 0);
> > > - tsc_msr |= BIT_ULL(0) | (u64)phys_addr;
> > > - hv_set_register(HV_REGISTER_REFERENCE_TSC, tsc_msr);
> > > + tsc_msr.as_uint64 = hv_get_register(HV_REGISTER_REFERENCE_TSC);
> > > + tsc_msr.enable = 1;
> > > + tsc_msr.pfn = __phys_to_pfn(phys_addr);
>
> My previous review missed a problem here (and in the similar line below).
> __phys_to_pfn() will return a PFN based on the guest page size, which might
> be different from Hyper-V's page size that is always 4K. This needs to be a
> Hyper-V PFN, and we have virt_to_hvpfn() available to do just that, assuming
> that function is safe to use here and in the case below.
Anirudh, please take a look.
I'm holding off sending hyperv-fixes to Linus for now.
Thanks,
Wei.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/2] clocksource/drivers/hyperv: add data structure for reference TSC MSR
2022-11-03 14:24 ` Wei Liu
@ 2022-11-03 15:30 ` Anirudh Rayabharam
0 siblings, 0 replies; 10+ messages in thread
From: Anirudh Rayabharam @ 2022-11-03 15:30 UTC (permalink / raw)
To: Wei Liu
Cc: Michael Kelley (LINUX),
KY Srinivasan, Haiyang Zhang, Stephen Hemminger, Dexuan Cui,
tglx, mingo, bp, dave.hansen, x86, hpa, daniel.lezcano,
Arnd Bergmann, linux-hyperv, linux-kernel, linux-arch,
stanislav.kinsburskiy, kumarpraveen, mail
On Thu, Nov 03, 2022 at 02:24:05PM +0000, Wei Liu wrote:
> On Wed, Nov 02, 2022 at 08:33:31PM +0000, Michael Kelley (LINUX) wrote:
> > From: Michael Kelley (LINUX) <mikelley@microsoft.com> Sent: Thursday, October 27, 2022 6:43 AM
> > > From: Anirudh Rayabharam <anrayabh@linux.microsoft.com> Sent: Thursday,
> > > October 27, 2022 2:57 AM
> > > >
> > > > Add a data structure to represent the reference TSC MSR similar to
> > > > other MSRs. This simplifies the code for updating the MSR.
> > > >
> > > > Signed-off-by: Anirudh Rayabharam <anrayabh@linux.microsoft.com>
> > > > ---
> > > > drivers/clocksource/hyperv_timer.c | 28 ++++++++++++++--------------
> > > > include/asm-generic/hyperv-tlfs.h | 9 +++++++++
> > > > 2 files changed, 23 insertions(+), 14 deletions(-)
> > > >
> > > > diff --git a/drivers/clocksource/hyperv_timer.c
> > > b/drivers/clocksource/hyperv_timer.c
> > > > index bb47610bbd1c..11332c82d1af 100644
> > > > --- a/drivers/clocksource/hyperv_timer.c
> > > > +++ b/drivers/clocksource/hyperv_timer.c
> > > > @@ -395,25 +395,25 @@ static u64 notrace read_hv_sched_clock_tsc(void)
> > > >
> > > > static void suspend_hv_clock_tsc(struct clocksource *arg)
> > > > {
> > > > - u64 tsc_msr;
> > > > + union hv_reference_tsc_msr tsc_msr;
> > > >
> > > > /* Disable the TSC page */
> > > > - tsc_msr = hv_get_register(HV_REGISTER_REFERENCE_TSC);
> > > > - tsc_msr &= ~BIT_ULL(0);
> > > > - hv_set_register(HV_REGISTER_REFERENCE_TSC, tsc_msr);
> > > > + tsc_msr.as_uint64 = hv_get_register(HV_REGISTER_REFERENCE_TSC);
> > > > + tsc_msr.enable = 0;
> > > > + hv_set_register(HV_REGISTER_REFERENCE_TSC, tsc_msr.as_uint64);
> > > > }
> > > >
> > > >
> > > > static void resume_hv_clock_tsc(struct clocksource *arg)
> > > > {
> > > > phys_addr_t phys_addr = virt_to_phys(&tsc_pg);
> > > > - u64 tsc_msr;
> > > > + union hv_reference_tsc_msr tsc_msr;
> > > >
> > > > /* Re-enable the TSC page */
> > > > - tsc_msr = hv_get_register(HV_REGISTER_REFERENCE_TSC);
> > > > - tsc_msr &= GENMASK_ULL(11, 0);
> > > > - tsc_msr |= BIT_ULL(0) | (u64)phys_addr;
> > > > - hv_set_register(HV_REGISTER_REFERENCE_TSC, tsc_msr);
> > > > + tsc_msr.as_uint64 = hv_get_register(HV_REGISTER_REFERENCE_TSC);
> > > > + tsc_msr.enable = 1;
> > > > + tsc_msr.pfn = __phys_to_pfn(phys_addr);
> >
> > My previous review missed a problem here (and in the similar line below).
> > __phys_to_pfn() will return a PFN based on the guest page size, which might
> > be different from Hyper-V's page size that is always 4K. This needs to be a
> > Hyper-V PFN, and we have virt_to_hvpfn() available to do just that, assuming
> > that function is safe to use here and in the case below.
>
> Anirudh, please take a look.
Just sent a fix for this using HVPFN_DOWN() which looks safe to use
here.
Thanks,
Anirudh.
>
> I'm holding off sending hyperv-fixes to Linus for now.
>
> Thanks,
> Wei.
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2022-11-03 15:31 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-10-27 9:57 [PATCH v2 0/2] Fix MSR access errors during kexec in root partition Anirudh Rayabharam
2022-10-27 9:57 ` [PATCH v2 1/2] clocksource/drivers/hyperv: add data structure for reference TSC MSR Anirudh Rayabharam
2022-10-27 13:42 ` Michael Kelley (LINUX)
2022-11-02 20:33 ` Michael Kelley (LINUX)
2022-11-03 14:24 ` Wei Liu
2022-11-03 15:30 ` Anirudh Rayabharam
2022-10-27 9:57 ` [PATCH v2 2/2] x86/hyperv: fix invalid writes to MSRs during root partition kexec Anirudh Rayabharam
2022-10-27 13:44 ` Michael Kelley (LINUX)
2022-10-27 19:16 ` Wei Liu
2022-10-28 12:34 ` Anirudh Rayabharam
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.