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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <BYAPR21MB1688C426F44E40E1415DCD21D7399@BYAPR21MB1688.namprd21.prod.outlook.com>
Date:   Wed, 2 Nov 2022 19:06:59 +0000
From:   "Michael Kelley (LINUX)" <mikelley@...rosoft.com>
To:     Stanislav Kinsburskii <skinsburskii@...ux.microsoft.com>
CC:     Stanislav Kinsburskiy <stanislav.kinsburskiy@...il.com>,
        KY Srinivasan <kys@...rosoft.com>,
        Haiyang Zhang <haiyangz@...rosoft.com>,
        Wei Liu <wei.liu@...nel.org>, Dexuan Cui <decui@...rosoft.com>,
        Daniel Lezcano <daniel.lezcano@...aro.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        "linux-hyperv@...r.kernel.org" <linux-hyperv@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH 2/4] drivers/clocksource/hyper-v: Introduce TSC MSR
 register structure

From: Stanislav Kinsburskii <skinsburskii@...ux.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@...il.com>
> CC: "K. Y. Srinivasan" <kys@...rosoft.com>
> CC: Haiyang Zhang <haiyangz@...rosoft.com>
> CC: Wei Liu <wei.liu@...nel.org>
> CC: Dexuan Cui <decui@...rosoft.com>
> CC: Daniel Lezcano <daniel.lezcano@...aro.org>
> CC: Thomas Gleixner <tglx@...utronix.de>
> CC: linux-hyperv@...r.kernel.org
> CC: linux-kernel@...r.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);
>