[v2] Fix MSR access errors during kexec in root partition

[PATCH v2 1/2] clocksource/drivers/hyperv: add data structure for reference TSC MSR

Posted by Anirudh Rayabharam 3 years, 5 months ago

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

RE: [PATCH v2 1/2] clocksource/drivers/hyperv: add data structure for reference TSC MSR

Posted by Michael Kelley (LINUX) 3 years, 5 months ago

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>

RE: [PATCH v2 1/2] clocksource/drivers/hyperv: add data structure for reference TSC MSR

Posted by Michael Kelley (LINUX) 3 years, 5 months ago

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>

Re: [PATCH v2 1/2] clocksource/drivers/hyperv: add data structure for reference TSC MSR

Posted by Wei Liu 3 years, 5 months ago

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.

Re: [PATCH v2 1/2] clocksource/drivers/hyperv: add data structure for reference TSC MSR

Posted by Anirudh Rayabharam 3 years, 5 months ago

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.

[PATCH v2 1/2] clocksource/drivers/hyperv: add data structure for reference TSC MSR
[PATCH v2 2/2] x86/hyperv: fix invalid writes to MSRs during root partition kexec