Message ID | 166732386986.9827.12356845572628674464.stgit@skinsburskii-cloud-desktop.internal.cloudapp.net |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:6687:0:0:0:0:0 with SMTP id l7csp3107523wru; Tue, 1 Nov 2022 10:35:45 -0700 (PDT) X-Google-Smtp-Source: AMsMyM6lOL3Kuiz+pFZMRmEpxG7tDFFODYcu9b0a3B4m83veCmoXAznIwD+A6UWAiPV6EOHREvg8 X-Received: by 2002:a17:90b:4a04:b0:213:587b:204e with SMTP id kk4-20020a17090b4a0400b00213587b204emr21134514pjb.98.1667324144579; Tue, 01 Nov 2022 10:35:44 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1667324144; cv=none; d=google.com; s=arc-20160816; b=UE47Hhg9nl5wYO96o8ON1SZxMewloqFkjvIfxLpzueNiHdlUhOBl/qEztBBSGFCqUh cKCVa2JMvmJLE0gdwuiFW/wbf/ZdQB9n/+kwJrupzVCiKIcHvWAXUWAbmXnjkUhEjQpM wzi9FuLL6F2vzlVLbBCz0bmMPyjeXAV+ZoRTkoBoqtl7sPpYd38YnHiWloMPmiTx03nA pR0DkkqrA+buUkNMW6RMoXJdSzMayi6WJcMZOplEqn24kc/3lDxskeb8K3WrkI+8DxQW yNug+OUyTlsueyX1rdhftbrLFLJQu4PW0/TIQvGdOdi/O3DaJN9OI5/Qpx6M4S+vuoYH f/SQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:to:content-transfer-encoding:mime-version :user-agent:references:in-reply-to:message-id:date:cc:from:subject :dkim-signature:dkim-filter; bh=bFl7hxOTym7GqSMmVRUZEZkOBSvYSKk8SZ3l7c9kTt0=; b=XMSI14Ng3OclvBOKbN4QwC5dVcX2lkzPfusQS5iTQcnmD85+OvcJMGiBPKe8SIv6p/ 6aXhNC4L3o8tGWUi7DEMnX1KZEuSo9kzqTUjw7NGdKkdbTKZr81hRAAJqmNg0aMoniuV yxBJSFfpVRM+3mAOs4Xf9eR7Bfa/RvbMNt+Xlk/LEtQ7kV37N9F8s46MJZkkLvkWOOyB Hx4nPUsw4Bgu6aEcMds32xogXIWC0zpXON6iuVADyrNl+tO63mW3DQAXRK18sGtgiOkZ OVxTwwhJBFd7gw7mPIYOeKwejuIO9nHzcs3Ysz9jqQ18LQnzIIeu9+9iiVJmZ0rhFf7p PWpA== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@linux.microsoft.com header.s=default header.b=deE016wC; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=linux.microsoft.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id mu17-20020a17090b389100b00213b01e42adsi14837758pjb.42.2022.11.01.10.35.26; Tue, 01 Nov 2022 10:35:44 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@linux.microsoft.com header.s=default header.b=deE016wC; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=linux.microsoft.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230384AbiKARbO (ORCPT <rfc822;kartikey406@gmail.com> + 99 others); Tue, 1 Nov 2022 13:31:14 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:60704 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230336AbiKARbK (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Tue, 1 Nov 2022 13:31:10 -0400 Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by lindbergh.monkeyblade.net (Postfix) with ESMTP id 37BA81C12E; Tue, 1 Nov 2022 10:31:10 -0700 (PDT) Received: from skinsburskii-cloud-desktop.internal.cloudapp.net (unknown [20.120.152.163]) by linux.microsoft.com (Postfix) with ESMTPSA id 015D020B9F80; Tue, 1 Nov 2022 10:31:10 -0700 (PDT) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com 015D020B9F80 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1667323870; bh=bFl7hxOTym7GqSMmVRUZEZkOBSvYSKk8SZ3l7c9kTt0=; h=Subject:From:Cc:Date:In-Reply-To:References:From; b=deE016wC5+YZS1tkfB6MHf6jU2aGMN8r/UhID9NTOJTMryK/FiDlGWJs5UkTizutP 6wKGTwsglpOa2Jt1Qn+bwUopxVBWRSqUkJrw70WULOF4cnK5DeOpjphyApNwtJNPCt ziWczGn7W8uQ1zWo0OrkGg2kHrDIGD2ljKfo5fP4= Subject: [PATCH 2/4] drivers/clocksource/hyper-v: Introduce TSC MSR register structure From: Stanislav Kinsburskii <skinsburskii@linux.microsoft.com> Cc: Stanislav Kinsburskiy <stanislav.kinsburskiy@gmail.com>, "K. Y. Srinivasan" <kys@microsoft.com>, Haiyang Zhang <haiyangz@microsoft.com>, Wei Liu <wei.liu@kernel.org>, Dexuan Cui <decui@microsoft.com>, Daniel Lezcano <daniel.lezcano@linaro.org>, Thomas Gleixner <tglx@linutronix.de>, linux-hyperv@vger.kernel.org, linux-kernel@vger.kernel.org Date: Tue, 01 Nov 2022 17:31:09 +0000 Message-ID: <166732386986.9827.12356845572628674464.stgit@skinsburskii-cloud-desktop.internal.cloudapp.net> In-Reply-To: <166732356767.9827.4925884794177179249.stgit@skinsburskii-cloud-desktop.internal.cloudapp.net> References: <166732356767.9827.4925884794177179249.stgit@skinsburskii-cloud-desktop.internal.cloudapp.net> User-Agent: StGit/0.19 MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-18.8 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,ENV_AND_HDR_SPF_MATCH,MISSING_HEADERS, RCVD_IN_DNSWL_MED,SPF_HELO_PASS,SPF_PASS,USER_IN_DEF_DKIM_WL, USER_IN_DEF_SPF_WL autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net To: unlisted-recipients:; (no To-header on input) Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1748316081935747927?= X-GMAIL-MSGID: =?utf-8?q?1748316081935747927?= |
Series |
hyper-v: Introduce TSC page for root partition
|
|
Commit Message
Stanislav Kinsburskii
Nov. 1, 2022, 5:31 p.m. UTC
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(-)
Comments
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 >
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);