[2/4] drivers/clocksource/hyper-v: Introduce TSC MSR register structure

  From: Stanislav Kinsburskiy <stanislav.kinsburskiy@gmail.com>

And rework the code to use it instead of the physical address.
This is a cleanup and precursor patch for upcoming support for TSC page
mapping into hyper-v root partition.

Signed-off-by: Stanislav Kinsburskiy <stanislav.kinsburskiy@gmail.com>
CC: "K. Y. Srinivasan" <kys@microsoft.com>
CC: Haiyang Zhang <haiyangz@microsoft.com>
CC: Wei Liu <wei.liu@kernel.org>
CC: Dexuan Cui <decui@microsoft.com>
CC: Daniel Lezcano <daniel.lezcano@linaro.org>
CC: Thomas Gleixner <tglx@linutronix.de>
CC: linux-hyperv@vger.kernel.org
CC: linux-kernel@vger.kernel.org
---
 drivers/clocksource/hyperv_timer.c |   14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)
  

Please update the patch title since this doesn't introduce the TSC MSR
register structure.

Thanks,
Anirudh.

On Tue, Nov 01, 2022 at 05:31:09PM +0000, Stanislav Kinsburskii wrote:
> From: Stanislav Kinsburskiy <stanislav.kinsburskiy@gmail.com>
> 
> And rework the code to use it instead of the physical address.
> This is a cleanup and precursor patch for upcoming support for TSC page
> mapping into hyper-v root partition.
> 
> Signed-off-by: Stanislav Kinsburskiy <stanislav.kinsburskiy@gmail.com>
> CC: "K. Y. Srinivasan" <kys@microsoft.com>
> CC: Haiyang Zhang <haiyangz@microsoft.com>
> CC: Wei Liu <wei.liu@kernel.org>
> CC: Dexuan Cui <decui@microsoft.com>
> CC: Daniel Lezcano <daniel.lezcano@linaro.org>
> CC: Thomas Gleixner <tglx@linutronix.de>
> CC: linux-hyperv@vger.kernel.org
> CC: linux-kernel@vger.kernel.org
> ---
>  drivers/clocksource/hyperv_timer.c |   14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/clocksource/hyperv_timer.c b/drivers/clocksource/hyperv_timer.c
> index c4dbf40a3d3e..d447bc99a399 100644
> --- a/drivers/clocksource/hyperv_timer.c
> +++ b/drivers/clocksource/hyperv_timer.c
> @@ -367,6 +367,12 @@ static union {
>  } tsc_pg __aligned(PAGE_SIZE);
>  
>  static struct ms_hyperv_tsc_page *tsc_page = &tsc_pg.page;
> +static unsigned long tsc_pfn;
> +
> +static unsigned long hv_get_tsc_pfn(void)
> +{
> +	return tsc_pfn;
> +}
>  
>  struct ms_hyperv_tsc_page *hv_get_tsc_page(void)
>  {
> @@ -408,13 +414,12 @@ static void suspend_hv_clock_tsc(struct clocksource *arg)
>  
>  static void resume_hv_clock_tsc(struct clocksource *arg)
>  {
> -	phys_addr_t phys_addr = virt_to_phys(tsc_page);
>  	union hv_reference_tsc_msr tsc_msr;
>  
>  	/* Re-enable the TSC page */
>  	tsc_msr.as_uint64 = hv_get_register(HV_REGISTER_REFERENCE_TSC);
>  	tsc_msr.enable = 1;
> -	tsc_msr.pfn = __phys_to_pfn(phys_addr);
> +	tsc_msr.pfn = tsc_pfn;
>  	hv_set_register(HV_REGISTER_REFERENCE_TSC, tsc_msr.as_uint64);
>  }
>  
> @@ -498,7 +503,6 @@ static __always_inline void hv_setup_sched_clock(void *sched_clock) {}
>  static bool __init hv_init_tsc_clocksource(void)
>  {
>  	union hv_reference_tsc_msr tsc_msr;
> -	phys_addr_t	phys_addr;
>  
>  	if (!(ms_hyperv.features & HV_MSR_REFERENCE_TSC_AVAILABLE))
>  		return false;
> @@ -523,7 +527,7 @@ static bool __init hv_init_tsc_clocksource(void)
>  	}
>  
>  	hv_read_reference_counter = read_hv_clock_tsc;
> -	phys_addr = virt_to_phys(hv_get_tsc_page());
> +	tsc_pfn = __phys_to_pfn(virt_to_phys(tsc_page));
>  
>  	/*
>  	 * The Hyper-V TLFS specifies to preserve the value of reserved
> @@ -534,7 +538,7 @@ static bool __init hv_init_tsc_clocksource(void)
>  	 */
>  	tsc_msr.as_uint64 = hv_get_register(HV_REGISTER_REFERENCE_TSC);
>  	tsc_msr.enable = 1;
> -	tsc_msr.pfn = __phys_to_pfn(phys_addr);
> +	tsc_msr.pfn = tsc_pfn;
>  	hv_set_register(HV_REGISTER_REFERENCE_TSC, tsc_msr.as_uint64);
>  
>  	clocksource_register_hz(&hyperv_cs_tsc, NSEC_PER_SEC/100);
>
  

From: Stanislav Kinsburskii <skinsburskii@linux.microsoft.com> Sent: Tuesday, November 1, 2022 10:31 AM
> 
> And rework the code to use it instead of the physical address.
> This is a cleanup and precursor patch for upcoming support for TSC page
> mapping into hyper-v root partition.

Again, a slightly more robust commit message would be good.  Avoid a partial
sentence that is a continuation of the commit title (which isn't correct, as
Anirudh already pointed out).

> 
> Signed-off-by: Stanislav Kinsburskiy <stanislav.kinsburskiy@gmail.com>
> CC: "K. Y. Srinivasan" <kys@microsoft.com>
> CC: Haiyang Zhang <haiyangz@microsoft.com>
> CC: Wei Liu <wei.liu@kernel.org>
> CC: Dexuan Cui <decui@microsoft.com>
> CC: Daniel Lezcano <daniel.lezcano@linaro.org>
> CC: Thomas Gleixner <tglx@linutronix.de>
> CC: linux-hyperv@vger.kernel.org
> CC: linux-kernel@vger.kernel.org
> ---
>  drivers/clocksource/hyperv_timer.c |   14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/clocksource/hyperv_timer.c b/drivers/clocksource/hyperv_timer.c
> index c4dbf40a3d3e..d447bc99a399 100644
> --- a/drivers/clocksource/hyperv_timer.c
> +++ b/drivers/clocksource/hyperv_timer.c
> @@ -367,6 +367,12 @@ static union {
>  } tsc_pg __aligned(PAGE_SIZE);
> 
>  static struct ms_hyperv_tsc_page *tsc_page = &tsc_pg.page;
> +static unsigned long tsc_pfn;
> +
> +static unsigned long hv_get_tsc_pfn(void)
> +{
> +	return tsc_pfn;
> +}
> 
>  struct ms_hyperv_tsc_page *hv_get_tsc_page(void)
>  {
> @@ -408,13 +414,12 @@ static void suspend_hv_clock_tsc(struct clocksource *arg)
> 
>  static void resume_hv_clock_tsc(struct clocksource *arg)
>  {
> -	phys_addr_t phys_addr = virt_to_phys(tsc_page);
>  	union hv_reference_tsc_msr tsc_msr;
> 
>  	/* Re-enable the TSC page */
>  	tsc_msr.as_uint64 = hv_get_register(HV_REGISTER_REFERENCE_TSC);
>  	tsc_msr.enable = 1;
> -	tsc_msr.pfn = __phys_to_pfn(phys_addr);
> +	tsc_msr.pfn = tsc_pfn;
>  	hv_set_register(HV_REGISTER_REFERENCE_TSC, tsc_msr.as_uint64);
>  }
> 
> @@ -498,7 +503,6 @@ static __always_inline void hv_setup_sched_clock(void
> *sched_clock) {}
>  static bool __init hv_init_tsc_clocksource(void)
>  {
>  	union hv_reference_tsc_msr tsc_msr;
> -	phys_addr_t	phys_addr;
> 
>  	if (!(ms_hyperv.features & HV_MSR_REFERENCE_TSC_AVAILABLE))
>  		return false;
> @@ -523,7 +527,7 @@ static bool __init hv_init_tsc_clocksource(void)
>  	}
> 
>  	hv_read_reference_counter = read_hv_clock_tsc;
> -	phys_addr = virt_to_phys(hv_get_tsc_page());
> +	tsc_pfn = __phys_to_pfn(virt_to_phys(tsc_page));
> 
>  	/*
>  	 * The Hyper-V TLFS specifies to preserve the value of reserved
> @@ -534,7 +538,7 @@ static bool __init hv_init_tsc_clocksource(void)
>  	 */
>  	tsc_msr.as_uint64 = hv_get_register(HV_REGISTER_REFERENCE_TSC);
>  	tsc_msr.enable = 1;
> -	tsc_msr.pfn = __phys_to_pfn(phys_addr);
> +	tsc_msr.pfn = tsc_pfn;
>  	hv_set_register(HV_REGISTER_REFERENCE_TSC, tsc_msr.as_uint64);
> 
>  	clocksource_register_hz(&hyperv_cs_tsc, NSEC_PER_SEC/100);
>
  

From: Stanislav Kinsburskii <skinsburskii@linux.microsoft.com> Sent: Tuesday, November 1, 2022 10:31 AM
> 
> And rework the code to use it instead of the physical address.
> This is a cleanup and precursor patch for upcoming support for TSC page
> mapping into hyper-v root partition.
> 
> Signed-off-by: Stanislav Kinsburskiy <stanislav.kinsburskiy@gmail.com>
> CC: "K. Y. Srinivasan" <kys@microsoft.com>
> CC: Haiyang Zhang <haiyangz@microsoft.com>
> CC: Wei Liu <wei.liu@kernel.org>
> CC: Dexuan Cui <decui@microsoft.com>
> CC: Daniel Lezcano <daniel.lezcano@linaro.org>
> CC: Thomas Gleixner <tglx@linutronix.de>
> CC: linux-hyperv@vger.kernel.org
> CC: linux-kernel@vger.kernel.org
> ---
>  drivers/clocksource/hyperv_timer.c |   14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/clocksource/hyperv_timer.c b/drivers/clocksource/hyperv_timer.c
> index c4dbf40a3d3e..d447bc99a399 100644
> --- a/drivers/clocksource/hyperv_timer.c
> +++ b/drivers/clocksource/hyperv_timer.c
> @@ -367,6 +367,12 @@ static union {
>  } tsc_pg __aligned(PAGE_SIZE);
> 
>  static struct ms_hyperv_tsc_page *tsc_page = &tsc_pg.page;
> +static unsigned long tsc_pfn;
> +
> +static unsigned long hv_get_tsc_pfn(void)
> +{
> +	return tsc_pfn;
> +}

It makes sense to have the tsc_page global variable so that we can
handle the root partition and guest partition cases with common code,
even though the TSC page memory originates differently in the two cases.

But do we also need a tsc_pfn global variable and getter function?  When
the PFN is needed, conversion from the tsc_page virtual address to the PFN
isn't hard, and such a conversion is needed in only a couple of places.  To me,
it's simpler to keep a single global variable and getter function (i.e.,
hv_get_tsc_page), and do the conversions where needed.   Adding tsc_pfn
and the getter function introduces a fair amount of code churn for not much
benefit.  It's a judgment call, but that's my $.02.

I think this may be the same as what Anirudh is saying in his comments on
Patch 4/4 in this series.

Michael

> 
>  struct ms_hyperv_tsc_page *hv_get_tsc_page(void)
>  {
> @@ -408,13 +414,12 @@ static void suspend_hv_clock_tsc(struct clocksource *arg)
> 
>  static void resume_hv_clock_tsc(struct clocksource *arg)
>  {
> -	phys_addr_t phys_addr = virt_to_phys(tsc_page);
>  	union hv_reference_tsc_msr tsc_msr;
> 
>  	/* Re-enable the TSC page */
>  	tsc_msr.as_uint64 = hv_get_register(HV_REGISTER_REFERENCE_TSC);
>  	tsc_msr.enable = 1;
> -	tsc_msr.pfn = __phys_to_pfn(phys_addr);
> +	tsc_msr.pfn = tsc_pfn;
>  	hv_set_register(HV_REGISTER_REFERENCE_TSC, tsc_msr.as_uint64);
>  }
> 
> @@ -498,7 +503,6 @@ static __always_inline void hv_setup_sched_clock(void
> *sched_clock) {}
>  static bool __init hv_init_tsc_clocksource(void)
>  {
>  	union hv_reference_tsc_msr tsc_msr;
> -	phys_addr_t	phys_addr;
> 
>  	if (!(ms_hyperv.features & HV_MSR_REFERENCE_TSC_AVAILABLE))
>  		return false;
> @@ -523,7 +527,7 @@ static bool __init hv_init_tsc_clocksource(void)
>  	}
> 
>  	hv_read_reference_counter = read_hv_clock_tsc;
> -	phys_addr = virt_to_phys(hv_get_tsc_page());
> +	tsc_pfn = __phys_to_pfn(virt_to_phys(tsc_page));
> 
>  	/*
>  	 * The Hyper-V TLFS specifies to preserve the value of reserved
> @@ -534,7 +538,7 @@ static bool __init hv_init_tsc_clocksource(void)
>  	 */
>  	tsc_msr.as_uint64 = hv_get_register(HV_REGISTER_REFERENCE_TSC);
>  	tsc_msr.enable = 1;
> -	tsc_msr.pfn = __phys_to_pfn(phys_addr);
> +	tsc_msr.pfn = tsc_pfn;
>  	hv_set_register(HV_REGISTER_REFERENCE_TSC, tsc_msr.as_uint64);
> 
>  	clocksource_register_hz(&hyperv_cs_tsc, NSEC_PER_SEC/100);
>
  

From: Stanislav Kinsburskiy <stanislav.kinsburskiy@gmail.com>  Sent: Wednesday, November 2, 2022 12:26 PM

> ср, 2 нояб. 2022 г. в 12:07, Michael Kelley (LINUX) <mailto:mikelley@microsoft.com>:
> From: Stanislav Kinsburskii <mailto:skinsburskii@linux.microsoft.com> Sent: Tuesday, November 1, 2022 10:31 AM
> > 
> > And rework the code to use it instead of the physical address.
> > This is a cleanup and precursor patch for upcoming support for TSC page
> > mapping into hyper-v root partition.
> > 
> >  drivers/clocksource/hyperv_timer.c |   14 +++++++++-----
> >  1 file changed, 9 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/clocksource/hyperv_timer.c b/drivers/clocksource/hyperv_timer.c
> > index c4dbf40a3d3e..d447bc99a399 100644
> > --- a/drivers/clocksource/hyperv_timer.c
> > +++ b/drivers/clocksource/hyperv_timer.c
> > @@ -367,6 +367,12 @@ static union {
> >  } tsc_pg __aligned(PAGE_SIZE);
> > 
> >  static struct ms_hyperv_tsc_page *tsc_page = &tsc_pg.page;
> > +static unsigned long tsc_pfn;
> > +
> > +static unsigned long hv_get_tsc_pfn(void)
> > +{
> > +     return tsc_pfn;
> > +}
>
> > It makes sense to have the tsc_page global variable so that we can
> > handle the root partition and guest partition cases with common code,
> > even though the TSC page memory originates differently in the two cases.
> >
> > But do we also need a tsc_pfn global variable and getter function?  When
> > the PFN is needed, conversion from the tsc_page virtual address to the PFN
> > isn't hard, and such a conversion is needed in only a couple of places.  To me,
> > it's simpler to keep a single global variable and getter function (i.e.,
> > hv_get_tsc_page), and do the conversions where needed.   Adding tsc_pfn
> > and the getter function introduces a fair amount of code churn for not much
> > benefit.  It's a judgment call, but that's my $.02.
>
> As I replied to Anirudh , AFAIK virt_to_phys doesn't work for remapped pages.
> Another option would be to read the MSR each time PFN has to be returned to
> the vvar mapping function (i.e. on every fork), which introduces unnecessary
> performance regression..
> Another modification would be to make pfn a static variable and initialize it
> once in hv_get_tsc_pfn() on first access. But I like this implementation less.

Valid point about virt_to_phys().  But virt_to_hvpfn() does the right thing.  It
distinguishes between kernel addresses in the main linear mapping and
vmalloc() addresses, which is what you get from memremap().  But I haven't
looked through all the places virt_to_hvpfn() would be needed to make sure
it's safe to use.

However, thinking about virt_to_hvpfn(), there's a problem with Anirudh's
earlier patch set that started using __phys_to_pfn().  That won't work correctly
if the guest page size is not 4K because it will return a PFN based on the guest
page size, not based on Hyper-V's expectation that the PFN is based on a
4K page size.  So that needs to be fixed either way.

Michael
  

From: Stanislav Kinsburskiy <stanislav.kinsburskiy@gmail.com>  Sent: Wednesday, November 2, 2022 1:58 PM

>  ср, 2 нояб. 2022 г. в 13:30, Michael Kelley (LINUX) <mailto:mikelley@microsoft.com>:
> From: Stanislav Kinsburskiy <mailto:stanislav.kinsburskiy@gmail.com>  Sent: Wednesday, November 2, 2022 12:26 PM
>
> > > It makes sense to have the tsc_page global variable so that we can
> > > handle the root partition and guest partition cases with common code,
> > > even though the TSC page memory originates differently in the two cases.
> > >
> > > But do we also need a tsc_pfn global variable and getter function?  When
> > > the PFN is needed, conversion from the tsc_page virtual address to the PFN
> > > isn't hard, and such a conversion is needed in only a couple of places.  To me,
> > > it's simpler to keep a single global variable and getter function (i.e.,
> > > hv_get_tsc_page), and do the conversions where needed.   Adding tsc_pfn
> > > and the getter function introduces a fair amount of code churn for not much
> > > benefit.  It's a judgment call, but that's my $.02.
> >
> > As I replied to Anirudh , AFAIK virt_to_phys doesn't work for remapped pages.
> > Another option would be to read the MSR each time PFN has to be returned to
> > the vvar mapping function (i.e. on every fork), which introduces unnecessary
> > performance regression..
> > Another modification would be to make pfn a static variable and initialize it
> > once in hv_get_tsc_pfn() on first access. But I like this implementation less.

> > Valid point about virt_to_phys().  But virt_to_hvpfn() does the right thing.  It
> > distinguishes between kernel addresses in the main linear mapping and
> > vmalloc() addresses, which is what you get from memremap().  But I haven't
> > looked through all the places virt_to_hvpfn() would be needed to make sure
> > it's safe to use.
>
> Yeah, I guess virt_to_hvpfn() will do.
> But I'm not sure it the current code should be reworked to use it: it would save only a
> few lines of code, but will remove the explicit distinguishment between root and guest
> partitions, currently reflected in the code.
> Please, let me know if you insist on reworking the series to use virt_to_hvpfn().

Your call.  I'm OK with leaving things "as is" due to the additional complexity
of dealing with the vmalloc() address that comes from memremap().
 
> > However, thinking about virt_to_hvpfn(), there's a problem with Anirudh's
> > earlier patch set that started using __phys_to_pfn().  That won't work correctly
> > if the guest page size is not 4K because it will return a PFN based on the guest
> > page size, not based on Hyper-V's expectation that the PFN is based on a
> > 4K page size.  So that needs to be fixed either way.

> Could you elaborate more on the problem? 
 
The key is to recognize that PFNs are inherently interpreted in the context
of the page size.  Consider Guest Physical Address (GPA)  0x12340000.
If the page size is 4096, the PFN is 0x12340.  If the page size is 64K, the PFN
is 0x1234.  Hyper-V is always expecting PFNs in the context of a page size
of 4096.  But Linux guests running on Hyper-V on ARM64 could have a
guest page size of 64K, even though Hyper-V itself is using a page size
of 4096.  For my example, in an ARM64 VM with 64K page size,
__phys_to_pfn(0x12340000) would return 0x1234.  If that value is
stored in the PFN field of the MSR, Hyper-V will think the GPA is
0x1234000 when it should be 0x12340000.

Michael
  

On Wed, Nov 02, 2022 at 09:27:07PM +0000, Michael Kelley (LINUX) wrote:
> From: Stanislav Kinsburskiy <stanislav.kinsburskiy@gmail.com>  Sent: Wednesday, November 2, 2022 1:58 PM
> 
> >  ср, 2 нояб. 2022 г. в 13:30, Michael Kelley (LINUX) <mailto:mikelley@microsoft.com>:
> > From: Stanislav Kinsburskiy <mailto:stanislav.kinsburskiy@gmail.com>  Sent: Wednesday, November 2, 2022 12:26 PM
> >
> > > > It makes sense to have the tsc_page global variable so that we can
> > > > handle the root partition and guest partition cases with common code,
> > > > even though the TSC page memory originates differently in the two cases.
> > > >
> > > > But do we also need a tsc_pfn global variable and getter function?  When
> > > > the PFN is needed, conversion from the tsc_page virtual address to the PFN
> > > > isn't hard, and such a conversion is needed in only a couple of places.  To me,
> > > > it's simpler to keep a single global variable and getter function (i.e.,
> > > > hv_get_tsc_page), and do the conversions where needed.   Adding tsc_pfn
> > > > and the getter function introduces a fair amount of code churn for not much
> > > > benefit.  It's a judgment call, but that's my $.02.
> > >
> > > As I replied to Anirudh , AFAIK virt_to_phys doesn't work for remapped pages.
> > > Another option would be to read the MSR each time PFN has to be returned to
> > > the vvar mapping function (i.e. on every fork), which introduces unnecessary
> > > performance regression..
> > > Another modification would be to make pfn a static variable and initialize it
> > > once in hv_get_tsc_pfn() on first access. But I like this implementation less.
> 
> > > Valid point about virt_to_phys().  But virt_to_hvpfn() does the right thing.  It
> > > distinguishes between kernel addresses in the main linear mapping and
> > > vmalloc() addresses, which is what you get from memremap().  But I haven't
> > > looked through all the places virt_to_hvpfn() would be needed to make sure
> > > it's safe to use.
> >
> > Yeah, I guess virt_to_hvpfn() will do.
> > But I'm not sure it the current code should be reworked to use it: it would save only a
> > few lines of code, but will remove the explicit distinguishment between root and guest
> > partitions, currently reflected in the code.
> > Please, let me know if you insist on reworking the series to use virt_to_hvpfn().
> 
> Your call.  I'm OK with leaving things "as is" due to the additional complexity
> of dealing with the vmalloc() address that comes from memremap().
>  

I'll keep as it is then. Thanks.

> > > However, thinking about virt_to_hvpfn(), there's a problem with Anirudh's
> > > earlier patch set that started using __phys_to_pfn().  That won't work correctly
> > > if the guest page size is not 4K because it will return a PFN based on the guest
> > > page size, not based on Hyper-V's expectation that the PFN is based on a
> > > 4K page size.  So that needs to be fixed either way.
> 
> > Could you elaborate more on the problem? 
>  
> The key is to recognize that PFNs are inherently interpreted in the context
> of the page size.  Consider Guest Physical Address (GPA)  0x12340000.
> If the page size is 4096, the PFN is 0x12340.  If the page size is 64K, the PFN
> is 0x1234.  Hyper-V is always expecting PFNs in the context of a page size
> of 4096.  But Linux guests running on Hyper-V on ARM64 could have a
> guest page size of 64K, even though Hyper-V itself is using a page size
> of 4096.  For my example, in an ARM64 VM with 64K page size,
> __phys_to_pfn(0x12340000) would return 0x1234.  If that value is
> stored in the PFN field of the MSR, Hyper-V will think the GPA is
> 0x1234000 when it should be 0x12340000.
> 

Thank you for the verbose explanation.

Stas

> Michael
>