diff mbox series

[1/4] drivers/clocksource/hyper-v: Introduce a pointer to TSC page

Message ID 166732386464.9827.16622091938453285710.stgit@skinsburskii-cloud-desktop.internal.cloudapp.net
State New
Headers
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>

Will be used later keep the address of the remapped page for the 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 |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

Michael Kelley (LINUX) Nov. 2, 2022, 6:56 p.m. UTC | #1 
From: Stanislav Kinsburskii <skinsburskii@linux.microsoft.com> Sent: Tuesday, November 1, 2022 10:31 AM
> 
> Will be used later keep the address of the remapped page for the root
> partition.

s/later keep/later to keep/

I'd like to see a more robust commit message, and not a partial
sentence that is a continuation of the commit title.  Something along the
lines of:

When Linux is running in the root partition of the Microsoft Hypervisor,
the memory for the TSC page is provided by the hypervisor, so the TSC
page address can't be the address of a Linux global variable.

Introduce a global variable to contain the TSC page address.  For a guest VM,
it defaults to the address of the Linux global variable.  If running in the root
partition, a later patch overrides to be the address of the page provided by
the hypervisor.

> 
> 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 |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/clocksource/hyperv_timer.c b/drivers/clocksource/hyperv_timer.c
> index 11332c82d1af..c4dbf40a3d3e 100644
> --- a/drivers/clocksource/hyperv_timer.c
> +++ b/drivers/clocksource/hyperv_timer.c
> @@ -366,9 +366,11 @@ static union {
>  	u8 reserved[PAGE_SIZE];
>  } tsc_pg __aligned(PAGE_SIZE);
> 
> +static struct ms_hyperv_tsc_page *tsc_page = &tsc_pg.page;
> +
>  struct ms_hyperv_tsc_page *hv_get_tsc_page(void)
>  {
> -	return &tsc_pg.page;
> +	return tsc_page;
>  }
>  EXPORT_SYMBOL_GPL(hv_get_tsc_page);
> 
> @@ -406,7 +408,7 @@ 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_pg);
> +	phys_addr_t phys_addr = virt_to_phys(tsc_page);
>  	union hv_reference_tsc_msr tsc_msr;
> 
>  	/* Re-enable the TSC page */
>
Michael Kelley (LINUX) Nov. 2, 2022, 8:44 p.m. UTC | #2 
From: Stanislav Kinsburskiy <stanislav.kinsburskiy@gmail.com> Sent: Wednesday, November 2, 2022 12:19 PM

> ср, 2 нояб. 2022 г. в 11:56, Michael Kelley (LINUX) <mailto:mikelley@microsoft.com>:
> From: Stanislav Kinsburskii <mailto:skinsburskii@linux.microsoft.com> Sent: Tuesday, November 1, 2022 10:31 AM
> > > 
> > > Will be used later keep the address of the remapped page for the root
> > > partition.
> >
> > s/later keep/later to keep/
> 
> I'll fix it, thanks.
>  
> > I'd like to see a more robust commit message, and not a partial
> > sentence that is a continuation of the commit title.  Something along the lines of:
> > 
> > When Linux is running in the root partition of the Microsoft Hypervisor,
> > the memory for the TSC page is provided by the hypervisor, so the TSC
> > page address can't be the address of a Linux global variable.
> >
> > Introduce a global variable to contain the TSC page address.  For a guest VM,
> > it defaults to the address of the Linux global variable.  If running in the root
> > partition, a later patch overrides to be the address of the page provided by
> > the hypervisor.
> 
> This is a cleanup patch whose goal is to provide a clear separation between the
> actual feature (which comes in the last patch of the series) and other precursor
> changes, making the feature introduction more laconic and clean.
> 
> I doubt it needs any additional text to expose the details of the resulting goal.
> Why do you think otherwise?

To me, the additional text clearly answers the "why" question for the patch.  Here's
the quote from Documentation/process/submitting-patches.rst:

   The explanation body will be committed to the permanent source
   changelog, so should make sense to a competent reader who has long since
   forgotten the immediate details of the discussion that might have led to
   this patch. Including symptoms of the failure which the patch addresses
   (kernel log messages, oops messages, etc.) are especially useful for
   people who might be searching the commit logs looking for the applicable
   patch. The text should be written in such detail so that when read
   weeks, months or even years later, it can give the reader the needed
   details to grasp the reasoning for **why** the patch was created.

Certainly, it's somewhat a matter of personal style, and I tend to lean toward the
"more explanation is better" approach.  But if no one else cares to weigh in on
the topic, it's not a blocker for me.

Michael
diff mbox series

Patch

diff --git a/drivers/clocksource/hyperv_timer.c b/drivers/clocksource/hyperv_timer.c
index 11332c82d1af..c4dbf40a3d3e 100644
--- a/drivers/clocksource/hyperv_timer.c
+++ b/drivers/clocksource/hyperv_timer.c
@@ -366,9 +366,11 @@  static union {
 	u8 reserved[PAGE_SIZE];
 } tsc_pg __aligned(PAGE_SIZE);
 
+static struct ms_hyperv_tsc_page *tsc_page = &tsc_pg.page;
+
 struct ms_hyperv_tsc_page *hv_get_tsc_page(void)
 {
-	return &tsc_pg.page;
+	return tsc_page;
 }
 EXPORT_SYMBOL_GPL(hv_get_tsc_page);
 
@@ -406,7 +408,7 @@  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_pg);
+	phys_addr_t phys_addr = virt_to_phys(tsc_page);
 	union hv_reference_tsc_msr tsc_msr;
 
 	/* Re-enable the TSC page */