Re: [PATCH RFC v1 05/18] clocksource/hyperv: use MSR-based access if running as root

From: Wei Liu <wei.liu@kernel.org>
To: Vitaly Kuznetsov <vkuznets@redhat.com>
Cc: Wei Liu <wei.liu@kernel.org>,
	Linux on Hyper-V List <linux-hyperv@vger.kernel.org>,
	virtualization@lists.linux-foundation.org,
	Linux Kernel List <linux-kernel@vger.kernel.org>,
	Michael Kelley <mikelley@microsoft.com>,
	Vineeth Pillai <viremana@linux.microsoft.com>,
	Sunil Muthuswamy <sunilmut@microsoft.com>,
	Nuno Das Neves <nudasnev@microsoft.com>,
	"K. Y. Srinivasan" <kys@microsoft.com>,
	Haiyang Zhang <haiyangz@microsoft.com>,
	Stephen Hemminger <sthemmin@microsoft.com>,
	Daniel Lezcano <daniel.lezcano@linaro.org>,
	Thomas Gleixner <tglx@linutronix.de>
Subject: Re: [PATCH RFC v1 05/18] clocksource/hyperv: use MSR-based access if running as root
Date: Tue, 15 Sep 2020 10:32:04 +0000	[thread overview]
Message-ID: <20200915103204.53zlqx4jq7z2hpjw@liuwe-devbox-debian-v2> (raw)
In-Reply-To: <874knzl5ab.fsf@vitty.brq.redhat.com>

On Tue, Sep 15, 2020 at 12:10:04PM +0200, Vitaly Kuznetsov wrote:
> Wei Liu <wei.liu@kernel.org> writes:
> 
> > Signed-off-by: Wei Liu <wei.liu@kernel.org>
> > ---
> >  drivers/clocksource/hyperv_timer.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/clocksource/hyperv_timer.c b/drivers/clocksource/hyperv_timer.c
> > index 09aa44cb8a91..fe96082ce85e 100644
> > --- a/drivers/clocksource/hyperv_timer.c
> > +++ b/drivers/clocksource/hyperv_timer.c
> > @@ -426,6 +426,9 @@ static bool __init hv_init_tsc_clocksource(void)
> >  	if (!(ms_hyperv.features & HV_MSR_REFERENCE_TSC_AVAILABLE))
> >  		return false;
> >  
> > +	if (hv_root_partition)
> > +		return false;
> > +
> 
> Out of pure curiosity,
> 
> TSC page clocksource seems to be available to the root partition (as
> HV_MSR_REFERENCE_TSC_AVAILABLE is set), why don't we use it? (I
> understand that with TSC scaling support in modern CPUs even migration
> is a no-issue and we can use raw TSC but this all seems to be
> independent from root/non-root partition question).
> 

Yes. It is available to the root partition. An earlier version of this
patch did use that.  With some major restructuring of the code between
4.19 and 5.6 (splitting out the clocksource to and getting initialized
earlier), the old code wouldn't work anymore. 

The catch is that the page will have been set up by the hypervisor, so
in the function we will need to take a different path.  And making that
work again on 5.8 requires a bit more code churn.

Like I said in the cover letter, this is the bare minimum to get things
going. I try not distract people too much with less important stuff at
this stage, but making the reference TSC work again is definitely
something we want to look into in the future.

Wei.

> >  	hv_read_reference_counter = read_hv_clock_tsc;
> >  	phys_addr = virt_to_phys(hv_get_tsc_page());
> 
> -- 
> Vitaly
> 