diff mbox series

[v6,04/11] x86/smpboot: Reference count on smpboot_setup_warm_reset_vector()

Message ID 20230202215625.3248306-5-usama.arif@bytedance.com (mailing list archive)
State Superseded
Headers show
Series Parallel CPU bringup for x86_64 | expand

Commit Message

Usama Arif Feb. 2, 2023, 9:56 p.m. UTC 
From: David Woodhouse <dwmw@amazon.co.uk>

If we want to do parallel CPU bringup, we're going to need to set this up
and leave it until all CPUs are done. Might as well use the RTC spinlock
to protect the refcount, as we need to take it anyway.

[Usama Arif: fixed rebase conflict]
Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
Signed-off-by: Usama Arif <usama.arif@bytedance.com>
Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
---
 arch/x86/kernel/smpboot.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

Comments

Thomas Gleixner Feb. 6, 2023, 11:48 p.m. UTC | #1 
On Thu, Feb 02 2023 at 21:56, Usama Arif wrote:
> From: David Woodhouse <dwmw@amazon.co.uk>
>
> If we want to do parallel CPU bringup, we're going to need to set this up
> and leave it until all CPUs are done. Might as well use the RTC spinlock
> to protect the refcount, as we need to take it anyway.

https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#changelog

Aside of the 'We' this does not explain anything at all.

> [Usama Arif: fixed rebase conflict]
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> Signed-off-by: Usama Arif <usama.arif@bytedance.com>
> Signed-off-by: Paul E. McKenney <paulmck@kernel.org>

This SOB chain is even more broken...

> ---
>  arch/x86/kernel/smpboot.c | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
>
> diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
> index 55cad72715d9..a19eddcdccc2 100644
> --- a/arch/x86/kernel/smpboot.c
> +++ b/arch/x86/kernel/smpboot.c
> @@ -121,17 +121,22 @@ int arch_update_cpu_topology(void)
>  	return retval;
>  }
>  
> +
> +static unsigned int smpboot_warm_reset_vector_count;
> +
>  static inline void smpboot_setup_warm_reset_vector(unsigned long start_eip)
>  {
>  	unsigned long flags;
>  
>  	spin_lock_irqsave(&rtc_lock, flags);
> -	CMOS_WRITE(0xa, 0xf);
> +	if (!smpboot_warm_reset_vector_count++) {
> +		CMOS_WRITE(0xa, 0xf);
> +		*((volatile unsigned short *)phys_to_virt(TRAMPOLINE_PHYS_HIGH)) =
> +			start_eip >> 4;
> +		*((volatile unsigned short *)phys_to_virt(TRAMPOLINE_PHYS_LOW)) =
> +			start_eip & 0xf;

Again: 80 characters are history. Please fix that all over the series.

Thanks,

        tglx
Thomas Gleixner Feb. 7, 2023, 2:39 p.m. UTC | #2 
David!

On Tue, Feb 07 2023 at 09:49, David Woodhouse wrote:

Can you please fix your mail client to _NOT_ send multipart/mixed mails?
Despite the CC list being insanely large, your replies are dropped by
vger and missing in the archives.

> On Tue, 2023-02-07 at 00:48 +0100, Thomas Gleixner wrote:
>> On Thu, Feb 02 2023 at 21:56, Usama Arif wrote:
>> > From: David Woodhouse <dwmw@amazon.co.uk>
>> > 
>> > If we want to do parallel CPU bringup, we're going to need to set this up
>> > and leave it until all CPUs are done. Might as well use the RTC spinlock
>> > to protect the refcount, as we need to take it anyway.
>> 
>> https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#changelog
>> 
>> Aside of the 'We' this does not explain anything at all.
>
> Er, doesn't it?
>
> We refcount the warm reset vector because when we do parallel bringup,
> we'll want to set it up once and then put it back to normal (for cold
> reset) once all the CPUs are up.
>
> I can rework the phrasing; I'm aware that the whole nonsense about
> pronouns and the imperative mood has grown legs in the last couple of
> years since I originally wrote it — but is there anything actually
> missing? 

We can settle the imperative mood debate over a beer at the next
conference, but stuff which goes through tip is required to follow those
rules. No exception for you :)

Vs. the content: This changelog lacks context. Changelogs have to be
self contained and self explanatory. Assuming that they are
understandable due to the context of the patch series is just wrong. I
fundamentally hate it when I have to dig out the context when I stare at
the changelog of a commit.

So something like this:

   The warm reset vector on X86 is setup through the RTC (CMOS) clock
   for each CPU bringup operation and cleared after the CPU came online.

   Parallel bringup of multiple CPUs requires that the warm reset vector
   is valid until all CPUs came online.

   To prepare for that add refcounting for the reset vector and protect
   it with the rtc_lock which has to be taken for the setup operation
   anyway.

gives the full context and is simply factual, no?

Thanks,

       tglx
Sean Christopherson Feb. 7, 2023, 4:50 p.m. UTC | #3 
On Tue, Feb 07, 2023, Thomas Gleixner wrote:
> On Tue, Feb 07 2023 at 09:49, David Woodhouse wrote:
> > On Tue, 2023-02-07 at 00:48 +0100, Thomas Gleixner wrote:
> >> Aside of the 'We' this does not explain anything at all.
> >
> > Er, doesn't it?
> >
> > We refcount the warm reset vector because when we do parallel bringup,
> > we'll want to set it up once and then put it back to normal (for cold
> > reset) once all the CPUs are up.
> >
> > I can rework the phrasing; I'm aware that the whole nonsense about
> > pronouns and the imperative mood has grown legs in the last couple of
> > years since I originally wrote it — but is there anything actually
> > missing? 
> 
> We can settle the imperative mood debate over a beer at the next
> conference, but stuff which goes through tip is required to follow those
> rules. No exception for you :)

While we're reforming David, same goes for KVM x86.  :-)
David Woodhouse Feb. 7, 2023, 7:48 p.m. UTC | #4 
On Tue, 2023-02-07 at 15:39 +0100, Thomas Gleixner wrote:
> David!
> 
> On Tue, Feb 07 2023 at 09:49, David Woodhouse wrote:
> 
> Can you please fix your mail client to _NOT_ send multipart/mixed mails?
> Despite the CC list being insanely large, your replies are dropped by
> vger and missing in the archives.
> 

That's not the client; that's the stupid mail system doing it in
transit. Sorry, I'd already filed a ticket about that idiocy last week
when I noticed they'd started adding HTML parts to a previously sane
mail. But obviously they haven't managed to fix it yet.

The correct thing to do in the meantime is use a non-broken mail
account, and I just forgot this morning until half way through the
thread, when you'll note the coffee kicked in and I switched.


> > On Tue, 2023-02-07 at 00:48 +0100, Thomas Gleixner wrote:
> > > On Thu, Feb 02 2023 at 21:56, Usama Arif wrote:
> > > > From: David Woodhouse <dwmw@amazon.co.uk>
> > > > 
> > > > If we want to do parallel CPU bringup, we're going to need to set this up
> > > > and leave it until all CPUs are done. Might as well use the RTC spinlock
> > > > to protect the refcount, as we need to take it anyway.
> > > 
> > > https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#changelog
> > > 
> > > Aside of the 'We' this does not explain anything at all.
> > 
> > Er, doesn't it?
> > 
> > We refcount the warm reset vector because when we do parallel bringup,
> > we'll want to set it up once and then put it back to normal (for cold
> > reset) once all the CPUs are up.
> > 
> > I can rework the phrasing; I'm aware that the whole nonsense about
> > pronouns and the imperative mood has grown legs in the last couple of
> > years since I originally wrote it — but is there anything actually
> > missing?
> 
> We can settle the imperative mood debate over a beer at the next
> conference, but stuff which goes through tip is required to follow those
> rules. No exception for you :)
> 
> Vs. the content: This changelog lacks context. Changelogs have to be
> self contained and self explanatory. Assuming that they are
> understandable due to the context of the patch series is just wrong. I
> fundamentally hate it when I have to dig out the context when I stare at
> the changelog of a commit.
> 
> So something like this:
> 
>    The warm reset vector on X86 is setup through the RTC (CMOS) clock
>    for each CPU bringup operation and cleared after the CPU came online.
> 
>    Parallel bringup of multiple CPUs requires that the warm reset vector
>    is valid until all CPUs came online.
> 
>    To prepare for that add refcounting for the reset vector and protect
>    it with the rtc_lock which has to be taken for the setup operation
>    anyway.
> 
> gives the full context and is simply factual, no?

Ack.
diff mbox series

Patch

diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 55cad72715d9..a19eddcdccc2 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -121,17 +121,22 @@  int arch_update_cpu_topology(void)
 	return retval;
 }
 
+
+static unsigned int smpboot_warm_reset_vector_count;
+
 static inline void smpboot_setup_warm_reset_vector(unsigned long start_eip)
 {
 	unsigned long flags;
 
 	spin_lock_irqsave(&rtc_lock, flags);
-	CMOS_WRITE(0xa, 0xf);
+	if (!smpboot_warm_reset_vector_count++) {
+		CMOS_WRITE(0xa, 0xf);
+		*((volatile unsigned short *)phys_to_virt(TRAMPOLINE_PHYS_HIGH)) =
+			start_eip >> 4;
+		*((volatile unsigned short *)phys_to_virt(TRAMPOLINE_PHYS_LOW)) =
+			start_eip & 0xf;
+	}
 	spin_unlock_irqrestore(&rtc_lock, flags);
-	*((volatile unsigned short *)phys_to_virt(TRAMPOLINE_PHYS_HIGH)) =
-							start_eip >> 4;
-	*((volatile unsigned short *)phys_to_virt(TRAMPOLINE_PHYS_LOW)) =
-							start_eip & 0xf;
 }
 
 static inline void smpboot_restore_warm_reset_vector(void)
@@ -143,10 +148,12 @@  static inline void smpboot_restore_warm_reset_vector(void)
 	 * to default values.
 	 */
 	spin_lock_irqsave(&rtc_lock, flags);
-	CMOS_WRITE(0, 0xf);
+	if (!--smpboot_warm_reset_vector_count) {
+		CMOS_WRITE(0, 0xf);
+		*((volatile u32 *)phys_to_virt(TRAMPOLINE_PHYS_LOW)) = 0;
+	}
 	spin_unlock_irqrestore(&rtc_lock, flags);
 
-	*((volatile u32 *)phys_to_virt(TRAMPOLINE_PHYS_LOW)) = 0;
 }
 
 /*