-
Notifications
You must be signed in to change notification settings - Fork 53
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
AArch64: let ubuntu 20.04+ boot from rust hypervisor firmware #262
Conversation
6886d07
to
19fea2c
Compare
57dead0
to
a088e25
Compare
@@ -6,7 +6,7 @@ ENTRY(ram64_start) | |||
ACPI: [0x4020_0000-0x403f_ffff) | |||
kernel: [0x4048_0000-] | |||
The stack start is at the end of the DRAM region. */ | |||
ram_min = 0x40480000; | |||
ram_min = 0x40600000; | |||
|
|||
SECTIONS | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand how this gives more room for GRUB. This is file determines the layout of the ELF executable that is compiled from the project. Your commit message seems to imply that the EFI payload is loaded into the memory just below the RHF executable code. I think this is a bit different to how we do it elsewhere. If you want to keep it the same it would be better to.
Make ram_min = 0x4000000
the true start of RAM and then add a payload_start/payload_end
region to the linker file to save the memory and push back the code region.
@retrage Perhaps you can clarify on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @rbradford -, In current design, 0x40400000 ~ 0x40480000 is reserved for RHF payload. 0x40000000 ~0x40400000 is reserved for DTB and ACPI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So what does that have to do with the EFI payload?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better to load the kernel/grub so that it doesn't overwrite the memory that RHF is loaded at e.g. on x86 RHF is loaded at 1MiB and the EFI binary/kernel is loaded at 2MiB. The correct place to change is probably the value provided to StartInfo::new
for kernel_load_addr
as that is the address that is passed to the EFI PE loader.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a big change that impact CH, RHF and maybe also future change for EDK2 (I'd like to unify the boot process of EDK2 and RHF), so let me think it through carefully.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The points are:
- Assume that 2MB of space is enough to load GRUB. To do this:
- Move
ram_min
to0x40600000
. - Add a run-time payload size check.
I think the change in 6735c61 is little bit ad hoc, but it's acceptable because it works. It's future work to implement better a loading process.
- Move
So, my suggestion is to just add comments to the memory layout assumption. See #262 (comment).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks good to me. Thanks! Once this PR is merged, I'll add integration tests for aarch64.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @retrage for the review - just a few bits from me!
src/fat.rs
Outdated
@@ -232,6 +232,17 @@ fn name_to_str(input: &str, output: &mut [u8]) { | |||
break; | |||
} | |||
} | |||
|
|||
// Fixup for Director or file name, when the length of its name and extend name equals 11. There maybe no |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: typo here - I think you mean directory. Could you also wrap you comment to 80 characters
src/efi/mod.rs
Outdated
@@ -1115,6 +1114,8 @@ pub fn efi_exec( | |||
let mut ct_index = 0; | |||
|
|||
// Populate with FDT table if present | |||
// To boot with ACPI, we can't offer full fdt. Simply, neglect it. See https://github.com/torvalds/linux/blob/d528014517f2b0531862c02865b9d4c908019dc4/arch/arm64/kernel/acpi.c#L203 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you wrap this comment at 80 characters (or close - by putting the URL on a new line of comment.
@@ -614,7 +614,6 @@ pub extern "efiapi" fn install_configuration_table(guid: *mut Guid, table: *mut | |||
|
|||
for entry in ct.iter_mut() { | |||
if entry.vendor_guid == unsafe { *guid } { | |||
entry.vendor_table = table; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be a separate commit.
@@ -1115,6 +1114,8 @@ pub fn efi_exec( | |||
let mut ct_index = 0; | |||
|
|||
// Populate with FDT table if present |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you update the commit title to mark as aarch64 specific.
8e7f997
to
759881b
Compare
Thanks @rbradford -, all comments addressed |
de7c4af
to
117049c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll let you have a go at improving the other commit messages in the series. Let me know if you want help.
RHF: [0x40600000-] | ||
Assuming 2MB is enough to load payload. | ||
The stack start is at the end of the RHF region. */ | ||
ram_min = 0x40600000; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your commit message should focus on why you're doing something. Generally there is no need to repeat what the code does.
e.g.
aarch64: Increase size of EFI payload
In order to support the requirements of a larger GRUB binary extend the size of the
memory used for loading the payload. Since the payload address is hardcoded to
a location in RAM below that of where the RHF binary is loaded increase adjust the
ram_min constant to handle that. This does not require any VMM changes as the
load address in the PE binary will reflect this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @rbradford -, done.
In order to support the requirements of a larger GRUB binary extend the size of the memory used for loading the payload. Since the payload address is hardcoded to a location in RAM below that of where the RHF binary is loaded increase adjust the ram_min constant to handle that. This does not require any VMM changes as the load address in the PE binary will reflect this change. Fixes: cloud-hypervisor#261 Signed-off-by: Jianyong Wu <jianyong.wu@arm.com>
Increasing the maximum number of memory allocations is required to support booting the version of GRUB used in Ubuntu 20.04 for AArch64. Fixes: cloud-hypervisor#261 Signed-off-by: Jianyong Wu <jianyong.wu@arm.com>
This patch increases the size of the byte array to 12 bytes (so that the '.' can be included for files that are named the full 8 + 3 bytes. If the string presented is exactly 11 bytes then adjust it to correctly included the '.' separator. Fixes: cloud-hypervisor#261 Signed-off-by: Jianyong Wu <jianyong.wu@arm.com>
The Linux kernel only enabled ACPI if the provided FDT table is a stub [1]. Although this is a violation of the boot Arm Base Boot Requirement 7.4.3 [2] which requires an FDT table (a stub would be sufficient) for simplicity the FDT table is simply skipped which does not cause issues. Fixes: cloud-hypervisor#261 Signed-off-by: Jianyong Wu <jianyong.wu@arm.com> [1] https://github.com/torvalds/linux/blob/d528014517f2b0531862c02865b9d4c908019dc4/arch/arm64/kernel/acpi.c#L203 [2] https://developer.arm.com/documentation/den0044/latest
This variable will be immediately overwritten later in this function. Signed-off-by: Jianyong Wu <jianyong.wu@arm.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I worked on your commit messages @jongwu - please let me know if you're happy with those changes!
@rbradford -, Great! Thanks! 😄 |
Since PR cloud-hypervisor#262 was merged, we can boot vanilla Ubuntu images using RHF from Cloud Hypervisor. This commit adds integration tests for aarch64. Signed-off-by: Akira Moroo <retrage01@gmail.com>
Since PR cloud-hypervisor#262 was merged, we can boot vanilla Ubuntu images using RHF from Cloud Hypervisor. This commit adds integration tests for aarch64. Signed-off-by: Akira Moroo <retrage01@gmail.com>
Since PR cloud-hypervisor#262 was merged, we can boot vanilla Ubuntu images using RHF from Cloud Hypervisor. This commit adds integration tests for aarch64. Signed-off-by: Akira Moroo <retrage01@gmail.com>
Since PR #262 was merged, we can boot vanilla Ubuntu images using RHF from Cloud Hypervisor. This commit adds integration tests for aarch64. Signed-off-by: Akira Moroo <retrage01@gmail.com>
There are several issues block ubuntu 20.04+ boot from rust hypervisor firmware:
This PR try to fix them one by one:
After above fix, ubuntu 20.04+ can boot from rust hypervisor firmware. Test pass for ubuntu 18.04, 20.04 and 22.04.
Fixes: #261