public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [PATCH] UefiCpuPkg/LocalApicLib: Exclude second SendIpi on AMD
@ 2018-06-13 20:11 Leo Duran
  2018-06-13 20:11 ` [PATCH] UefiCpuPkg/LocalApicLib: Exclude second SendIpi sequence on AMD processors Leo Duran
  0 siblings, 1 reply; 12+ messages in thread
From: Leo Duran @ 2018-06-13 20:11 UTC (permalink / raw)
  To: edk2-devel

This patch is related to the patch-set discussed here:
https://www.mail-archive.com/edk2-devel@lists.01.org/msg27020.html

The GetProcessorLocationByApicId portion of that patch-set was pushed,
but the SendIpi issue was defered based on questions Jeff raised here:
https://www.mail-archive.com/edk2-devel@lists.01.org/msg27195.html

In any event, I do not have public documents explictly describing that
a) second SendIpi is not required on AMD processors, or that
b) only a single SendIpi is required on AMD processors.

However, as stated in the commit log:
On AMD processors the second SendIpi in the SendInitSipiSipi and
SendInitSipiSipiAllExcludingSelf routines is not required, and may cause
undesired side-effects during MP initialization.

Leo Duran (1):
  UefiCpuPkg/LocalApicLib: Exclude second SendIpi sequence on AMD
    processors.

 UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c             | 12 ++++++++----
 UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.c | 12 ++++++++----
 2 files changed, 16 insertions(+), 8 deletions(-)

-- 
2.7.4



^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH] UefiCpuPkg/LocalApicLib: Exclude second SendIpi sequence on AMD processors.
  2018-06-13 20:11 [PATCH] UefiCpuPkg/LocalApicLib: Exclude second SendIpi on AMD Leo Duran
@ 2018-06-13 20:11 ` Leo Duran
  2018-06-13 20:49   ` Laszlo Ersek
  2018-06-14 16:36   ` Laszlo Ersek
  0 siblings, 2 replies; 12+ messages in thread
From: Leo Duran @ 2018-06-13 20:11 UTC (permalink / raw)
  To: edk2-devel; +Cc: Leo Duran, Jordan Justen, Jeff Fan, Liming Gao

On AMD processors the second SendIpi in the SendInitSipiSipi and
SendInitSipiSipiAllExcludingSelf routines is not required, and may cause
undesired side-effects during MP initialization.

This patch leverages the StandardSignatureIsAuthenticAMD check to exclude
the second SendIpi and its associated MicroSecondDelay (200).

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Leo Duran <leo.duran@amd.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Jeff Fan <jeff.fan@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
---
 UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c             | 12 ++++++++----
 UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.c | 12 ++++++++----
 2 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c b/UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c
index b0b7e32..6e80536 100644
--- a/UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c
+++ b/UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c
@@ -554,8 +554,10 @@ SendInitSipiSipi (
   IcrLow.Bits.DeliveryMode = LOCAL_APIC_DELIVERY_MODE_STARTUP;
   IcrLow.Bits.Level = 1;
   SendIpi (IcrLow.Uint32, ApicId);
-  MicroSecondDelay (200);
-  SendIpi (IcrLow.Uint32, ApicId);
+  if (!StandardSignatureIsAuthenticAMD()) {
+    MicroSecondDelay (200);
+    SendIpi (IcrLow.Uint32, ApicId);
+  }
 }
 
 /**
@@ -588,8 +590,10 @@ SendInitSipiSipiAllExcludingSelf (
   IcrLow.Bits.Level = 1;
   IcrLow.Bits.DestinationShorthand = LOCAL_APIC_DESTINATION_SHORTHAND_ALL_EXCLUDING_SELF;
   SendIpi (IcrLow.Uint32, 0);
-  MicroSecondDelay (200);
-  SendIpi (IcrLow.Uint32, 0);
+  if (!StandardSignatureIsAuthenticAMD()) {
+    MicroSecondDelay (200);
+    SendIpi (IcrLow.Uint32, 0);
+  }
 }
 
 /**
diff --git a/UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.c b/UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.c
index 1f4dcf7..5d82836 100644
--- a/UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.c
+++ b/UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.c
@@ -649,8 +649,10 @@ SendInitSipiSipi (
   IcrLow.Bits.DeliveryMode = LOCAL_APIC_DELIVERY_MODE_STARTUP;
   IcrLow.Bits.Level = 1;
   SendIpi (IcrLow.Uint32, ApicId);
-  MicroSecondDelay (200);
-  SendIpi (IcrLow.Uint32, ApicId);
+  if (!StandardSignatureIsAuthenticAMD()) {
+    MicroSecondDelay (200);
+    SendIpi (IcrLow.Uint32, ApicId);
+  }
 }
 
 /**
@@ -683,8 +685,10 @@ SendInitSipiSipiAllExcludingSelf (
   IcrLow.Bits.Level = 1;
   IcrLow.Bits.DestinationShorthand = LOCAL_APIC_DESTINATION_SHORTHAND_ALL_EXCLUDING_SELF;
   SendIpi (IcrLow.Uint32, 0);
-  MicroSecondDelay (200);
-  SendIpi (IcrLow.Uint32, 0);
+  if (!StandardSignatureIsAuthenticAMD()) {
+    MicroSecondDelay (200);
+    SendIpi (IcrLow.Uint32, 0);
+  }
 }
 
 /**
-- 
2.7.4



^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] UefiCpuPkg/LocalApicLib: Exclude second SendIpi sequence on AMD processors.
  2018-06-13 20:11 ` [PATCH] UefiCpuPkg/LocalApicLib: Exclude second SendIpi sequence on AMD processors Leo Duran
@ 2018-06-13 20:49   ` Laszlo Ersek
  2018-06-13 20:52     ` Paolo Bonzini
  2018-06-14 14:08     ` Duran, Leo
  2018-06-14 16:36   ` Laszlo Ersek
  1 sibling, 2 replies; 12+ messages in thread
From: Laszlo Ersek @ 2018-06-13 20:49 UTC (permalink / raw)
  To: Leo Duran, edk2-devel
  Cc: Jordan Justen, Jeff Fan, Liming Gao, Brijesh Singh, Paolo Bonzini,
	Igor Mammedov

Hello Leo,

On 06/13/18 22:11, Leo Duran wrote:
> On AMD processors the second SendIpi in the SendInitSipiSipi and
> SendInitSipiSipiAllExcludingSelf routines is not required, and may cause
> undesired side-effects during MP initialization.
> 
> This patch leverages the StandardSignatureIsAuthenticAMD check to exclude
> the second SendIpi and its associated MicroSecondDelay (200).

QEMU and KVM emulate some AMD processors too; of particular interest is
the recent EPYC addition, I believe (for SME/SEV, minimally).

Did you check whether the StandardSignatureIsAuthenticAMD() check
applies to those QEMU VCPU models, and if so, whether omitting the
second Startup IPI interferes with *V*CPU startup in OVMF guests? (In
multiprocessing modules, such as CpuMpPei, CpuDxe, and PiSmmCpuDxeSmm.)

Adding Brijesh, Paolo and Igor.

Thanks!
Laszlo

> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Leo Duran <leo.duran@amd.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Jeff Fan <jeff.fan@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> ---
>  UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c             | 12 ++++++++----
>  UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.c | 12 ++++++++----
>  2 files changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c b/UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c
> index b0b7e32..6e80536 100644
> --- a/UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c
> +++ b/UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c
> @@ -554,8 +554,10 @@ SendInitSipiSipi (
>    IcrLow.Bits.DeliveryMode = LOCAL_APIC_DELIVERY_MODE_STARTUP;
>    IcrLow.Bits.Level = 1;
>    SendIpi (IcrLow.Uint32, ApicId);
> -  MicroSecondDelay (200);
> -  SendIpi (IcrLow.Uint32, ApicId);
> +  if (!StandardSignatureIsAuthenticAMD()) {
> +    MicroSecondDelay (200);
> +    SendIpi (IcrLow.Uint32, ApicId);
> +  }
>  }
>  
>  /**
> @@ -588,8 +590,10 @@ SendInitSipiSipiAllExcludingSelf (
>    IcrLow.Bits.Level = 1;
>    IcrLow.Bits.DestinationShorthand = LOCAL_APIC_DESTINATION_SHORTHAND_ALL_EXCLUDING_SELF;
>    SendIpi (IcrLow.Uint32, 0);
> -  MicroSecondDelay (200);
> -  SendIpi (IcrLow.Uint32, 0);
> +  if (!StandardSignatureIsAuthenticAMD()) {
> +    MicroSecondDelay (200);
> +    SendIpi (IcrLow.Uint32, 0);
> +  }
>  }
>  
>  /**
> diff --git a/UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.c b/UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.c
> index 1f4dcf7..5d82836 100644
> --- a/UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.c
> +++ b/UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.c
> @@ -649,8 +649,10 @@ SendInitSipiSipi (
>    IcrLow.Bits.DeliveryMode = LOCAL_APIC_DELIVERY_MODE_STARTUP;
>    IcrLow.Bits.Level = 1;
>    SendIpi (IcrLow.Uint32, ApicId);
> -  MicroSecondDelay (200);
> -  SendIpi (IcrLow.Uint32, ApicId);
> +  if (!StandardSignatureIsAuthenticAMD()) {
> +    MicroSecondDelay (200);
> +    SendIpi (IcrLow.Uint32, ApicId);
> +  }
>  }
>  
>  /**
> @@ -683,8 +685,10 @@ SendInitSipiSipiAllExcludingSelf (
>    IcrLow.Bits.Level = 1;
>    IcrLow.Bits.DestinationShorthand = LOCAL_APIC_DESTINATION_SHORTHAND_ALL_EXCLUDING_SELF;
>    SendIpi (IcrLow.Uint32, 0);
> -  MicroSecondDelay (200);
> -  SendIpi (IcrLow.Uint32, 0);
> +  if (!StandardSignatureIsAuthenticAMD()) {
> +    MicroSecondDelay (200);
> +    SendIpi (IcrLow.Uint32, 0);
> +  }
>  }
>  
>  /**
> 



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] UefiCpuPkg/LocalApicLib: Exclude second SendIpi sequence on AMD processors.
  2018-06-13 20:49   ` Laszlo Ersek
@ 2018-06-13 20:52     ` Paolo Bonzini
  2018-06-14  5:39       ` Ni, Ruiyu
  2018-06-14 14:08     ` Duran, Leo
  1 sibling, 1 reply; 12+ messages in thread
From: Paolo Bonzini @ 2018-06-13 20:52 UTC (permalink / raw)
  To: Laszlo Ersek, Leo Duran, edk2-devel
  Cc: Jordan Justen, Jeff Fan, Liming Gao, Brijesh Singh, Igor Mammedov

On 13/06/2018 22:49, Laszlo Ersek wrote:
> Hello Leo,
> 
> On 06/13/18 22:11, Leo Duran wrote:
>> On AMD processors the second SendIpi in the SendInitSipiSipi and
>> SendInitSipiSipiAllExcludingSelf routines is not required, and may cause
>> undesired side-effects during MP initialization.
>>
>> This patch leverages the StandardSignatureIsAuthenticAMD check to exclude
>> the second SendIpi and its associated MicroSecondDelay (200).
> 
> QEMU and KVM emulate some AMD processors too; of particular interest is
> the recent EPYC addition, I believe (for SME/SEV, minimally).
> 
> Did you check whether the StandardSignatureIsAuthenticAMD() check
> applies to those QEMU VCPU models, and if so, whether omitting the
> second Startup IPI interferes with *V*CPU startup in OVMF guests? (In
> multiprocessing modules, such as CpuMpPei, CpuDxe, and PiSmmCpuDxeSmm.)
> 
> Adding Brijesh, Paolo and Igor.

Actually I would be surprised if any recent processor needs the
INIT-SIPI-SIPI (though I'm not sure what undesired side effects it might
trigger).

Paolo


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] UefiCpuPkg/LocalApicLib: Exclude second SendIpi sequence on AMD processors.
  2018-06-13 20:52     ` Paolo Bonzini
@ 2018-06-14  5:39       ` Ni, Ruiyu
  2018-06-14  9:04         ` Paolo Bonzini
  0 siblings, 1 reply; 12+ messages in thread
From: Ni, Ruiyu @ 2018-06-14  5:39 UTC (permalink / raw)
  To: Paolo Bonzini, Laszlo Ersek, Leo Duran, edk2-devel@lists.01.org
  Cc: Justen, Jordan L, Brijesh Singh, Jeff Fan, Gao, Liming



Thanks/Ray

> -----Original Message-----
> From: edk2-devel <edk2-devel-bounces@lists.01.org> On Behalf Of Paolo
> Bonzini
> Sent: Thursday, June 14, 2018 4:52 AM
> To: Laszlo Ersek <lersek@redhat.com>; Leo Duran <leo.duran@amd.com>;
> edk2-devel@lists.01.org
> Cc: Justen, Jordan L <jordan.l.justen@intel.com>; Brijesh Singh
> <brijesh.singh@amd.com>; Jeff Fan <jeff.fan@intel.com>; Gao, Liming
> <liming.gao@intel.com>
> Subject: Re: [edk2] [PATCH] UefiCpuPkg/LocalApicLib: Exclude second
> SendIpi sequence on AMD processors.
> 
> On 13/06/2018 22:49, Laszlo Ersek wrote:
> > Hello Leo,
> >
> > On 06/13/18 22:11, Leo Duran wrote:
> >> On AMD processors the second SendIpi in the SendInitSipiSipi and
> >> SendInitSipiSipiAllExcludingSelf routines is not required, and may
> >> cause undesired side-effects during MP initialization.
> >>
> >> This patch leverages the StandardSignatureIsAuthenticAMD check to
> >> exclude the second SendIpi and its associated MicroSecondDelay (200).
> >
> > QEMU and KVM emulate some AMD processors too; of particular interest
> > is the recent EPYC addition, I believe (for SME/SEV, minimally).
> >
> > Did you check whether the StandardSignatureIsAuthenticAMD() check
> > applies to those QEMU VCPU models, and if so, whether omitting the
> > second Startup IPI interferes with *V*CPU startup in OVMF guests? (In
> > multiprocessing modules, such as CpuMpPei, CpuDxe, and
> > PiSmmCpuDxeSmm.)
> >
> > Adding Brijesh, Paolo and Igor.
> 
> Actually I would be surprised if any recent processor needs the INIT-SIPI-SIPI
> (though I'm not sure what undesired side effects it might trigger).

Why the recent processors don't need INIT-SIPI-SIPI?
I thought it is the only way to wake up processors when it's halted (HLT).

> 
> Paolo
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] UefiCpuPkg/LocalApicLib: Exclude second SendIpi sequence on AMD processors.
  2018-06-14  5:39       ` Ni, Ruiyu
@ 2018-06-14  9:04         ` Paolo Bonzini
  0 siblings, 0 replies; 12+ messages in thread
From: Paolo Bonzini @ 2018-06-14  9:04 UTC (permalink / raw)
  To: Ni, Ruiyu, Laszlo Ersek, Leo Duran, edk2-devel@lists.01.org
  Cc: Justen, Jordan L, Brijesh Singh, Jeff Fan, Gao, Liming

On 14/06/2018 07:39, Ni, Ruiyu wrote:
> 
> 
> Thanks/Ray
> 
>> -----Original Message-----
>> From: edk2-devel <edk2-devel-bounces@lists.01.org> On Behalf Of Paolo
>> Bonzini
>> Sent: Thursday, June 14, 2018 4:52 AM
>> To: Laszlo Ersek <lersek@redhat.com>; Leo Duran <leo.duran@amd.com>;
>> edk2-devel@lists.01.org
>> Cc: Justen, Jordan L <jordan.l.justen@intel.com>; Brijesh Singh
>> <brijesh.singh@amd.com>; Jeff Fan <jeff.fan@intel.com>; Gao, Liming
>> <liming.gao@intel.com>
>> Subject: Re: [edk2] [PATCH] UefiCpuPkg/LocalApicLib: Exclude second
>> SendIpi sequence on AMD processors.
>>
>> On 13/06/2018 22:49, Laszlo Ersek wrote:
>>> Hello Leo,
>>>
>>> On 06/13/18 22:11, Leo Duran wrote:
>>>> On AMD processors the second SendIpi in the SendInitSipiSipi and
>>>> SendInitSipiSipiAllExcludingSelf routines is not required, and may
>>>> cause undesired side-effects during MP initialization.
>>>>
>>>> This patch leverages the StandardSignatureIsAuthenticAMD check to
>>>> exclude the second SendIpi and its associated MicroSecondDelay (200).
>>>
>>> QEMU and KVM emulate some AMD processors too; of particular interest
>>> is the recent EPYC addition, I believe (for SME/SEV, minimally).
>>>
>>> Did you check whether the StandardSignatureIsAuthenticAMD() check
>>> applies to those QEMU VCPU models, and if so, whether omitting the
>>> second Startup IPI interferes with *V*CPU startup in OVMF guests? (In
>>> multiprocessing modules, such as CpuMpPei, CpuDxe, and
>>> PiSmmCpuDxeSmm.)
>>>
>>> Adding Brijesh, Paolo and Igor.
>>
>> Actually I would be surprised if any recent processor needs the INIT-SIPI-SIPI
>> (though I'm not sure what undesired side effects it might trigger).
> 
> Why the recent processors don't need INIT-SIPI-SIPI?
> I thought it is the only way to wake up processors when it's halted (HLT).

INIT-SIPI should be enough.  The second SIPI is just in case the first
one was missed.

Paolo


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] UefiCpuPkg/LocalApicLib: Exclude second SendIpi sequence on AMD processors.
  2018-06-13 20:49   ` Laszlo Ersek
  2018-06-13 20:52     ` Paolo Bonzini
@ 2018-06-14 14:08     ` Duran, Leo
  2018-06-14 14:52       ` Andrew Fish
  1 sibling, 1 reply; 12+ messages in thread
From: Duran, Leo @ 2018-06-14 14:08 UTC (permalink / raw)
  To: Laszlo Ersek, edk2-devel@lists.01.org
  Cc: Jordan Justen, Jeff Fan, Liming Gao, Singh, Brijesh,
	Paolo Bonzini, Igor Mammedov



> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Wednesday, June 13, 2018 3:50 PM
> To: Duran, Leo <leo.duran@amd.com>; edk2-devel@lists.01.org
> Cc: Jordan Justen <jordan.l.justen@intel.com>; Jeff Fan
> <jeff.fan@intel.com>; Liming Gao <liming.gao@intel.com>; Singh, Brijesh
> <brijesh.singh@amd.com>; Paolo Bonzini <pbonzini@redhat.com>; Igor
> Mammedov <imammedo@redhat.com>
> Subject: Re: [edk2] [PATCH] UefiCpuPkg/LocalApicLib: Exclude second
> SendIpi sequence on AMD processors.
> 
> Hello Leo,
> 
> On 06/13/18 22:11, Leo Duran wrote:
> > On AMD processors the second SendIpi in the SendInitSipiSipi and
> > SendInitSipiSipiAllExcludingSelf routines is not required, and may
> > cause undesired side-effects during MP initialization.
> >
> > This patch leverages the StandardSignatureIsAuthenticAMD check to
> > exclude the second SendIpi and its associated MicroSecondDelay (200).
> 
> QEMU and KVM emulate some AMD processors too; of particular interest is
> the recent EPYC addition, I believe (for SME/SEV, minimally).
> 
> Did you check whether the StandardSignatureIsAuthenticAMD() check
> applies to those QEMU VCPU models, and if so, whether omitting the second
> Startup IPI interferes with *V*CPU startup in OVMF guests? (In
> multiprocessing modules, such as CpuMpPei, CpuDxe, and
> PiSmmCpuDxeSmm.)
> 
> Adding Brijesh, Paolo and Igor.
> 
> Thanks!
> Laszlo

Hi Lazlo,

My understanding is that hypervisors simply ignore the second SIPI, so a single (or double) SIPI should be fine.
In any event, I'm checking with Brijesh on your specific question.

Leo.

> 
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Leo Duran <leo.duran@amd.com>
> > Cc: Jordan Justen <jordan.l.justen@intel.com>
> > Cc: Jeff Fan <jeff.fan@intel.com>
> > Cc: Liming Gao <liming.gao@intel.com>
> > ---
> >  UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c             | 12 ++++++++----
> >  UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.c | 12
> > ++++++++----
> >  2 files changed, 16 insertions(+), 8 deletions(-)
> >
> > diff --git a/UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c
> > b/UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c
> > index b0b7e32..6e80536 100644
> > --- a/UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c
> > +++ b/UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c
> > @@ -554,8 +554,10 @@ SendInitSipiSipi (
> >    IcrLow.Bits.DeliveryMode = LOCAL_APIC_DELIVERY_MODE_STARTUP;
> >    IcrLow.Bits.Level = 1;
> >    SendIpi (IcrLow.Uint32, ApicId);
> > -  MicroSecondDelay (200);
> > -  SendIpi (IcrLow.Uint32, ApicId);
> > +  if (!StandardSignatureIsAuthenticAMD()) {
> > +    MicroSecondDelay (200);
> > +    SendIpi (IcrLow.Uint32, ApicId);
> > +  }
> >  }
> >
> >  /**
> > @@ -588,8 +590,10 @@ SendInitSipiSipiAllExcludingSelf (
> >    IcrLow.Bits.Level = 1;
> >    IcrLow.Bits.DestinationShorthand =
> LOCAL_APIC_DESTINATION_SHORTHAND_ALL_EXCLUDING_SELF;
> >    SendIpi (IcrLow.Uint32, 0);
> > -  MicroSecondDelay (200);
> > -  SendIpi (IcrLow.Uint32, 0);
> > +  if (!StandardSignatureIsAuthenticAMD()) {
> > +    MicroSecondDelay (200);
> > +    SendIpi (IcrLow.Uint32, 0);
> > +  }
> >  }
> >
> >  /**
> > diff --git
> > a/UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.c
> > b/UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.c
> > index 1f4dcf7..5d82836 100644
> > --- a/UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.c
> > +++ b/UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.c
> > @@ -649,8 +649,10 @@ SendInitSipiSipi (
> >    IcrLow.Bits.DeliveryMode = LOCAL_APIC_DELIVERY_MODE_STARTUP;
> >    IcrLow.Bits.Level = 1;
> >    SendIpi (IcrLow.Uint32, ApicId);
> > -  MicroSecondDelay (200);
> > -  SendIpi (IcrLow.Uint32, ApicId);
> > +  if (!StandardSignatureIsAuthenticAMD()) {
> > +    MicroSecondDelay (200);
> > +    SendIpi (IcrLow.Uint32, ApicId);
> > +  }
> >  }
> >
> >  /**
> > @@ -683,8 +685,10 @@ SendInitSipiSipiAllExcludingSelf (
> >    IcrLow.Bits.Level = 1;
> >    IcrLow.Bits.DestinationShorthand =
> LOCAL_APIC_DESTINATION_SHORTHAND_ALL_EXCLUDING_SELF;
> >    SendIpi (IcrLow.Uint32, 0);
> > -  MicroSecondDelay (200);
> > -  SendIpi (IcrLow.Uint32, 0);
> > +  if (!StandardSignatureIsAuthenticAMD()) {
> > +    MicroSecondDelay (200);
> > +    SendIpi (IcrLow.Uint32, 0);
> > +  }
> >  }
> >
> >  /**
> >


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] UefiCpuPkg/LocalApicLib: Exclude second SendIpi sequence on AMD processors.
  2018-06-14 14:08     ` Duran, Leo
@ 2018-06-14 14:52       ` Andrew Fish
  2018-06-14 15:00         ` Laszlo Ersek
  0 siblings, 1 reply; 12+ messages in thread
From: Andrew Fish @ 2018-06-14 14:52 UTC (permalink / raw)
  To: Duran, Leo
  Cc: Laszlo Ersek, edk2-devel@lists.01.org, Singh, Brijesh,
	Jordan Justen, Liming Gao, Paolo Bonzini, Jeff Fan



> On Jun 14, 2018, at 7:08 AM, Duran, Leo <leo.duran@amd.com> wrote:
> 
> 
> 
>> -----Original Message-----
>> From: Laszlo Ersek <lersek@redhat.com <mailto:lersek@redhat.com>>
>> Sent: Wednesday, June 13, 2018 3:50 PM
>> To: Duran, Leo <leo.duran@amd.com <mailto:leo.duran@amd.com>>; edk2-devel@lists.01.org <mailto:edk2-devel@lists.01.org>
>> Cc: Jordan Justen <jordan.l.justen@intel.com <mailto:jordan.l.justen@intel.com>>; Jeff Fan
>> <jeff.fan@intel.com <mailto:jeff.fan@intel.com>>; Liming Gao <liming.gao@intel.com <mailto:liming.gao@intel.com>>; Singh, Brijesh
>> <brijesh.singh@amd.com <mailto:brijesh.singh@amd.com>>; Paolo Bonzini <pbonzini@redhat.com <mailto:pbonzini@redhat.com>>; Igor
>> Mammedov <imammedo@redhat.com <mailto:imammedo@redhat.com>>
>> Subject: Re: [edk2] [PATCH] UefiCpuPkg/LocalApicLib: Exclude second
>> SendIpi sequence on AMD processors.
>> 
>> Hello Leo,
>> 
>> On 06/13/18 22:11, Leo Duran wrote:
>>> On AMD processors the second SendIpi in the SendInitSipiSipi and
>>> SendInitSipiSipiAllExcludingSelf routines is not required, and may
>>> cause undesired side-effects during MP initialization.
>>> 
>>> This patch leverages the StandardSignatureIsAuthenticAMD check to
>>> exclude the second SendIpi and its associated MicroSecondDelay (200).
>> 
>> QEMU and KVM emulate some AMD processors too; of particular interest is
>> the recent EPYC addition, I believe (for SME/SEV, minimally).
>> 
>> Did you check whether the StandardSignatureIsAuthenticAMD() check
>> applies to those QEMU VCPU models, and if so, whether omitting the second
>> Startup IPI interferes with *V*CPU startup in OVMF guests? (In
>> multiprocessing modules, such as CpuMpPei, CpuDxe, and
>> PiSmmCpuDxeSmm.)
>> 
>> Adding Brijesh, Paolo and Igor.
>> 
>> Thanks!
>> Laszlo
> 
> Hi Lazlo,
> 
> My understanding is that hypervisors simply ignore the second SIPI, so a single (or double) SIPI should be fine.
> In any event, I'm checking with Brijesh on your specific question.
> 

My understanding is the 2nd SIPI was for an Intel processor bug in the mid 1990's and it has not been required since. People are just scared to change it since all the Operating Systems have been historically validated against INT SIPI SIPI. 

One of my co-works removed our extra SIPI, not knowing the history, and everything worked. Well we booted a little faster without the extra SIPI. 

If people still have the compatibility concern can we make the 2nd SIPI configurable via a PCD. But given the StandardSignatureIsAuthenticAMD() data point we should default the 2nd SIPI to off and move the world forward? What do folks think?

Thanks,

Andrew Fish

PS If I'm remembering correctly Mark Doran (of UEFI Fame) help lead the MP Spec back in the 1990s that 1st documented the INIT SIPI SIPI so we could get some more history from him if needed. 

> Leo.
> 
>> 
>>> 
>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>> Signed-off-by: Leo Duran <leo.duran@amd.com>
>>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>>> Cc: Jeff Fan <jeff.fan@intel.com>
>>> Cc: Liming Gao <liming.gao@intel.com>
>>> ---
>>> UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c             | 12 ++++++++----
>>> UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.c | 12
>>> ++++++++----
>>> 2 files changed, 16 insertions(+), 8 deletions(-)
>>> 
>>> diff --git a/UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c
>>> b/UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c
>>> index b0b7e32..6e80536 100644
>>> --- a/UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c
>>> +++ b/UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c
>>> @@ -554,8 +554,10 @@ SendInitSipiSipi (
>>>   IcrLow.Bits.DeliveryMode = LOCAL_APIC_DELIVERY_MODE_STARTUP;
>>>   IcrLow.Bits.Level = 1;
>>>   SendIpi (IcrLow.Uint32, ApicId);
>>> -  MicroSecondDelay (200);
>>> -  SendIpi (IcrLow.Uint32, ApicId);
>>> +  if (!StandardSignatureIsAuthenticAMD()) {
>>> +    MicroSecondDelay (200);
>>> +    SendIpi (IcrLow.Uint32, ApicId);
>>> +  }
>>> }
>>> 
>>> /**
>>> @@ -588,8 +590,10 @@ SendInitSipiSipiAllExcludingSelf (
>>>   IcrLow.Bits.Level = 1;
>>>   IcrLow.Bits.DestinationShorthand =
>> LOCAL_APIC_DESTINATION_SHORTHAND_ALL_EXCLUDING_SELF;
>>>   SendIpi (IcrLow.Uint32, 0);
>>> -  MicroSecondDelay (200);
>>> -  SendIpi (IcrLow.Uint32, 0);
>>> +  if (!StandardSignatureIsAuthenticAMD()) {
>>> +    MicroSecondDelay (200);
>>> +    SendIpi (IcrLow.Uint32, 0);
>>> +  }
>>> }
>>> 
>>> /**
>>> diff --git
>>> a/UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.c
>>> b/UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.c
>>> index 1f4dcf7..5d82836 100644
>>> --- a/UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.c
>>> +++ b/UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.c
>>> @@ -649,8 +649,10 @@ SendInitSipiSipi (
>>>   IcrLow.Bits.DeliveryMode = LOCAL_APIC_DELIVERY_MODE_STARTUP;
>>>   IcrLow.Bits.Level = 1;
>>>   SendIpi (IcrLow.Uint32, ApicId);
>>> -  MicroSecondDelay (200);
>>> -  SendIpi (IcrLow.Uint32, ApicId);
>>> +  if (!StandardSignatureIsAuthenticAMD()) {
>>> +    MicroSecondDelay (200);
>>> +    SendIpi (IcrLow.Uint32, ApicId);
>>> +  }
>>> }
>>> 
>>> /**
>>> @@ -683,8 +685,10 @@ SendInitSipiSipiAllExcludingSelf (
>>>   IcrLow.Bits.Level = 1;
>>>   IcrLow.Bits.DestinationShorthand =
>> LOCAL_APIC_DESTINATION_SHORTHAND_ALL_EXCLUDING_SELF;
>>>   SendIpi (IcrLow.Uint32, 0);
>>> -  MicroSecondDelay (200);
>>> -  SendIpi (IcrLow.Uint32, 0);
>>> +  if (!StandardSignatureIsAuthenticAMD()) {
>>> +    MicroSecondDelay (200);
>>> +    SendIpi (IcrLow.Uint32, 0);
>>> +  }
>>> }
>>> 
>>> /**
>>> 
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] UefiCpuPkg/LocalApicLib: Exclude second SendIpi sequence on AMD processors.
  2018-06-14 14:52       ` Andrew Fish
@ 2018-06-14 15:00         ` Laszlo Ersek
  2018-06-14 15:47           ` Brijesh Singh
  0 siblings, 1 reply; 12+ messages in thread
From: Laszlo Ersek @ 2018-06-14 15:00 UTC (permalink / raw)
  To: Andrew Fish, Duran, Leo
  Cc: edk2-devel@lists.01.org, Singh, Brijesh, Jordan Justen,
	Liming Gao, Paolo Bonzini, Jeff Fan

On 06/14/18 16:52, Andrew Fish wrote:
> 
> 
>> On Jun 14, 2018, at 7:08 AM, Duran, Leo <leo.duran@amd.com> wrote:
>>
>>
>>
>>> -----Original Message-----
>>> From: Laszlo Ersek <lersek@redhat.com <mailto:lersek@redhat.com>>
>>> Sent: Wednesday, June 13, 2018 3:50 PM
>>> To: Duran, Leo <leo.duran@amd.com <mailto:leo.duran@amd.com>>; edk2-devel@lists.01.org <mailto:edk2-devel@lists.01.org>
>>> Cc: Jordan Justen <jordan.l.justen@intel.com <mailto:jordan.l.justen@intel.com>>; Jeff Fan
>>> <jeff.fan@intel.com <mailto:jeff.fan@intel.com>>; Liming Gao <liming.gao@intel.com <mailto:liming.gao@intel.com>>; Singh, Brijesh
>>> <brijesh.singh@amd.com <mailto:brijesh.singh@amd.com>>; Paolo Bonzini <pbonzini@redhat.com <mailto:pbonzini@redhat.com>>; Igor
>>> Mammedov <imammedo@redhat.com <mailto:imammedo@redhat.com>>
>>> Subject: Re: [edk2] [PATCH] UefiCpuPkg/LocalApicLib: Exclude second
>>> SendIpi sequence on AMD processors.
>>>
>>> Hello Leo,
>>>
>>> On 06/13/18 22:11, Leo Duran wrote:
>>>> On AMD processors the second SendIpi in the SendInitSipiSipi and
>>>> SendInitSipiSipiAllExcludingSelf routines is not required, and may
>>>> cause undesired side-effects during MP initialization.
>>>>
>>>> This patch leverages the StandardSignatureIsAuthenticAMD check to
>>>> exclude the second SendIpi and its associated MicroSecondDelay (200).
>>>
>>> QEMU and KVM emulate some AMD processors too; of particular interest is
>>> the recent EPYC addition, I believe (for SME/SEV, minimally).
>>>
>>> Did you check whether the StandardSignatureIsAuthenticAMD() check
>>> applies to those QEMU VCPU models, and if so, whether omitting the second
>>> Startup IPI interferes with *V*CPU startup in OVMF guests? (In
>>> multiprocessing modules, such as CpuMpPei, CpuDxe, and
>>> PiSmmCpuDxeSmm.)
>>>
>>> Adding Brijesh, Paolo and Igor.
>>>
>>> Thanks!
>>> Laszlo
>>
>> Hi Lazlo,
>>
>> My understanding is that hypervisors simply ignore the second SIPI, so a single (or double) SIPI should be fine.
>> In any event, I'm checking with Brijesh on your specific question.
>>
> 
> My understanding is the 2nd SIPI was for an Intel processor bug in the mid 1990's and it has not been required since. People are just scared to change it since all the Operating Systems have been historically validated against INT SIPI SIPI. 
> 
> One of my co-works removed our extra SIPI, not knowing the history, and everything worked. Well we booted a little faster without the extra SIPI. 
> 
> If people still have the compatibility concern can we make the 2nd SIPI configurable via a PCD. But given the StandardSignatureIsAuthenticAMD() data point we should default the 2nd SIPI to off and move the world forward? What do folks think?

Given that I asked Brijesh to comment too, I'd like to see his feedback
as well (or Leo to forward Brijesh's findings); then I'm OK with
removing the second SIPI.

Thanks
Laszlo


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] UefiCpuPkg/LocalApicLib: Exclude second SendIpi sequence on AMD processors.
  2018-06-14 15:00         ` Laszlo Ersek
@ 2018-06-14 15:47           ` Brijesh Singh
  0 siblings, 0 replies; 12+ messages in thread
From: Brijesh Singh @ 2018-06-14 15:47 UTC (permalink / raw)
  To: Laszlo Ersek, Andrew Fish, Duran, Leo
  Cc: brijesh.singh, edk2-devel@lists.01.org, Jordan Justen, Liming Gao,
	Paolo Bonzini, Jeff Fan



On 06/14/2018 10:00 AM, Laszlo Ersek wrote:
> On 06/14/18 16:52, Andrew Fish wrote:
>>
>>
>>> On Jun 14, 2018, at 7:08 AM, Duran, Leo <leo.duran@amd.com> wrote:
>>>
>>>
>>>
>>>> -----Original Message-----
>>>> From: Laszlo Ersek <lersek@redhat.com <mailto:lersek@redhat.com>>
>>>> Sent: Wednesday, June 13, 2018 3:50 PM
>>>> To: Duran, Leo <leo.duran@amd.com <mailto:leo.duran@amd.com>>; edk2-devel@lists.01.org <mailto:edk2-devel@lists.01.org>
>>>> Cc: Jordan Justen <jordan.l.justen@intel.com <mailto:jordan.l.justen@intel.com>>; Jeff Fan
>>>> <jeff.fan@intel.com <mailto:jeff.fan@intel.com>>; Liming Gao <liming.gao@intel.com <mailto:liming.gao@intel.com>>; Singh, Brijesh
>>>> <brijesh.singh@amd.com <mailto:brijesh.singh@amd.com>>; Paolo Bonzini <pbonzini@redhat.com <mailto:pbonzini@redhat.com>>; Igor
>>>> Mammedov <imammedo@redhat.com <mailto:imammedo@redhat.com>>
>>>> Subject: Re: [edk2] [PATCH] UefiCpuPkg/LocalApicLib: Exclude second
>>>> SendIpi sequence on AMD processors.
>>>>
>>>> Hello Leo,
>>>>
>>>> On 06/13/18 22:11, Leo Duran wrote:
>>>>> On AMD processors the second SendIpi in the SendInitSipiSipi and
>>>>> SendInitSipiSipiAllExcludingSelf routines is not required, and may
>>>>> cause undesired side-effects during MP initialization.
>>>>>
>>>>> This patch leverages the StandardSignatureIsAuthenticAMD check to
>>>>> exclude the second SendIpi and its associated MicroSecondDelay (200).
>>>>
>>>> QEMU and KVM emulate some AMD processors too; of particular interest is
>>>> the recent EPYC addition, I believe (for SME/SEV, minimally).
>>>>
>>>> Did you check whether the StandardSignatureIsAuthenticAMD() check
>>>> applies to those QEMU VCPU models, and if so, whether omitting the second
>>>> Startup IPI interferes with *V*CPU startup in OVMF guests? (In
>>>> multiprocessing modules, such as CpuMpPei, CpuDxe, and
>>>> PiSmmCpuDxeSmm.)
>>>>
>>>> Adding Brijesh, Paolo and Igor.
>>>>
>>>> Thanks!
>>>> Laszlo
>>>
>>> Hi Lazlo,
>>>
>>> My understanding is that hypervisors simply ignore the second SIPI, so a single (or double) SIPI should be fine.
>>> In any event, I'm checking with Brijesh on your specific question.
>>>
>>
>> My understanding is the 2nd SIPI was for an Intel processor bug in the mid 1990's and it has not been required since. People are just scared to change it since all the Operating Systems have been historically validated against INT SIPI SIPI.
>>
>> One of my co-works removed our extra SIPI, not knowing the history, and everything worked. Well we booted a little faster without the extra SIPI.
>>
>> If people still have the compatibility concern can we make the 2nd SIPI configurable via a PCD. But given the StandardSignatureIsAuthenticAMD() data point we should default the 2nd SIPI to off and move the world forward? What do folks think?
> 
> Given that I asked Brijesh to comment too, I'd like to see his feedback
> as well (or Leo to forward Brijesh's findings); then I'm OK with
> removing the second SIPI.
> 


I did a quick test with and without SEV enabled, QEMU seems to be 
working fine. As Leo pointed out that the second SIPI is not required on 
AMD processor.

-Brijesh


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] UefiCpuPkg/LocalApicLib: Exclude second SendIpi sequence on AMD processors.
  2018-06-13 20:11 ` [PATCH] UefiCpuPkg/LocalApicLib: Exclude second SendIpi sequence on AMD processors Leo Duran
  2018-06-13 20:49   ` Laszlo Ersek
@ 2018-06-14 16:36   ` Laszlo Ersek
  2018-06-19  5:20     ` Dong, Eric
  1 sibling, 1 reply; 12+ messages in thread
From: Laszlo Ersek @ 2018-06-14 16:36 UTC (permalink / raw)
  To: Leo Duran, edk2-devel; +Cc: Jordan Justen, Jeff Fan, Liming Gao

On 06/13/18 22:11, Leo Duran wrote:
> On AMD processors the second SendIpi in the SendInitSipiSipi and
> SendInitSipiSipiAllExcludingSelf routines is not required, and may cause
> undesired side-effects during MP initialization.
> 
> This patch leverages the StandardSignatureIsAuthenticAMD check to exclude
> the second SendIpi and its associated MicroSecondDelay (200).
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Leo Duran <leo.duran@amd.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Jeff Fan <jeff.fan@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> ---
>  UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c             | 12 ++++++++----
>  UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.c | 12 ++++++++----
>  2 files changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c b/UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c
> index b0b7e32..6e80536 100644
> --- a/UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c
> +++ b/UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c
> @@ -554,8 +554,10 @@ SendInitSipiSipi (
>    IcrLow.Bits.DeliveryMode = LOCAL_APIC_DELIVERY_MODE_STARTUP;
>    IcrLow.Bits.Level = 1;
>    SendIpi (IcrLow.Uint32, ApicId);
> -  MicroSecondDelay (200);
> -  SendIpi (IcrLow.Uint32, ApicId);
> +  if (!StandardSignatureIsAuthenticAMD()) {
> +    MicroSecondDelay (200);
> +    SendIpi (IcrLow.Uint32, ApicId);
> +  }
>  }
>  
>  /**
> @@ -588,8 +590,10 @@ SendInitSipiSipiAllExcludingSelf (
>    IcrLow.Bits.Level = 1;
>    IcrLow.Bits.DestinationShorthand = LOCAL_APIC_DESTINATION_SHORTHAND_ALL_EXCLUDING_SELF;
>    SendIpi (IcrLow.Uint32, 0);
> -  MicroSecondDelay (200);
> -  SendIpi (IcrLow.Uint32, 0);
> +  if (!StandardSignatureIsAuthenticAMD()) {
> +    MicroSecondDelay (200);
> +    SendIpi (IcrLow.Uint32, 0);
> +  }
>  }
>  
>  /**
> diff --git a/UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.c b/UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.c
> index 1f4dcf7..5d82836 100644
> --- a/UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.c
> +++ b/UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.c
> @@ -649,8 +649,10 @@ SendInitSipiSipi (
>    IcrLow.Bits.DeliveryMode = LOCAL_APIC_DELIVERY_MODE_STARTUP;
>    IcrLow.Bits.Level = 1;
>    SendIpi (IcrLow.Uint32, ApicId);
> -  MicroSecondDelay (200);
> -  SendIpi (IcrLow.Uint32, ApicId);
> +  if (!StandardSignatureIsAuthenticAMD()) {
> +    MicroSecondDelay (200);
> +    SendIpi (IcrLow.Uint32, ApicId);
> +  }
>  }
>  
>  /**
> @@ -683,8 +685,10 @@ SendInitSipiSipiAllExcludingSelf (
>    IcrLow.Bits.Level = 1;
>    IcrLow.Bits.DestinationShorthand = LOCAL_APIC_DESTINATION_SHORTHAND_ALL_EXCLUDING_SELF;
>    SendIpi (IcrLow.Uint32, 0);
> -  MicroSecondDelay (200);
> -  SendIpi (IcrLow.Uint32, 0);
> +  if (!StandardSignatureIsAuthenticAMD()) {
> +    MicroSecondDelay (200);
> +    SendIpi (IcrLow.Uint32, 0);
> +  }
>  }
>  
>  /**
> 

Given all the feedback (thanks all for that!), I'm fine with the patch.

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

I'm only a reviewer for UefiCpuPkg, not a maintainer, so I can't go
ahead and commit the patch just yet. Anyway, I do suggest a small coding
style improvement (which we can do ourselves before we push the patch):
there should be a space character between
"StandardSignatureIsAuthenticAMD" and "()".

Thanks
Laszlo


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] UefiCpuPkg/LocalApicLib: Exclude second SendIpi sequence on AMD processors.
  2018-06-14 16:36   ` Laszlo Ersek
@ 2018-06-19  5:20     ` Dong, Eric
  0 siblings, 0 replies; 12+ messages in thread
From: Dong, Eric @ 2018-06-19  5:20 UTC (permalink / raw)
  To: Laszlo Ersek, Leo Duran, edk2-devel@lists.01.org
  Cc: Justen, Jordan L, Jeff Fan, Gao, Liming

Reviewed-by: Eric Dong <eric.dong@intel.com>  and pushed the patch with the change suggested by Laszlo.

Thanks,
Eric
-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek
Sent: Friday, June 15, 2018 12:36 AM
To: Leo Duran <leo.duran@amd.com>; edk2-devel@lists.01.org
Cc: Justen, Jordan L <jordan.l.justen@intel.com>; Jeff Fan <jeff.fan@intel.com>; Gao, Liming <liming.gao@intel.com>
Subject: Re: [edk2] [PATCH] UefiCpuPkg/LocalApicLib: Exclude second SendIpi sequence on AMD processors.

On 06/13/18 22:11, Leo Duran wrote:
> On AMD processors the second SendIpi in the SendInitSipiSipi and 
> SendInitSipiSipiAllExcludingSelf routines is not required, and may 
> cause undesired side-effects during MP initialization.
> 
> This patch leverages the StandardSignatureIsAuthenticAMD check to 
> exclude the second SendIpi and its associated MicroSecondDelay (200).
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Leo Duran <leo.duran@amd.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Jeff Fan <jeff.fan@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> ---
>  UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c             | 12 ++++++++----
>  UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.c | 12 
> ++++++++----
>  2 files changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c 
> b/UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c
> index b0b7e32..6e80536 100644
> --- a/UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c
> +++ b/UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c
> @@ -554,8 +554,10 @@ SendInitSipiSipi (
>    IcrLow.Bits.DeliveryMode = LOCAL_APIC_DELIVERY_MODE_STARTUP;
>    IcrLow.Bits.Level = 1;
>    SendIpi (IcrLow.Uint32, ApicId);
> -  MicroSecondDelay (200);
> -  SendIpi (IcrLow.Uint32, ApicId);
> +  if (!StandardSignatureIsAuthenticAMD()) {
> +    MicroSecondDelay (200);
> +    SendIpi (IcrLow.Uint32, ApicId);
> +  }
>  }
>  
>  /**
> @@ -588,8 +590,10 @@ SendInitSipiSipiAllExcludingSelf (
>    IcrLow.Bits.Level = 1;
>    IcrLow.Bits.DestinationShorthand = LOCAL_APIC_DESTINATION_SHORTHAND_ALL_EXCLUDING_SELF;
>    SendIpi (IcrLow.Uint32, 0);
> -  MicroSecondDelay (200);
> -  SendIpi (IcrLow.Uint32, 0);
> +  if (!StandardSignatureIsAuthenticAMD()) {
> +    MicroSecondDelay (200);
> +    SendIpi (IcrLow.Uint32, 0);
> +  }
>  }
>  
>  /**
> diff --git 
> a/UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.c 
> b/UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.c
> index 1f4dcf7..5d82836 100644
> --- a/UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.c
> +++ b/UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.c
> @@ -649,8 +649,10 @@ SendInitSipiSipi (
>    IcrLow.Bits.DeliveryMode = LOCAL_APIC_DELIVERY_MODE_STARTUP;
>    IcrLow.Bits.Level = 1;
>    SendIpi (IcrLow.Uint32, ApicId);
> -  MicroSecondDelay (200);
> -  SendIpi (IcrLow.Uint32, ApicId);
> +  if (!StandardSignatureIsAuthenticAMD()) {
> +    MicroSecondDelay (200);
> +    SendIpi (IcrLow.Uint32, ApicId);
> +  }
>  }
>  
>  /**
> @@ -683,8 +685,10 @@ SendInitSipiSipiAllExcludingSelf (
>    IcrLow.Bits.Level = 1;
>    IcrLow.Bits.DestinationShorthand = LOCAL_APIC_DESTINATION_SHORTHAND_ALL_EXCLUDING_SELF;
>    SendIpi (IcrLow.Uint32, 0);
> -  MicroSecondDelay (200);
> -  SendIpi (IcrLow.Uint32, 0);
> +  if (!StandardSignatureIsAuthenticAMD()) {
> +    MicroSecondDelay (200);
> +    SendIpi (IcrLow.Uint32, 0);
> +  }
>  }
>  
>  /**
> 

Given all the feedback (thanks all for that!), I'm fine with the patch.

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

I'm only a reviewer for UefiCpuPkg, not a maintainer, so I can't go ahead and commit the patch just yet. Anyway, I do suggest a small coding style improvement (which we can do ourselves before we push the patch):
there should be a space character between "StandardSignatureIsAuthenticAMD" and "()".

Thanks
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel


^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2018-06-19  5:20 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-06-13 20:11 [PATCH] UefiCpuPkg/LocalApicLib: Exclude second SendIpi on AMD Leo Duran
2018-06-13 20:11 ` [PATCH] UefiCpuPkg/LocalApicLib: Exclude second SendIpi sequence on AMD processors Leo Duran
2018-06-13 20:49   ` Laszlo Ersek
2018-06-13 20:52     ` Paolo Bonzini
2018-06-14  5:39       ` Ni, Ruiyu
2018-06-14  9:04         ` Paolo Bonzini
2018-06-14 14:08     ` Duran, Leo
2018-06-14 14:52       ` Andrew Fish
2018-06-14 15:00         ` Laszlo Ersek
2018-06-14 15:47           ` Brijesh Singh
2018-06-14 16:36   ` Laszlo Ersek
2018-06-19  5:20     ` Dong, Eric

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox