* [PATCH] UefiCpuPkg/SecCore: Add pre-memory AP vector
@ 2020-06-01 22:32 Deric Cole
2020-06-02 4:50 ` Ni, Ray
2020-06-02 12:53 ` Laszlo Ersek
0 siblings, 2 replies; 6+ messages in thread
From: Deric Cole @ 2020-06-01 22:32 UTC (permalink / raw)
To: devel; +Cc: Deric Cole, Eric Dong, Ray Ni, Laszlo Ersek
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2776
Add a vector at 0xFF000 (0xFFFFF000) that can be used by Init-SIPI-SIPI
to start an AP before memory is initialized. This vector jumps into the
same SEC entry point as the ordinary reset vector, with a special value
of "AP" in the DI register. The platform-specific SEC code is expected
to check for that value and take a different path for APs, if this
feature is supported by the platform.
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Deric Cole <deric.cole@intel.com>
---
UefiCpuPkg/SecCore/Ia32/ResetVec.nasmb | 31 ++++++++++++++++++++++++-------
1 file changed, 24 insertions(+), 7 deletions(-)
diff --git a/UefiCpuPkg/SecCore/Ia32/ResetVec.nasmb b/UefiCpuPkg/SecCore/Ia32/ResetVec.nasmb
index f41b9669d0..1dfc4efe4c 100644
--- a/UefiCpuPkg/SecCore/Ia32/ResetVec.nasmb
+++ b/UefiCpuPkg/SecCore/Ia32/ResetVec.nasmb
@@ -10,7 +10,7 @@
; Abstract:
;
; Reset Vector Data structure
-; This structure is located at 0xFFFFFFC0
+; This structure is located at 0xFFFFF000
;
;------------------------------------------------------------------------------
@@ -23,19 +23,36 @@ USE16
;
ORG 0h
+
+;
+; 0xFFFFF000
+;
+; We enter here with CS:IP = 0xFF00:0x0000. Do a far-jump to change CS to 0xF000
+; and IP to ApStartup.
+;
+ApVector:
+ mov di, "AP"
+ jmp 0xF000:0xF000+ApStartup
+
+ TIMES 0xFC0-($-$$) nop
+
+;
+; This should be at 0xFFFFFFC0
+;
+
;
; Reserved
;
ReservedData: DD 0eeeeeeeeh, 0eeeeeeeeh
- TIMES 0x10-($-$$) DB 0
+ TIMES 0xFD0-($-$$) nop
;
-; This is located at 0xFFFFFFD0h
+; This is located at 0xFFFFFFD0
;
mov di, "PA"
jmp ApStartup
- TIMES 0x20-($-$$) DB 0
+ TIMES 0xFE0-($-$$) nop
;
; Pointer to the entry point of the PEI core
; It is located at 0xFFFFFFE0, and is fixed up by some build tool
@@ -53,7 +70,7 @@ ASM_PFX(InterruptHandler):
jmp $
iret
- TIMES 0x30-($-$$) DB 0
+ TIMES 0xFF0-($-$$) nop
;
; For IA32, the reset vector must be at 0xFFFFFFF0, i.e., 4G-16 byte
; Execution starts here upon power-on/platform-reset.
@@ -74,7 +91,7 @@ ApStartup:
DW -3
- TIMES 0x38-($-$$) DB 0
+ TIMES 0xFF8-($-$$) nop
;
; Ap reset vector segment address is at 0xFFFFFFF8
; This will be fixed up by some build tool,
@@ -83,7 +100,7 @@ ApStartup:
;
ApSegAddress: dd 12345678h
- TIMES 0x3c-($-$$) DB 0
+ TIMES 0xFFC-($-$$) nop
;
; BFV Base is at 0xFFFFFFFC
; This will be fixed up by some build tool,
--
2.26.2.windows.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] UefiCpuPkg/SecCore: Add pre-memory AP vector
2020-06-01 22:32 [PATCH] UefiCpuPkg/SecCore: Add pre-memory AP vector Deric Cole
@ 2020-06-02 4:50 ` Ni, Ray
2020-06-02 16:41 ` Cole, Deric
2020-06-02 12:53 ` Laszlo Ersek
1 sibling, 1 reply; 6+ messages in thread
From: Ni, Ray @ 2020-06-02 4:50 UTC (permalink / raw)
To: Cole, Deric, devel@edk2.groups.io; +Cc: Dong, Eric, Laszlo Ersek
Deric,
Can you explain why changing all padding 0x0 to 0x90 (nop) in your patch?
Is it required to enable AP start up in pre-mem?
Thanks,
Ray
> -----Original Message-----
> From: Cole, Deric <deric.cole@intel.com>
> Sent: Tuesday, June 2, 2020 6:32 AM
> To: devel@edk2.groups.io
> Cc: Cole, Deric <deric.cole@intel.com>; Dong, Eric <eric.dong@intel.com>; Ni,
> Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>
> Subject: [PATCH] UefiCpuPkg/SecCore: Add pre-memory AP vector
>
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2776
>
> Add a vector at 0xFF000 (0xFFFFF000) that can be used by Init-SIPI-SIPI
> to start an AP before memory is initialized. This vector jumps into the
> same SEC entry point as the ordinary reset vector, with a special value
> of "AP" in the DI register. The platform-specific SEC code is expected
> to check for that value and take a different path for APs, if this
> feature is supported by the platform.
>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Deric Cole <deric.cole@intel.com>
> ---
> UefiCpuPkg/SecCore/Ia32/ResetVec.nasmb | 31
> ++++++++++++++++++++++++-------
> 1 file changed, 24 insertions(+), 7 deletions(-)
>
> diff --git a/UefiCpuPkg/SecCore/Ia32/ResetVec.nasmb
> b/UefiCpuPkg/SecCore/Ia32/ResetVec.nasmb
> index f41b9669d0..1dfc4efe4c 100644
> --- a/UefiCpuPkg/SecCore/Ia32/ResetVec.nasmb
> +++ b/UefiCpuPkg/SecCore/Ia32/ResetVec.nasmb
> @@ -10,7 +10,7 @@
> ; Abstract:
>
> ;
>
> ; Reset Vector Data structure
>
> -; This structure is located at 0xFFFFFFC0
>
> +; This structure is located at 0xFFFFF000
>
> ;
>
> ;------------------------------------------------------------------------------
>
>
>
> @@ -23,19 +23,36 @@ USE16
> ;
>
>
>
> ORG 0h
>
> +
>
> +;
>
> +; 0xFFFFF000
>
> +;
>
> +; We enter here with CS:IP = 0xFF00:0x0000. Do a far-jump to change CS to
> 0xF000
>
> +; and IP to ApStartup.
>
> +;
>
> +ApVector:
>
> + mov di, "AP"
>
> + jmp 0xF000:0xF000+ApStartup
>
> +
>
> + TIMES 0xFC0-($-$$) nop
>
> +
>
> +;
>
> +; This should be at 0xFFFFFFC0
>
> +;
>
> +
>
> ;
>
> ; Reserved
>
> ;
>
> ReservedData: DD 0eeeeeeeeh, 0eeeeeeeeh
>
>
>
> - TIMES 0x10-($-$$) DB 0
>
> + TIMES 0xFD0-($-$$) nop
>
> ;
>
> -; This is located at 0xFFFFFFD0h
>
> +; This is located at 0xFFFFFFD0
>
> ;
>
> mov di, "PA"
>
> jmp ApStartup
>
>
>
> - TIMES 0x20-($-$$) DB 0
>
> + TIMES 0xFE0-($-$$) nop
>
> ;
>
> ; Pointer to the entry point of the PEI core
>
> ; It is located at 0xFFFFFFE0, and is fixed up by some build tool
>
> @@ -53,7 +70,7 @@ ASM_PFX(InterruptHandler):
> jmp $
>
> iret
>
>
>
> - TIMES 0x30-($-$$) DB 0
>
> + TIMES 0xFF0-($-$$) nop
>
> ;
>
> ; For IA32, the reset vector must be at 0xFFFFFFF0, i.e., 4G-16 byte
>
> ; Execution starts here upon power-on/platform-reset.
>
> @@ -74,7 +91,7 @@ ApStartup:
> DW -3
>
>
>
>
>
> - TIMES 0x38-($-$$) DB 0
>
> + TIMES 0xFF8-($-$$) nop
>
> ;
>
> ; Ap reset vector segment address is at 0xFFFFFFF8
>
> ; This will be fixed up by some build tool,
>
> @@ -83,7 +100,7 @@ ApStartup:
> ;
>
> ApSegAddress: dd 12345678h
>
>
>
> - TIMES 0x3c-($-$$) DB 0
>
> + TIMES 0xFFC-($-$$) nop
>
> ;
>
> ; BFV Base is at 0xFFFFFFFC
>
> ; This will be fixed up by some build tool,
>
> --
> 2.26.2.windows.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] UefiCpuPkg/SecCore: Add pre-memory AP vector
2020-06-01 22:32 [PATCH] UefiCpuPkg/SecCore: Add pre-memory AP vector Deric Cole
2020-06-02 4:50 ` Ni, Ray
@ 2020-06-02 12:53 ` Laszlo Ersek
1 sibling, 0 replies; 6+ messages in thread
From: Laszlo Ersek @ 2020-06-02 12:53 UTC (permalink / raw)
To: Deric Cole, devel; +Cc: Eric Dong, Ray Ni
On 06/02/20 00:32, Deric Cole wrote:
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2776
>
> Add a vector at 0xFF000 (0xFFFFF000) that can be used by Init-SIPI-SIPI
> to start an AP before memory is initialized. This vector jumps into the
> same SEC entry point as the ordinary reset vector, with a special value
> of "AP" in the DI register. The platform-specific SEC code is expected
> to check for that value and take a different path for APs, if this
> feature is supported by the platform.
>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Deric Cole <deric.cole@intel.com>
> ---
> UefiCpuPkg/SecCore/Ia32/ResetVec.nasmb | 31 ++++++++++++++++++++++++-------
> 1 file changed, 24 insertions(+), 7 deletions(-)
I'll let Ray and Eric review this.
Thanks
Laszlo
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] UefiCpuPkg/SecCore: Add pre-memory AP vector
2020-06-02 4:50 ` Ni, Ray
@ 2020-06-02 16:41 ` Cole, Deric
2020-06-23 0:56 ` Ni, Ray
0 siblings, 1 reply; 6+ messages in thread
From: Cole, Deric @ 2020-06-02 16:41 UTC (permalink / raw)
To: Ni, Ray, devel@edk2.groups.io; +Cc: Dong, Eric, Laszlo Ersek
Ray,
The NOP is not a functional change, it's just for ease of debug.
Before, this file was padding with zeros in various places. When viewing this memory using a disassembler, two subsequent zero-bytes show up as an ADD instruction, which I found confusing. But worse, if the number of zero-bytes was odd, the disassembler might try to "consume" part of the next (real) instruction as an operand to the last hypothetical ADD.
Since NOP is a 1-byte instruction, I used that instead, so it is easier to visually identify the real code versus the padding when viewing disassembly.
-Deric
-----Original Message-----
From: Ni, Ray <ray.ni@intel.com>
Sent: Monday, June 1, 2020 9:51 PM
To: Cole, Deric <deric.cole@intel.com>; devel@edk2.groups.io
Cc: Dong, Eric <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com>
Subject: RE: [PATCH] UefiCpuPkg/SecCore: Add pre-memory AP vector
Deric,
Can you explain why changing all padding 0x0 to 0x90 (nop) in your patch?
Is it required to enable AP start up in pre-mem?
Thanks,
Ray
> -----Original Message-----
> From: Cole, Deric <deric.cole@intel.com>
> Sent: Tuesday, June 2, 2020 6:32 AM
> To: devel@edk2.groups.io
> Cc: Cole, Deric <deric.cole@intel.com>; Dong, Eric
> <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Laszlo Ersek
> <lersek@redhat.com>
> Subject: [PATCH] UefiCpuPkg/SecCore: Add pre-memory AP vector
>
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2776
>
> Add a vector at 0xFF000 (0xFFFFF000) that can be used by
> Init-SIPI-SIPI to start an AP before memory is initialized. This
> vector jumps into the same SEC entry point as the ordinary reset
> vector, with a special value of "AP" in the DI register. The
> platform-specific SEC code is expected to check for that value and
> take a different path for APs, if this feature is supported by the platform.
>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Deric Cole <deric.cole@intel.com>
> ---
> UefiCpuPkg/SecCore/Ia32/ResetVec.nasmb | 31
> ++++++++++++++++++++++++-------
> 1 file changed, 24 insertions(+), 7 deletions(-)
>
> diff --git a/UefiCpuPkg/SecCore/Ia32/ResetVec.nasmb
> b/UefiCpuPkg/SecCore/Ia32/ResetVec.nasmb
> index f41b9669d0..1dfc4efe4c 100644
> --- a/UefiCpuPkg/SecCore/Ia32/ResetVec.nasmb
> +++ b/UefiCpuPkg/SecCore/Ia32/ResetVec.nasmb
> @@ -10,7 +10,7 @@
> ; Abstract:
>
> ;
>
> ; Reset Vector Data structure
>
> -; This structure is located at 0xFFFFFFC0
>
> +; This structure is located at 0xFFFFF000
>
> ;
>
>
> ;---------------------------------------------------------------------
> ---------
>
>
>
> @@ -23,19 +23,36 @@ USE16
> ;
>
>
>
> ORG 0h
>
> +
>
> +;
>
> +; 0xFFFFF000
>
> +;
>
> +; We enter here with CS:IP = 0xFF00:0x0000. Do a far-jump to change
> +CS to
> 0xF000
>
> +; and IP to ApStartup.
>
> +;
>
> +ApVector:
>
> + mov di, "AP"
>
> + jmp 0xF000:0xF000+ApStartup
>
> +
>
> + TIMES 0xFC0-($-$$) nop
>
> +
>
> +;
>
> +; This should be at 0xFFFFFFC0
>
> +;
>
> +
>
> ;
>
> ; Reserved
>
> ;
>
> ReservedData: DD 0eeeeeeeeh, 0eeeeeeeeh
>
>
>
> - TIMES 0x10-($-$$) DB 0
>
> + TIMES 0xFD0-($-$$) nop
>
> ;
>
> -; This is located at 0xFFFFFFD0h
>
> +; This is located at 0xFFFFFFD0
>
> ;
>
> mov di, "PA"
>
> jmp ApStartup
>
>
>
> - TIMES 0x20-($-$$) DB 0
>
> + TIMES 0xFE0-($-$$) nop
>
> ;
>
> ; Pointer to the entry point of the PEI core
>
> ; It is located at 0xFFFFFFE0, and is fixed up by some build tool
>
> @@ -53,7 +70,7 @@ ASM_PFX(InterruptHandler):
> jmp $
>
> iret
>
>
>
> - TIMES 0x30-($-$$) DB 0
>
> + TIMES 0xFF0-($-$$) nop
>
> ;
>
> ; For IA32, the reset vector must be at 0xFFFFFFF0, i.e., 4G-16 byte
>
> ; Execution starts here upon power-on/platform-reset.
>
> @@ -74,7 +91,7 @@ ApStartup:
> DW -3
>
>
>
>
>
> - TIMES 0x38-($-$$) DB 0
>
> + TIMES 0xFF8-($-$$) nop
>
> ;
>
> ; Ap reset vector segment address is at 0xFFFFFFF8
>
> ; This will be fixed up by some build tool,
>
> @@ -83,7 +100,7 @@ ApStartup:
> ;
>
> ApSegAddress: dd 12345678h
>
>
>
> - TIMES 0x3c-($-$$) DB 0
>
> + TIMES 0xFFC-($-$$) nop
>
> ;
>
> ; BFV Base is at 0xFFFFFFFC
>
> ; This will be fixed up by some build tool,
>
> --
> 2.26.2.windows.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] UefiCpuPkg/SecCore: Add pre-memory AP vector
2020-06-02 16:41 ` Cole, Deric
@ 2020-06-23 0:56 ` Ni, Ray
2020-06-23 2:14 ` Dong, Eric
0 siblings, 1 reply; 6+ messages in thread
From: Ni, Ray @ 2020-06-23 0:56 UTC (permalink / raw)
To: Cole, Deric, devel@edk2.groups.io; +Cc: Dong, Eric, Laszlo Ersek
Reviewed-by: Ray Ni <ray.ni@intel.com>
> -----Original Message-----
> From: Cole, Deric <deric.cole@intel.com>
> Sent: Wednesday, June 3, 2020 12:42 AM
> To: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io
> Cc: Dong, Eric <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com>
> Subject: RE: [PATCH] UefiCpuPkg/SecCore: Add pre-memory AP vector
>
> Ray,
>
> The NOP is not a functional change, it's just for ease of debug.
>
> Before, this file was padding with zeros in various places. When viewing this memory using a disassembler, two
> subsequent zero-bytes show up as an ADD instruction, which I found confusing. But worse, if the number of zero-bytes was
> odd, the disassembler might try to "consume" part of the next (real) instruction as an operand to the last hypothetical
> ADD.
>
> Since NOP is a 1-byte instruction, I used that instead, so it is easier to visually identify the real code versus the padding
> when viewing disassembly.
>
> -Deric
>
> -----Original Message-----
> From: Ni, Ray <ray.ni@intel.com>
> Sent: Monday, June 1, 2020 9:51 PM
> To: Cole, Deric <deric.cole@intel.com>; devel@edk2.groups.io
> Cc: Dong, Eric <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com>
> Subject: RE: [PATCH] UefiCpuPkg/SecCore: Add pre-memory AP vector
>
> Deric,
> Can you explain why changing all padding 0x0 to 0x90 (nop) in your patch?
>
> Is it required to enable AP start up in pre-mem?
>
> Thanks,
> Ray
>
> > -----Original Message-----
> > From: Cole, Deric <deric.cole@intel.com>
> > Sent: Tuesday, June 2, 2020 6:32 AM
> > To: devel@edk2.groups.io
> > Cc: Cole, Deric <deric.cole@intel.com>; Dong, Eric
> > <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Laszlo Ersek
> > <lersek@redhat.com>
> > Subject: [PATCH] UefiCpuPkg/SecCore: Add pre-memory AP vector
> >
> > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2776
> >
> > Add a vector at 0xFF000 (0xFFFFF000) that can be used by
> > Init-SIPI-SIPI to start an AP before memory is initialized. This
> > vector jumps into the same SEC entry point as the ordinary reset
> > vector, with a special value of "AP" in the DI register. The
> > platform-specific SEC code is expected to check for that value and
> > take a different path for APs, if this feature is supported by the platform.
> >
> > Cc: Eric Dong <eric.dong@intel.com>
> > Cc: Ray Ni <ray.ni@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Signed-off-by: Deric Cole <deric.cole@intel.com>
> > ---
> > UefiCpuPkg/SecCore/Ia32/ResetVec.nasmb | 31
> > ++++++++++++++++++++++++-------
> > 1 file changed, 24 insertions(+), 7 deletions(-)
> >
> > diff --git a/UefiCpuPkg/SecCore/Ia32/ResetVec.nasmb
> > b/UefiCpuPkg/SecCore/Ia32/ResetVec.nasmb
> > index f41b9669d0..1dfc4efe4c 100644
> > --- a/UefiCpuPkg/SecCore/Ia32/ResetVec.nasmb
> > +++ b/UefiCpuPkg/SecCore/Ia32/ResetVec.nasmb
> > @@ -10,7 +10,7 @@
> > ; Abstract:
> >
> > ;
> >
> > ; Reset Vector Data structure
> >
> > -; This structure is located at 0xFFFFFFC0
> >
> > +; This structure is located at 0xFFFFF000
> >
> > ;
> >
> >
> > ;---------------------------------------------------------------------
> > ---------
> >
> >
> >
> > @@ -23,19 +23,36 @@ USE16
> > ;
> >
> >
> >
> > ORG 0h
> >
> > +
> >
> > +;
> >
> > +; 0xFFFFF000
> >
> > +;
> >
> > +; We enter here with CS:IP = 0xFF00:0x0000. Do a far-jump to change
> > +CS to
> > 0xF000
> >
> > +; and IP to ApStartup.
> >
> > +;
> >
> > +ApVector:
> >
> > + mov di, "AP"
> >
> > + jmp 0xF000:0xF000+ApStartup
> >
> > +
> >
> > + TIMES 0xFC0-($-$$) nop
> >
> > +
> >
> > +;
> >
> > +; This should be at 0xFFFFFFC0
> >
> > +;
> >
> > +
> >
> > ;
> >
> > ; Reserved
> >
> > ;
> >
> > ReservedData: DD 0eeeeeeeeh, 0eeeeeeeeh
> >
> >
> >
> > - TIMES 0x10-($-$$) DB 0
> >
> > + TIMES 0xFD0-($-$$) nop
> >
> > ;
> >
> > -; This is located at 0xFFFFFFD0h
> >
> > +; This is located at 0xFFFFFFD0
> >
> > ;
> >
> > mov di, "PA"
> >
> > jmp ApStartup
> >
> >
> >
> > - TIMES 0x20-($-$$) DB 0
> >
> > + TIMES 0xFE0-($-$$) nop
> >
> > ;
> >
> > ; Pointer to the entry point of the PEI core
> >
> > ; It is located at 0xFFFFFFE0, and is fixed up by some build tool
> >
> > @@ -53,7 +70,7 @@ ASM_PFX(InterruptHandler):
> > jmp $
> >
> > iret
> >
> >
> >
> > - TIMES 0x30-($-$$) DB 0
> >
> > + TIMES 0xFF0-($-$$) nop
> >
> > ;
> >
> > ; For IA32, the reset vector must be at 0xFFFFFFF0, i.e., 4G-16 byte
> >
> > ; Execution starts here upon power-on/platform-reset.
> >
> > @@ -74,7 +91,7 @@ ApStartup:
> > DW -3
> >
> >
> >
> >
> >
> > - TIMES 0x38-($-$$) DB 0
> >
> > + TIMES 0xFF8-($-$$) nop
> >
> > ;
> >
> > ; Ap reset vector segment address is at 0xFFFFFFF8
> >
> > ; This will be fixed up by some build tool,
> >
> > @@ -83,7 +100,7 @@ ApStartup:
> > ;
> >
> > ApSegAddress: dd 12345678h
> >
> >
> >
> > - TIMES 0x3c-($-$$) DB 0
> >
> > + TIMES 0xFFC-($-$$) nop
> >
> > ;
> >
> > ; BFV Base is at 0xFFFFFFFC
> >
> > ; This will be fixed up by some build tool,
> >
> > --
> > 2.26.2.windows.1
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] UefiCpuPkg/SecCore: Add pre-memory AP vector
2020-06-23 0:56 ` Ni, Ray
@ 2020-06-23 2:14 ` Dong, Eric
0 siblings, 0 replies; 6+ messages in thread
From: Dong, Eric @ 2020-06-23 2:14 UTC (permalink / raw)
To: Ni, Ray, Cole, Deric, devel@edk2.groups.io; +Cc: Laszlo Ersek
[-- Attachment #1: Type: text/plain, Size: 6435 bytes --]
Reviewed-by: Eric Dong eric.dong@intel.com<mailto:eric.dong@intel.com>
Pushed:
SHA-1: 00b8bf7eda00fb6f0197d3968b6078cfdb4870fa
* UefiCpuPkg/SecCore: Add pre-memory AP vector
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2776
Thanks,
Eric
From: Ni, Ray <ray.ni@intel.com>
Sent: Tuesday, June 23, 2020 8:57 AM
To: Cole, Deric <deric.cole@intel.com>; devel@edk2.groups.io
Cc: Dong, Eric <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com>
Subject: RE: [PATCH] UefiCpuPkg/SecCore: Add pre-memory AP vector
Reviewed-by: Ray Ni <ray.ni@intel.com<mailto:ray.ni@intel.com>>
> -----Original Message-----
> From: Cole, Deric <deric.cole@intel.com<mailto:deric.cole@intel.com>>
> Sent: Wednesday, June 3, 2020 12:42 AM
> To: Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>
> Cc: Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>
> Subject: RE: [PATCH] UefiCpuPkg/SecCore: Add pre-memory AP vector
>
> Ray,
>
> The NOP is not a functional change, it's just for ease of debug.
>
> Before, this file was padding with zeros in various places. When viewing this memory using a disassembler, two
> subsequent zero-bytes show up as an ADD instruction, which I found confusing. But worse, if the number of zero-bytes was
> odd, the disassembler might try to "consume" part of the next (real) instruction as an operand to the last hypothetical
> ADD.
>
> Since NOP is a 1-byte instruction, I used that instead, so it is easier to visually identify the real code versus the padding
> when viewing disassembly.
>
> -Deric
>
> -----Original Message-----
> From: Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>
> Sent: Monday, June 1, 2020 9:51 PM
> To: Cole, Deric <deric.cole@intel.com<mailto:deric.cole@intel.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>
> Cc: Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>
> Subject: RE: [PATCH] UefiCpuPkg/SecCore: Add pre-memory AP vector
>
> Deric,
> Can you explain why changing all padding 0x0 to 0x90 (nop) in your patch?
>
> Is it required to enable AP start up in pre-mem?
>
> Thanks,
> Ray
>
> > -----Original Message-----
> > From: Cole, Deric <deric.cole@intel.com<mailto:deric.cole@intel.com>>
> > Sent: Tuesday, June 2, 2020 6:32 AM
> > To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>
> > Cc: Cole, Deric <deric.cole@intel.com<mailto:deric.cole@intel.com>>; Dong, Eric
> > <eric.dong@intel.com<mailto:eric.dong@intel.com>>; Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>; Laszlo Ersek
> > <lersek@redhat.com<mailto:lersek@redhat.com>>
> > Subject: [PATCH] UefiCpuPkg/SecCore: Add pre-memory AP vector
> >
> > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2776
> >
> > Add a vector at 0xFF000 (0xFFFFF000) that can be used by
> > Init-SIPI-SIPI to start an AP before memory is initialized. This
> > vector jumps into the same SEC entry point as the ordinary reset
> > vector, with a special value of "AP" in the DI register. The
> > platform-specific SEC code is expected to check for that value and
> > take a different path for APs, if this feature is supported by the platform.
> >
> > Cc: Eric Dong <eric.dong@intel.com<mailto:eric.dong@intel.com>>
> > Cc: Ray Ni <ray.ni@intel.com<mailto:ray.ni@intel.com>>
> > Cc: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>
> > Signed-off-by: Deric Cole <deric.cole@intel.com<mailto:deric.cole@intel.com>>
> > ---
> > UefiCpuPkg/SecCore/Ia32/ResetVec.nasmb | 31
> > ++++++++++++++++++++++++-------
> > 1 file changed, 24 insertions(+), 7 deletions(-)
> >
> > diff --git a/UefiCpuPkg/SecCore/Ia32/ResetVec.nasmb
> > b/UefiCpuPkg/SecCore/Ia32/ResetVec.nasmb
> > index f41b9669d0..1dfc4efe4c 100644
> > --- a/UefiCpuPkg/SecCore/Ia32/ResetVec.nasmb
> > +++ b/UefiCpuPkg/SecCore/Ia32/ResetVec.nasmb
> > @@ -10,7 +10,7 @@
> > ; Abstract:
> >
> > ;
> >
> > ; Reset Vector Data structure
> >
> > -; This structure is located at 0xFFFFFFC0
> >
> > +; This structure is located at 0xFFFFF000
> >
> > ;
> >
> >
> > ;---------------------------------------------------------------------
> > ---------
> >
> >
> >
> > @@ -23,19 +23,36 @@ USE16
> > ;
> >
> >
> >
> > ORG 0h
> >
> > +
> >
> > +;
> >
> > +; 0xFFFFF000
> >
> > +;
> >
> > +; We enter here with CS:IP = 0xFF00:0x0000. Do a far-jump to change
> > +CS to
> > 0xF000
> >
> > +; and IP to ApStartup.
> >
> > +;
> >
> > +ApVector:
> >
> > + mov di, "AP"
> >
> > + jmp 0xF000:0xF000+ApStartup
> >
> > +
> >
> > + TIMES 0xFC0-($-$$) nop
> >
> > +
> >
> > +;
> >
> > +; This should be at 0xFFFFFFC0
> >
> > +;
> >
> > +
> >
> > ;
> >
> > ; Reserved
> >
> > ;
> >
> > ReservedData: DD 0eeeeeeeeh, 0eeeeeeeeh
> >
> >
> >
> > - TIMES 0x10-($-$$) DB 0
> >
> > + TIMES 0xFD0-($-$$) nop
> >
> > ;
> >
> > -; This is located at 0xFFFFFFD0h
> >
> > +; This is located at 0xFFFFFFD0
> >
> > ;
> >
> > mov di, "PA"
> >
> > jmp ApStartup
> >
> >
> >
> > - TIMES 0x20-($-$$) DB 0
> >
> > + TIMES 0xFE0-($-$$) nop
> >
> > ;
> >
> > ; Pointer to the entry point of the PEI core
> >
> > ; It is located at 0xFFFFFFE0, and is fixed up by some build tool
> >
> > @@ -53,7 +70,7 @@ ASM_PFX(InterruptHandler):
> > jmp $
> >
> > iret
> >
> >
> >
> > - TIMES 0x30-($-$$) DB 0
> >
> > + TIMES 0xFF0-($-$$) nop
> >
> > ;
> >
> > ; For IA32, the reset vector must be at 0xFFFFFFF0, i.e., 4G-16 byte
> >
> > ; Execution starts here upon power-on/platform-reset.
> >
> > @@ -74,7 +91,7 @@ ApStartup:
> > DW -3
> >
> >
> >
> >
> >
> > - TIMES 0x38-($-$$) DB 0
> >
> > + TIMES 0xFF8-($-$$) nop
> >
> > ;
> >
> > ; Ap reset vector segment address is at 0xFFFFFFF8
> >
> > ; This will be fixed up by some build tool,
> >
> > @@ -83,7 +100,7 @@ ApStartup:
> > ;
> >
> > ApSegAddress: dd 12345678h
> >
> >
> >
> > - TIMES 0x3c-($-$$) DB 0
> >
> > + TIMES 0xFFC-($-$$) nop
> >
> > ;
> >
> > ; BFV Base is at 0xFFFFFFFC
> >
> > ; This will be fixed up by some build tool,
> >
> > --
> > 2.26.2.windows.1
>
>
[-- Attachment #2: Type: text/html, Size: 24674 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-06-23 2:14 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-06-01 22:32 [PATCH] UefiCpuPkg/SecCore: Add pre-memory AP vector Deric Cole
2020-06-02 4:50 ` Ni, Ray
2020-06-02 16:41 ` Cole, Deric
2020-06-23 0:56 ` Ni, Ray
2020-06-23 2:14 ` Dong, Eric
2020-06-02 12:53 ` Laszlo Ersek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox