[<prev] [next>] [day] [month] [year] [list]
Message-ID: <BYAPR21MB1688D5D33AC0D7D88AF6FD7BD7399@BYAPR21MB1688.namprd21.prod.outlook.com>
Date: Wed, 2 Nov 2022 20:30:27 +0000
From: "Michael Kelley (LINUX)" <mikelley@...rosoft.com>
To: Stanislav Kinsburskiy <stanislav.kinsburskiy@...il.com>,
Anirudh Rayabharam <anrayabh@...ux.microsoft.com>
CC: Stanislav Kinsburskii <skinsburskii@...ux.microsoft.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 Kinsburskiy <stanislav.kinsburskiy@...il.com> Sent: Wednesday, November 2, 2022 12:26 PM
> ср, 2 нояб. 2022 г. в 12:07, Michael Kelley (LINUX) <mailto:mikelley@...rosoft.com>:
> From: Stanislav Kinsburskii <mailto: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.
> >
> > 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
Powered by blists - more mailing lists