OvmfPkg/OvmfPkgIa32.fdf | 2 +- OvmfPkg/OvmfPkgIa32X64.fdf | 2 +- OvmfPkg/OvmfPkgX64.fdf | 2 +- OvmfPkg/Sec/SecMain.inf | 1 + OvmfPkg/Sec/Ia32/SecEntry.nasm | 19 ++++++++++++++++--- OvmfPkg/Sec/X64/SecEntry.nasm | 15 +++++++++++++++ 6 files changed, 35 insertions(+), 6 deletions(-)
The first three patches enable the PEI_CORE to report OVMF's temp
SEC/PEI stack and heap usage.
- This depends on the new fixed PCD "PcdInitValueInTempStack",
recently added for
<https://bugzilla.tianocore.org/show_bug.cgi?id=740>
("INIT_CAR_VALUE should be defined in a central location").
- Ard recently implemented the same in ArmPlatformPkg, for
<https://bugzilla.tianocore.org/show_bug.cgi?id=748> ("measure temp
SEC/PEI stack usage").
The last (fourth) patch adapts OVMF to the larger MtrrLib stack demand
that originates from commit 2bbd7e2fbd4b ("UefiCpuPkg/MtrrLib: Update
algorithm to calculate optimal settings", 2017-09-27). OVMF's temp RAM
size is restored to the historical / original 64KB.
This work is tracked in
<https://bugzilla.tianocore.org/show_bug.cgi?id=747> ("measure temp
SEC/PEI stack usage; grow temp SEC/PEI RAM"), with links to related
mailing list discussions.
Repo: https://github.com/lersek/edk2.git
Branch: temp_ram_tweaks
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Thanks
Laszlo
Laszlo Ersek (4):
OvmfPkg/Sec/Ia32: free up EAX for other uses while setting up the
stack
OvmfPkg/Sec/Ia32: seed the temporary RAM with PcdInitValueInTempStack
OvmfPkg/Sec/X64: seed the temporary RAM with PcdInitValueInTempStack
OvmfPkg: restore temporary SEC/PEI RAM size to 64KB
OvmfPkg/OvmfPkgIa32.fdf | 2 +-
OvmfPkg/OvmfPkgIa32X64.fdf | 2 +-
OvmfPkg/OvmfPkgX64.fdf | 2 +-
OvmfPkg/Sec/SecMain.inf | 1 +
OvmfPkg/Sec/Ia32/SecEntry.nasm | 19 ++++++++++++++++---
OvmfPkg/Sec/X64/SecEntry.nasm | 15 +++++++++++++++
6 files changed, 35 insertions(+), 6 deletions(-)
--
2.14.1.3.gb7cf6e02401b
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
On 10 November 2017 at 15:49, Laszlo Ersek <lersek@redhat.com> wrote:
> The first three patches enable the PEI_CORE to report OVMF's temp
> SEC/PEI stack and heap usage.
>
> - This depends on the new fixed PCD "PcdInitValueInTempStack",
> recently added for
> <https://bugzilla.tianocore.org/show_bug.cgi?id=740>
> ("INIT_CAR_VALUE should be defined in a central location").
>
> - Ard recently implemented the same in ArmPlatformPkg, for
> <https://bugzilla.tianocore.org/show_bug.cgi?id=748> ("measure temp
> SEC/PEI stack usage").
>
> The last (fourth) patch adapts OVMF to the larger MtrrLib stack demand
> that originates from commit 2bbd7e2fbd4b ("UefiCpuPkg/MtrrLib: Update
> algorithm to calculate optimal settings", 2017-09-27). OVMF's temp RAM
> size is restored to the historical / original 64KB.
>
> This work is tracked in
> <https://bugzilla.tianocore.org/show_bug.cgi?id=747> ("measure temp
> SEC/PEI stack usage; grow temp SEC/PEI RAM"), with links to related
> mailing list discussions.
>
> Repo: https://github.com/lersek/edk2.git
> Branch: temp_ram_tweaks
>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
>
> Thanks
> Laszlo
>
> Laszlo Ersek (4):
> OvmfPkg/Sec/Ia32: free up EAX for other uses while setting up the
> stack
> OvmfPkg/Sec/Ia32: seed the temporary RAM with PcdInitValueInTempStack
> OvmfPkg/Sec/X64: seed the temporary RAM with PcdInitValueInTempStack
> OvmfPkg: restore temporary SEC/PEI RAM size to 64KB
>
Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> OvmfPkg/OvmfPkgIa32.fdf | 2 +-
> OvmfPkg/OvmfPkgIa32X64.fdf | 2 +-
> OvmfPkg/OvmfPkgX64.fdf | 2 +-
> OvmfPkg/Sec/SecMain.inf | 1 +
> OvmfPkg/Sec/Ia32/SecEntry.nasm | 19 ++++++++++++++++---
> OvmfPkg/Sec/X64/SecEntry.nasm | 15 +++++++++++++++
> 6 files changed, 35 insertions(+), 6 deletions(-)
>
> --
> 2.14.1.3.gb7cf6e02401b
>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
On 2017-11-10 07:49:04, Laszlo Ersek wrote:
> The first three patches enable the PEI_CORE to report OVMF's temp
> SEC/PEI stack and heap usage.
>
> - This depends on the new fixed PCD "PcdInitValueInTempStack",
> recently added for
> <https://bugzilla.tianocore.org/show_bug.cgi?id=740>
> ("INIT_CAR_VALUE should be defined in a central location").
>
> - Ard recently implemented the same in ArmPlatformPkg, for
> <https://bugzilla.tianocore.org/show_bug.cgi?id=748> ("measure temp
> SEC/PEI stack usage").
>
> The last (fourth) patch adapts OVMF to the larger MtrrLib stack demand
> that originates from commit 2bbd7e2fbd4b ("UefiCpuPkg/MtrrLib: Update
> algorithm to calculate optimal settings", 2017-09-27). OVMF's temp RAM
> size is restored to the historical / original 64KB.
>
> This work is tracked in
> <https://bugzilla.tianocore.org/show_bug.cgi?id=747> ("measure temp
> SEC/PEI stack usage; grow temp SEC/PEI RAM"), with links to related
> mailing list discussions.
>
> Repo: https://github.com/lersek/edk2.git
> Branch: temp_ram_tweaks
>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
>
> Thanks
> Laszlo
>
> Laszlo Ersek (4):
> OvmfPkg/Sec/Ia32: free up EAX for other uses while setting up the
> stack
> OvmfPkg/Sec/Ia32: seed the temporary RAM with PcdInitValueInTempStack
> OvmfPkg/Sec/X64: seed the temporary RAM with PcdInitValueInTempStack
I'd like to try a different option for these 3. Can you hold off a bit
before pushing this series?
-Jordan
> OvmfPkg: restore temporary SEC/PEI RAM size to 64KB
>
> OvmfPkg/OvmfPkgIa32.fdf | 2 +-
> OvmfPkg/OvmfPkgIa32X64.fdf | 2 +-
> OvmfPkg/OvmfPkgX64.fdf | 2 +-
> OvmfPkg/Sec/SecMain.inf | 1 +
> OvmfPkg/Sec/Ia32/SecEntry.nasm | 19 ++++++++++++++++---
> OvmfPkg/Sec/X64/SecEntry.nasm | 15 +++++++++++++++
> 6 files changed, 35 insertions(+), 6 deletions(-)
>
> --
> 2.14.1.3.gb7cf6e02401b
>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
On 2017-11-11 12:38:21, Jordan Justen wrote:
> On 2017-11-10 07:49:04, Laszlo Ersek wrote:
> > The first three patches enable the PEI_CORE to report OVMF's temp
> > SEC/PEI stack and heap usage.
> >
> > - This depends on the new fixed PCD "PcdInitValueInTempStack",
> > recently added for
> > <https://bugzilla.tianocore.org/show_bug.cgi?id=740>
> > ("INIT_CAR_VALUE should be defined in a central location").
> >
> > - Ard recently implemented the same in ArmPlatformPkg, for
> > <https://bugzilla.tianocore.org/show_bug.cgi?id=748> ("measure temp
> > SEC/PEI stack usage").
> >
> > The last (fourth) patch adapts OVMF to the larger MtrrLib stack demand
> > that originates from commit 2bbd7e2fbd4b ("UefiCpuPkg/MtrrLib: Update
> > algorithm to calculate optimal settings", 2017-09-27). OVMF's temp RAM
> > size is restored to the historical / original 64KB.
> >
> > This work is tracked in
> > <https://bugzilla.tianocore.org/show_bug.cgi?id=747> ("measure temp
> > SEC/PEI stack usage; grow temp SEC/PEI RAM"), with links to related
> > mailing list discussions.
> >
> > Repo: https://github.com/lersek/edk2.git
> > Branch: temp_ram_tweaks
> >
> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > Cc: Jordan Justen <jordan.l.justen@intel.com>
> > Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> >
> > Thanks
> > Laszlo
> >
> > Laszlo Ersek (4):
> > OvmfPkg/Sec/Ia32: free up EAX for other uses while setting up the
> > stack
> > OvmfPkg/Sec/Ia32: seed the temporary RAM with PcdInitValueInTempStack
> > OvmfPkg/Sec/X64: seed the temporary RAM with PcdInitValueInTempStack
>
> I'd like to try a different option for these 3. Can you hold off a bit
> before pushing this series?
I think we should use a C based approach instead, like in the attached
patch.
> > OvmfPkg: restore temporary SEC/PEI RAM size to 64KB
This patch is Reviewed-by: Jordan Justen <jordan.l.justen@intel.com>
> >
> > OvmfPkg/OvmfPkgIa32.fdf | 2 +-
> > OvmfPkg/OvmfPkgIa32X64.fdf | 2 +-
> > OvmfPkg/OvmfPkgX64.fdf | 2 +-
> > OvmfPkg/Sec/SecMain.inf | 1 +
> > OvmfPkg/Sec/Ia32/SecEntry.nasm | 19 ++++++++++++++++---
> > OvmfPkg/Sec/X64/SecEntry.nasm | 15 +++++++++++++++
> > 6 files changed, 35 insertions(+), 6 deletions(-)
> >
> > --
> > 2.14.1.3.gb7cf6e02401b
> >
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
On 11 November 2017 at 22:04, Jordan Justen <jordan.l.justen@intel.com> wrote:
> On 2017-11-11 12:38:21, Jordan Justen wrote:
>> On 2017-11-10 07:49:04, Laszlo Ersek wrote:
>> > The first three patches enable the PEI_CORE to report OVMF's temp
>> > SEC/PEI stack and heap usage.
>> >
>> > - This depends on the new fixed PCD "PcdInitValueInTempStack",
>> > recently added for
>> > <https://bugzilla.tianocore.org/show_bug.cgi?id=740>
>> > ("INIT_CAR_VALUE should be defined in a central location").
>> >
>> > - Ard recently implemented the same in ArmPlatformPkg, for
>> > <https://bugzilla.tianocore.org/show_bug.cgi?id=748> ("measure temp
>> > SEC/PEI stack usage").
>> >
>> > The last (fourth) patch adapts OVMF to the larger MtrrLib stack demand
>> > that originates from commit 2bbd7e2fbd4b ("UefiCpuPkg/MtrrLib: Update
>> > algorithm to calculate optimal settings", 2017-09-27). OVMF's temp RAM
>> > size is restored to the historical / original 64KB.
>> >
>> > This work is tracked in
>> > <https://bugzilla.tianocore.org/show_bug.cgi?id=747> ("measure temp
>> > SEC/PEI stack usage; grow temp SEC/PEI RAM"), with links to related
>> > mailing list discussions.
>> >
>> > Repo: https://github.com/lersek/edk2.git
>> > Branch: temp_ram_tweaks
>> >
>> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> > Cc: Jordan Justen <jordan.l.justen@intel.com>
>> > Cc: Ruiyu Ni <ruiyu.ni@intel.com>
>> >
>> > Thanks
>> > Laszlo
>> >
>> > Laszlo Ersek (4):
>> > OvmfPkg/Sec/Ia32: free up EAX for other uses while setting up the
>> > stack
>> > OvmfPkg/Sec/Ia32: seed the temporary RAM with PcdInitValueInTempStack
>> > OvmfPkg/Sec/X64: seed the temporary RAM with PcdInitValueInTempStack
>>
>> I'd like to try a different option for these 3. Can you hold off a bit
>> before pushing this series?
>
> I think we should use a C based approach instead, like in the attached
> patch.
>
I'm not sure: having to abuse SetJump () and having to leave an
arbitrary 512 byte window both seem pretty good reasons to stick with
assembly.
Is your concern that the stack gets cleared in RELEASE builds as well?
Can't we put an #ifndef MDEPKG_NDEBUG around the code instead?
>> > OvmfPkg: restore temporary SEC/PEI RAM size to 64KB
>
> This patch is Reviewed-by: Jordan Justen <jordan.l.justen@intel.com>
>
>> >
>> > OvmfPkg/OvmfPkgIa32.fdf | 2 +-
>> > OvmfPkg/OvmfPkgIa32X64.fdf | 2 +-
>> > OvmfPkg/OvmfPkgX64.fdf | 2 +-
>> > OvmfPkg/Sec/SecMain.inf | 1 +
>> > OvmfPkg/Sec/Ia32/SecEntry.nasm | 19 ++++++++++++++++---
>> > OvmfPkg/Sec/X64/SecEntry.nasm | 15 +++++++++++++++
>> > 6 files changed, 35 insertions(+), 6 deletions(-)
>> >
>> > --
>> > 2.14.1.3.gb7cf6e02401b
>> >
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
On 2017-11-12 02:58:37, Ard Biesheuvel wrote:
> On 11 November 2017 at 22:04, Jordan Justen <jordan.l.justen@intel.com> wrote:
> > On 2017-11-11 12:38:21, Jordan Justen wrote:
> >> On 2017-11-10 07:49:04, Laszlo Ersek wrote:
> >> > The first three patches enable the PEI_CORE to report OVMF's temp
> >> > SEC/PEI stack and heap usage.
> >> >
> >> > - This depends on the new fixed PCD "PcdInitValueInTempStack",
> >> > recently added for
> >> > <https://bugzilla.tianocore.org/show_bug.cgi?id=740>
> >> > ("INIT_CAR_VALUE should be defined in a central location").
> >> >
> >> > - Ard recently implemented the same in ArmPlatformPkg, for
> >> > <https://bugzilla.tianocore.org/show_bug.cgi?id=748> ("measure temp
> >> > SEC/PEI stack usage").
> >> >
> >> > The last (fourth) patch adapts OVMF to the larger MtrrLib stack demand
> >> > that originates from commit 2bbd7e2fbd4b ("UefiCpuPkg/MtrrLib: Update
> >> > algorithm to calculate optimal settings", 2017-09-27). OVMF's temp RAM
> >> > size is restored to the historical / original 64KB.
> >> >
> >> > This work is tracked in
> >> > <https://bugzilla.tianocore.org/show_bug.cgi?id=747> ("measure temp
> >> > SEC/PEI stack usage; grow temp SEC/PEI RAM"), with links to related
> >> > mailing list discussions.
> >> >
> >> > Repo: https://github.com/lersek/edk2.git
> >> > Branch: temp_ram_tweaks
> >> >
> >> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >> > Cc: Jordan Justen <jordan.l.justen@intel.com>
> >> > Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> >> >
> >> > Thanks
> >> > Laszlo
> >> >
> >> > Laszlo Ersek (4):
> >> > OvmfPkg/Sec/Ia32: free up EAX for other uses while setting up the
> >> > stack
> >> > OvmfPkg/Sec/Ia32: seed the temporary RAM with PcdInitValueInTempStack
> >> > OvmfPkg/Sec/X64: seed the temporary RAM with PcdInitValueInTempStack
> >>
> >> I'd like to try a different option for these 3. Can you hold off a bit
> >> before pushing this series?
> >
> > I think we should use a C based approach instead, like in the attached
> > patch.
> >
>
> I'm not sure: having to abuse SetJump ()
True, that was annoying. It seems like we could have AsmReadEsp and
AsmReadRsp in BaseLib since we have AsmReadSp for IPF.
> and having to leave an
> arbitrary 512 byte window both seem pretty good reasons to stick with
> assembly.
Also true. I chose 512 because it seemed like more than SetMem32 could
reasonably need, but also much below the minimum I would expect PEI to
use. (It seemed that around 4k ended up being used.)
> Is your concern that the stack gets cleared in RELEASE builds as well?
No. I just prefer if we can use C rather than assembly whenever it is
reasonable.
-Jordan
> Can't we put an #ifndef MDEPKG_NDEBUG around the code instead?
>
> >> > OvmfPkg: restore temporary SEC/PEI RAM size to 64KB
> >
> > This patch is Reviewed-by: Jordan Justen <jordan.l.justen@intel.com>
> >
> >> >
> >> > OvmfPkg/OvmfPkgIa32.fdf | 2 +-
> >> > OvmfPkg/OvmfPkgIa32X64.fdf | 2 +-
> >> > OvmfPkg/OvmfPkgX64.fdf | 2 +-
> >> > OvmfPkg/Sec/SecMain.inf | 1 +
> >> > OvmfPkg/Sec/Ia32/SecEntry.nasm | 19 ++++++++++++++++---
> >> > OvmfPkg/Sec/X64/SecEntry.nasm | 15 +++++++++++++++
> >> > 6 files changed, 35 insertions(+), 6 deletions(-)
> >> >
> >> > --
> >> > 2.14.1.3.gb7cf6e02401b
> >> >
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
On 13 November 2017 at 09:08, Jordan Justen <jordan.l.justen@intel.com> wrote:
> On 2017-11-12 02:58:37, Ard Biesheuvel wrote:
>> On 11 November 2017 at 22:04, Jordan Justen <jordan.l.justen@intel.com> wrote:
>> > On 2017-11-11 12:38:21, Jordan Justen wrote:
>> >> On 2017-11-10 07:49:04, Laszlo Ersek wrote:
>> >> > The first three patches enable the PEI_CORE to report OVMF's temp
>> >> > SEC/PEI stack and heap usage.
>> >> >
>> >> > - This depends on the new fixed PCD "PcdInitValueInTempStack",
>> >> > recently added for
>> >> > <https://bugzilla.tianocore.org/show_bug.cgi?id=740>
>> >> > ("INIT_CAR_VALUE should be defined in a central location").
>> >> >
>> >> > - Ard recently implemented the same in ArmPlatformPkg, for
>> >> > <https://bugzilla.tianocore.org/show_bug.cgi?id=748> ("measure temp
>> >> > SEC/PEI stack usage").
>> >> >
>> >> > The last (fourth) patch adapts OVMF to the larger MtrrLib stack demand
>> >> > that originates from commit 2bbd7e2fbd4b ("UefiCpuPkg/MtrrLib: Update
>> >> > algorithm to calculate optimal settings", 2017-09-27). OVMF's temp RAM
>> >> > size is restored to the historical / original 64KB.
>> >> >
>> >> > This work is tracked in
>> >> > <https://bugzilla.tianocore.org/show_bug.cgi?id=747> ("measure temp
>> >> > SEC/PEI stack usage; grow temp SEC/PEI RAM"), with links to related
>> >> > mailing list discussions.
>> >> >
>> >> > Repo: https://github.com/lersek/edk2.git
>> >> > Branch: temp_ram_tweaks
>> >> >
>> >> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> >> > Cc: Jordan Justen <jordan.l.justen@intel.com>
>> >> > Cc: Ruiyu Ni <ruiyu.ni@intel.com>
>> >> >
>> >> > Thanks
>> >> > Laszlo
>> >> >
>> >> > Laszlo Ersek (4):
>> >> > OvmfPkg/Sec/Ia32: free up EAX for other uses while setting up the
>> >> > stack
>> >> > OvmfPkg/Sec/Ia32: seed the temporary RAM with PcdInitValueInTempStack
>> >> > OvmfPkg/Sec/X64: seed the temporary RAM with PcdInitValueInTempStack
>> >>
>> >> I'd like to try a different option for these 3. Can you hold off a bit
>> >> before pushing this series?
>> >
>> > I think we should use a C based approach instead, like in the attached
>> > patch.
>> >
>>
>> I'm not sure: having to abuse SetJump ()
>
> True, that was annoying. It seems like we could have AsmReadEsp and
> AsmReadRsp in BaseLib since we have AsmReadSp for IPF.
>
>> and having to leave an
>> arbitrary 512 byte window both seem pretty good reasons to stick with
>> assembly.
>
> Also true. I chose 512 because it seemed like more than SetMem32 could
> reasonably need, but also much below the minimum I would expect PEI to
> use. (It seemed that around 4k ended up being used.)
>
>> Is your concern that the stack gets cleared in RELEASE builds as well?
>
> No. I just prefer if we can use C rather than assembly whenever it is
> reasonable.
>
No discussion there. But in my opinion, anything involving the
absolute value of the stack pointer does not belong in C.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
On 11/13/17 11:09, Ard Biesheuvel wrote:
> On 13 November 2017 at 09:08, Jordan Justen
> <jordan.l.justen@intel.com> wrote:
>> On 2017-11-12 02:58:37, Ard Biesheuvel wrote:
>>> On 11 November 2017 at 22:04, Jordan Justen
>>> <jordan.l.justen@intel.com> wrote:
>>>> On 2017-11-11 12:38:21, Jordan Justen wrote:
>>>>> On 2017-11-10 07:49:04, Laszlo Ersek wrote:
>>>>>> The first three patches enable the PEI_CORE to report OVMF's temp
>>>>>> SEC/PEI stack and heap usage.
>>>>>>
>>>>>> - This depends on the new fixed PCD "PcdInitValueInTempStack",
>>>>>> recently added for
>>>>>> <https://bugzilla.tianocore.org/show_bug.cgi?id=740>
>>>>>> ("INIT_CAR_VALUE should be defined in a central location").
>>>>>>
>>>>>> - Ard recently implemented the same in ArmPlatformPkg, for
>>>>>> <https://bugzilla.tianocore.org/show_bug.cgi?id=748>
>>>>>> ("measure temp SEC/PEI stack usage").
>>>>>>
>>>>>> The last (fourth) patch adapts OVMF to the larger MtrrLib stack
>>>>>> demand that originates from commit 2bbd7e2fbd4b
>>>>>> ("UefiCpuPkg/MtrrLib: Update algorithm to calculate optimal
>>>>>> settings", 2017-09-27). OVMF's temp RAM size is restored to the
>>>>>> historical / original 64KB.
>>>>>>
>>>>>> This work is tracked in
>>>>>> <https://bugzilla.tianocore.org/show_bug.cgi?id=747> ("measure
>>>>>> temp SEC/PEI stack usage; grow temp SEC/PEI RAM"), with links to
>>>>>> related mailing list discussions.
>>>>>>
>>>>>> Repo: https://github.com/lersek/edk2.git
>>>>>> Branch: temp_ram_tweaks
>>>>>>
>>>>>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>>>>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>>>>>> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
>>>>>>
>>>>>> Thanks
>>>>>> Laszlo
>>>>>>
>>>>>> Laszlo Ersek (4):
>>>>>> OvmfPkg/Sec/Ia32: free up EAX for other uses while setting up
>>>>>> the stack
>>>>>> OvmfPkg/Sec/Ia32: seed the temporary RAM with
>>>>>> PcdInitValueInTempStack
>>>>>> OvmfPkg/Sec/X64: seed the temporary RAM with
>>>>>> PcdInitValueInTempStack
>>>>>
>>>>> I'd like to try a different option for these 3. Can you hold off a
>>>>> bit before pushing this series?
>>>>
>>>> I think we should use a C based approach instead, like in the
>>>> attached patch.
>>>>
>>>
>>> I'm not sure: having to abuse SetJump ()
>>
>> True, that was annoying. It seems like we could have AsmReadEsp and
>> AsmReadRsp in BaseLib since we have AsmReadSp for IPF.
>>
>>> and having to leave an arbitrary 512 byte window both seem pretty
>>> good reasons to stick with assembly.
>>
>> Also true. I chose 512 because it seemed like more than SetMem32
>> could reasonably need, but also much below the minimum I would expect
>> PEI to use. (It seemed that around 4k ended up being used.)
>>
>>> Is your concern that the stack gets cleared in RELEASE builds as
>>> well?
>>
>> No. I just prefer if we can use C rather than assembly whenever it is
>> reasonable.
>>
>
> No discussion there. But in my opinion, anything involving the
> absolute value of the stack pointer does not belong in C.
>
I chose assembly because it seemed much cleaner and simpler to seed the
stack before C code actually started using the stack.
GCC sometimes plays dirty tricks with laying out local variables on the
stack, so addresses of local variables cannot / should not be used for
this purpose. (For example, see commit f98f5ec304ec, "UefiCpuPkg:
S3Resume2Pei: align return stacks explicitly", 2013-12-13.)
BASE_LIBRARY_JUMP_BUFFER didn't occur to me (I've always considered jump
buffers opaque to client code).
AsmReadSp() is IPF only (the BaseLib class header says so, and the tree
agrees).
AsmReadEsp() and AsmReadRsp() do not exist in BaseLib, and they would
only be implementable for x86. I think platform-dependent library
*interfaces* don't belong into BaseLib (in other words, AsmReadSp() is
already a mistake in my opinion; we had better not make it worse).
I guess I could live with BASE_LIBRARY_JUMP_BUFFER.
More specific comments:
> diff --git a/OvmfPkg/Sec/SecMain.c b/OvmfPkg/Sec/SecMain.c
> index f7fec3d8c0..077f7d6563 100644
> --- a/OvmfPkg/Sec/SecMain.c
> +++ b/OvmfPkg/Sec/SecMain.c
> @@ -1,7 +1,7 @@
> /** @file
> Main SEC phase code. Transitions to PEI.
>
> - Copyright (c) 2008 - 2015, Intel Corporation. All rights reserved.<BR>
> + Copyright (c) 2008 - 2017, Intel Corporation. All rights reserved.<BR>
> (C) Copyright 2016 Hewlett Packard Enterprise Development LP<BR>
>
> This program and the accompanying materials
> @@ -731,6 +731,25 @@ SecCoreStartupWithStack (
> UINT32 Index;
> volatile UINT8 *Table;
>
> + //
> + // Fill most of temporary RAM with PcdInitValueInTempStack. We stop
> + // filling at the current stack pointer - 512 bytes.
> + //
> + DEBUG_CODE_BEGIN ();
> + BASE_LIBRARY_JUMP_BUFFER JumpBuffer;
> + UINTN StackUsed;
> +
> + SetJump (&JumpBuffer);
> +#if defined (MDE_CPU_IA32)
> + StackUsed = (UINTN)TopOfCurrentStack - JumpBuffer.Esp;
> +#elif defined (MDE_CPU_X64)
> + StackUsed = (UINTN)TopOfCurrentStack - JumpBuffer.Rsp;
> +#endif
> + SetMem32 ((VOID*)(UINTN)PcdGet32 (PcdOvmfSecPeiTempRamBase),
> + PcdGet32 (PcdOvmfSecPeiTempRamSize) - StackUsed - 512,
> + FixedPcdGet32 (PcdInitValueInTempStack));
(1) SetMem32() is likely problematic in itself; please refer to the
following comment -- partly visible in the context of Jordan's patch --,
from commit 320b4f084a25 ("OvmfPkg: Sec: force reinit of
BaseExtractGuidedSectionLib handler table", 2015-11-30):
//
// To ensure SMM can't be compromised on S3 resume, we must force re-init of
// the BaseExtractGuidedSectionLib. Since this is before library contructors
// are called, we must use a loop rather than SetMem.
//
Thus, we should use a loop and a pointer-to-volatile. (It would likely
be slower than the REP STOSD / REP STOSQ.)
(2) The indentation of arguments is off.
(It doesn't matter if we replace SetMem32() anyway, due to (1).)
(3) If we replace SetMem32() with a loop, and (consequently) there's no
risk for SetMem32() to bust its own stack / return address, then how
should the constant 512 change? Does it become zero?
What does GCC guarantee about the value of ESP / RSP at that point,
versus the addresses of SecCoreStartupWithStack()'s own local variables?
> + DEBUG_CODE_END ();
> +
> //
> // To ensure SMM can't be compromised on S3 resume, we must force re-init of
> // the BaseExtractGuidedSectionLib. Since this is before library contructors
Thanks
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
On 11/13/17 13:34, Laszlo Ersek wrote:
> I guess I could live with BASE_LIBRARY_JUMP_BUFFER.
Actually:
> More specific comments:
>
>> diff --git a/OvmfPkg/Sec/SecMain.c b/OvmfPkg/Sec/SecMain.c
>> index f7fec3d8c0..077f7d6563 100644
>> --- a/OvmfPkg/Sec/SecMain.c
>> +++ b/OvmfPkg/Sec/SecMain.c
>> @@ -1,7 +1,7 @@
>> /** @file
>> Main SEC phase code. Transitions to PEI.
>>
>> - Copyright (c) 2008 - 2015, Intel Corporation. All rights reserved.<BR>
>> + Copyright (c) 2008 - 2017, Intel Corporation. All rights reserved.<BR>
>> (C) Copyright 2016 Hewlett Packard Enterprise Development LP<BR>
>>
>> This program and the accompanying materials
>> @@ -731,6 +731,25 @@ SecCoreStartupWithStack (
>> UINT32 Index;
>> volatile UINT8 *Table;
>>
>> + //
>> + // Fill most of temporary RAM with PcdInitValueInTempStack. We stop
>> + // filling at the current stack pointer - 512 bytes.
>> + //
>> + DEBUG_CODE_BEGIN ();
>> + BASE_LIBRARY_JUMP_BUFFER JumpBuffer;
>> + UINTN StackUsed;
>> +
>> + SetJump (&JumpBuffer);
>> +#if defined (MDE_CPU_IA32)
>> + StackUsed = (UINTN)TopOfCurrentStack - JumpBuffer.Esp;
>> +#elif defined (MDE_CPU_X64)
>> + StackUsed = (UINTN)TopOfCurrentStack - JumpBuffer.Rsp;
>> +#endif
>> + SetMem32 ((VOID*)(UINTN)PcdGet32 (PcdOvmfSecPeiTempRamBase),
>> + PcdGet32 (PcdOvmfSecPeiTempRamSize) - StackUsed - 512,
>> + FixedPcdGet32 (PcdInitValueInTempStack));
>
> (1) SetMem32() is likely problematic in itself; please refer to the
> following comment -- partly visible in the context of Jordan's patch --,
> from commit 320b4f084a25 ("OvmfPkg: Sec: force reinit of
> BaseExtractGuidedSectionLib handler table", 2015-11-30):
>
> //
> // To ensure SMM can't be compromised on S3 resume, we must force re-init of
> // the BaseExtractGuidedSectionLib. Since this is before library contructors
> // are called, we must use a loop rather than SetMem.
> //
>
> Thus, we should use a loop and a pointer-to-volatile. (It would likely
> be slower than the REP STOSD / REP STOSQ.)
given that I'm opposed to calling any library functions before we reach
the ProcessLibraryConstructorList() call lower down in
SecCoreStartupWithStack(), I cannot agree to calling SetJump() either.
Thanks,
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
On 2017-11-13 05:09:03, Laszlo Ersek wrote: > given that I'm opposed to calling any library functions before we reach > the ProcessLibraryConstructorList() call lower down in > SecCoreStartupWithStack(), I cannot agree to calling SetJump() either. Good point. -Jordan _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 2017-11-13 04:34:05, Laszlo Ersek wrote: > AsmReadSp() is IPF only (the BaseLib class header says so, and the tree > agrees). > > AsmReadEsp() and AsmReadRsp() do not exist in BaseLib, and they would > only be implementable for x86. I think platform-dependent library > *interfaces* don't belong into BaseLib (in other words, AsmReadSp() is > already a mistake in my opinion; we had better not make it worse). There are plenty of arch specific register accessor functions in BaseLib. -Jordan _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
© 2016 - 2024 Red Hat, Inc.