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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
From: Anirudh Rayabharam <[email protected]> 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 <[email protected]>
> ---
> 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 <[email protected]>
From: Anirudh Rayabharam <[email protected]> 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 <[email protected]>
> ---
> 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 <[email protected]>
On Thu, Oct 27, 2022 at 01:44:40PM +0000, Michael Kelley (LINUX) wrote:
> From: Anirudh Rayabharam <[email protected]> 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.
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 <[email protected]> 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.
From: Michael Kelley (LINUX) <[email protected]> Sent: Thursday, October 27, 2022 6:43 AM
> From: Anirudh Rayabharam <[email protected]> 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 <[email protected]>
> > ---
> > 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 <[email protected]>
On Wed, Nov 02, 2022 at 08:33:31PM +0000, Michael Kelley (LINUX) wrote:
> From: Michael Kelley (LINUX) <[email protected]> Sent: Thursday, October 27, 2022 6:43 AM
> > From: Anirudh Rayabharam <[email protected]> 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 <[email protected]>
> > > ---
> > > 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.
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) <[email protected]> Sent: Thursday, October 27, 2022 6:43 AM
> > > From: Anirudh Rayabharam <[email protected]> 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 <[email protected]>
> > > > ---
> > > > 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.