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

[Michael Kelley (LINUX)]

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

> ср, 2 нояб. 2022 г. в 12:07, Michael Kelley (LINUX) <mailto:mikelley@xxxxxxxxxxxxx>:
> From: Stanislav Kinsburskii <mailto:skinsburskii@xxxxxxxxxxxxxxxxxxx> 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