Discussion:
[edk2] [PATCH] ArmPlatformPkg: Fix boot on FVP with ArmPlatformPkg SEC at EL3
Cohen, Eugene
2015-05-16 15:02:30 UTC
Permalink
Olivier (or if you prefer "Dear ArmPkg and ArmPlatformPkg maintainer"),

Here are three patches (in addition to the one I sent earlier) to fix issues I encountered booting the FVP Base platform using SEC from ArmPlatformPkg instead of Trusted Firmware. This boot path is still one where SEC runs at EL3 and the remaining boot stages run at EL2/EL1.

As a preface to these patch sets I should add that I'm unclear about the structure of the ARM platforms. From a directory naming perspective I see an ArmVExpressPkg which supports an FVP-AArch64 platform but I don't know whether this platform is intended to support both the FVP_Base platforms as well as the FVP_VE (Versatile Express) platforms. There are significant differences between these platforms, for example the GIC revision, the memory maps and peripherals are all different. As such it's not clear whether I'm making changes in the right place but that being said I am able to boot to the EFI Shell after these few fixes. Olivier, please advise what the proper way is to handle Versatile Express and Base platform differences.

The fixes break down as follows:

1. The intent on the FVP is to use main memory at 0x880000000 but a PCD value causes PEI and the DXE core to be loaded at a a sub-4GB address. This patch brings all of the DRAM usage into the same area.

2. The architectural timer was not advancing because the FVP Reference Counter, which feeds the processor's timer, must be started by EL3 firmware on the FVP Base platforms

3. Interrupts were not being delivered to the core because EL3 firmware must clear the ProcessorSleep bit in the GICv3 redistributor.

I've been trying to use the github development model for this so I've created a pull request with these changes for your convenience: https://github.com/tianocore/edk2/pull/10 . This tool has a significantly better code review capability than what is possible on a mailing list.

(My next efforts will be around extending EL3 into DXE to support SMM-style initialization but that's a future subject.)


Contributed-under: TianoCore Contribution Agreement 1.0

Signed-off-by: Eugene Cohen <***@hp.com<mailto:***@hp.com>>
Olivier Martin
2015-05-29 15:54:42 UTC
Permalink
For the community information, I commented Eugene's patch into Github (https://github.com/tianocore/edk2/pull/10) 3 days ago.
As I told Eugene there is no chance his pull-request would be accepted as EDK2 project is not supported through github. But once I would be happy with his patch I would be happy to commit them into SVN.
To come back on the current discussion about code review. Github is not the prefect tool to review patches because it is difficult to compare changes between pull-request (or at least it was not last time I tried). But email code review is not better on this point neither. And Gerrit do it pretty well.


From: Cohen, Eugene [mailto:***@hp.com]
Sent: 16 May 2015 16:03
To: edk2-***@lists.sourceforge.net
Subject: [edk2] [PATCH] ArmPlatformPkg: Fix boot on FVP with ArmPlatformPkg SEC at EL3

Olivier (or if you prefer "Dear ArmPkg and ArmPlatformPkg maintainer"),

Here are three patches (in addition to the one I sent earlier) to fix issues I encountered booting the FVP Base platform using SEC from ArmPlatformPkg instead of Trusted Firmware. This boot path is still one where SEC runs at EL3 and the remaining boot stages run at EL2/EL1.

As a preface to these patch sets I should add that I'm unclear about the structure of the ARM platforms. From a directory naming perspective I see an ArmVExpressPkg which supports an FVP-AArch64 platform but I don't know whether this platform is intended to support both the FVP_Base platforms as well as the FVP_VE (Versatile Express) platforms. There are significant differences between these platforms, for example the GIC revision, the memory maps and peripherals are all different. As such it's not clear whether I'm making changes in the right place but that being said I am able to boot to the EFI Shell after these few fixes. Olivier, please advise what the proper way is to handle Versatile Express and Base platform differences.

The fixes break down as follows:

1. The intent on the FVP is to use main memory at 0x880000000 but a PCD value causes PEI and the DXE core to be loaded at a a sub-4GB address. This patch brings all of the DRAM usage into the same area.

2. The architectural timer was not advancing because the FVP Reference Counter, which feeds the processor's timer, must be started by EL3 firmware on the FVP Base platforms

3. Interrupts were not being delivered to the core because EL3 firmware must clear the ProcessorSleep bit in the GICv3 redistributor.

I've been trying to use the github development model for this so I've created a pull request with these changes for your convenience: https://github.com/tianocore/edk2/pull/10 . This tool has a significantly better code review capability than what is possible on a mailing list.

(My next efforts will be around extending EL3 into DXE to support SMM-style initialization but that's a future subject.)


Contributed-under: TianoCore Contribution Agreement 1.0

Signed-off-by: Eugene Cohen <***@hp.com<mailto:***@hp.com>>


-- IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

ARM Limited, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No: 2557590
ARM Holdings plc, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No: 2548782
Laszlo Ersek
2015-05-29 16:21:37 UTC
Permalink
For the community information, I commented Eugene’s patch into Github
(https://github.com/tianocore/edk2/pull/10) 3 days ago.
As I told Eugene there is no chance his pull-request would be accepted
as EDK2 project is not supported through github. But once I would be
happy with his patch I would be happy to commit them into SVN.
To come back on the current discussion about code review. Github is not
the prefect tool to review patches because it is difficult to compare
changes between pull-request (or at least it was not last time I tried).
But email code review is not better on this point neither. And Gerrit do
it pretty well.
I compare v(n) patchsets against v(n+1) all the time. I apply both to
separate local branches that are forked from the same master branch
commit. Commits (trees) that constitute the same "stage" of the series
in each branch can be easily diffed against each other.

I also tend to format the individual patch files for both v(n) and
v(n+1), and compare the corresponding patch files across the versions
with plain "diff". (Well, "colordiff" to be precise, and a number of
shell utilities.)

It gives an interdiff-like result (two columns of +/- at the left). Its
interpretation is different from that of "git-diff for merges" (*), but
it's not hard to understand. Some of Ard's early series went through
many iterations, and this was a very efficient way to focus on the changes.

((*) "git-diff for merges" (= combined diff) assigns each column to a
separate parent. Whereas diffing patch files gives a diff of diffs.)

Thanks
Laszlo
*Sent:* 16 May 2015 16:03
*Subject:* [edk2] [PATCH] ArmPlatformPkg: Fix boot on FVP with
ArmPlatformPkg SEC at EL3
Olivier (or if you prefer “Dear ArmPkg and ArmPlatformPkg maintainer”),
Here are three patches (in addition to the one I sent earlier) to fix
issues I encountered booting the FVP Base platform using SEC from
ArmPlatformPkg instead of Trusted Firmware. This boot path is still one
where SEC runs at EL3 and the remaining boot stages run at EL2/EL1.
As a preface to these patch sets I should add that I’m unclear about the
structure of the ARM platforms. From a directory naming perspective I
see an ArmVExpressPkg which supports an FVP-AArch64 platform but I don’t
know whether this platform is intended to support both the FVP_Base
platforms as well as the FVP_VE (Versatile Express) platforms. There
are significant differences between these platforms, for example the GIC
revision, the memory maps and peripherals are all different. As such
it’s not clear whether I’m making changes in the right place but that
being said I am able to boot to the EFI Shell after these few fixes.
Olivier, please advise what the proper way is to handle Versatile
Express and Base platform differences.
1. The intent on the FVP is to use main memory at 0x880000000 but
a PCD value causes PEI and the DXE core to be loaded at a a sub-4GB
address. This patch brings all of the DRAM usage into the same area.
2. The architectural timer was not advancing because the FVP
Reference Counter, which feeds the processor’s timer, must be started by
EL3 firmware on the FVP Base platforms
3. Interrupts were not being delivered to the core because EL3
firmware must clear the ProcessorSleep bit in the GICv3 redistributor.
I’ve been trying to use the github development model for this so I’ve
https://github.com/tianocore/edk2/pull/10 . This tool has a
significantly better code review capability than what is possible on a
mailing list.
(My next efforts will be around extending EL3 into DXE to support
SMM-style initialization but that’s a future subject.)
Contributed-under: TianoCore Contribution Agreement 1.0
-- IMPORTANT NOTICE: The contents of this email and any attachments are
confidential and may also be privileged. If you are not the intended
recipient, please notify the sender immediately and do not disclose the
contents to any other person, use it for any purpose, or store or copy
the information in any medium. Thank you.
ARM Limited, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ,
Registered in England & Wales, Company No: 2557590
ARM Holdings plc, Registered office 110 Fulbourn Road, Cambridge CB1
9NJ, Registered in England & Wales, Company No: 2548782
------------------------------------------------------------------------------
_______________________________________________
edk2-devel mailing list
https://lists.sourceforge.net/lists/listinfo/edk2-devel
------------------------------------------------------------------------------
Cohen, Eugene
2015-05-29 16:27:31 UTC
Permalink
Thanks Olivier - I quite liked the github review, my favorite part being the "line note" comments that are correlated to the source code. We also use gerrit internally and it has similar capabilities. I should note that both tools do generate email for the comments so it does not exclude people who prefer email-based flows. And with both tools you can always pull down the branch in git and do local review/build/testing.

As for the pull request itself, I'm reviewing your feedback and will make the changes, thanks.

Eugene

From: Olivier Martin [mailto:***@arm.com]
Sent: Friday, May 29, 2015 9:55 AM
To: edk2-***@lists.sourceforge.net
Subject: Re: [edk2] [PATCH] ArmPlatformPkg: Fix boot on FVP with ArmPlatformPkg SEC at EL3

For the community information, I commented Eugene's patch into Github (https://github.com/tianocore/edk2/pull/10) 3 days ago.
As I told Eugene there is no chance his pull-request would be accepted as EDK2 project is not supported through github. But once I would be happy with his patch I would be happy to commit them into SVN.
To come back on the current discussion about code review. Github is not the prefect tool to review patches because it is difficult to compare changes between pull-request (or at least it was not last time I tried). But email code review is not better on this point neither. And Gerrit do it pretty well.


From: Cohen, Eugene [mailto:***@hp.com]
Sent: 16 May 2015 16:03
To: edk2-***@lists.sourceforge.net<mailto:edk2-***@lists.sourceforge.net>
Subject: [edk2] [PATCH] ArmPlatformPkg: Fix boot on FVP with ArmPlatformPkg SEC at EL3

Olivier (or if you prefer "Dear ArmPkg and ArmPlatformPkg maintainer"),

Here are three patches (in addition to the one I sent earlier) to fix issues I encountered booting the FVP Base platform using SEC from ArmPlatformPkg instead of Trusted Firmware. This boot path is still one where SEC runs at EL3 and the remaining boot stages run at EL2/EL1.

As a preface to these patch sets I should add that I'm unclear about the structure of the ARM platforms. From a directory naming perspective I see an ArmVExpressPkg which supports an FVP-AArch64 platform but I don't know whether this platform is intended to support both the FVP_Base platforms as well as the FVP_VE (Versatile Express) platforms. There are significant differences between these platforms, for example the GIC revision, the memory maps and peripherals are all different. As such it's not clear whether I'm making changes in the right place but that being said I am able to boot to the EFI Shell after these few fixes. Olivier, please advise what the proper way is to handle Versatile Express and Base platform differences.

The fixes break down as follows:

1. The intent on the FVP is to use main memory at 0x880000000 but a PCD value causes PEI and the DXE core to be loaded at a a sub-4GB address. This patch brings all of the DRAM usage into the same area.

2. The architectural timer was not advancing because the FVP Reference Counter, which feeds the processor's timer, must be started by EL3 firmware on the FVP Base platforms

3. Interrupts were not being delivered to the core because EL3 firmware must clear the ProcessorSleep bit in the GICv3 redistributor.

I've been trying to use the github development model for this so I've created a pull request with these changes for your convenience: https://github.com/tianocore/edk2/pull/10 . This tool has a significantly better code review capability than what is possible on a mailing list.

(My next efforts will be around extending EL3 into DXE to support SMM-style initialization but that's a future subject.)


Contributed-under: TianoCore Contribution Agreement 1.0

Signed-off-by: Eugene Cohen <***@hp.com<mailto:***@hp.com>>


-- IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

ARM Limited, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No: 2557590
ARM Holdings plc, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No: 2548782
Loading...