| Message ID | 40eb19fb56b24e10a5eaa99ccb78aff6@dornerworks.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/arm/boot: Increase compliance with kernel arm64 boot protocol. | expand |
Hi Stewart, On 15/10/2018 23:26, Stewart Hildebrand wrote: > "The Image must be placed text_offset bytes from a 2MB aligned base > address anywhere in usable system RAM and called there." > > For the virt board, we write our startup bootloader at the very > bottom of RAM, so that bit can't be used for the image. To avoid > overlap in case the image requests to be loaded at an offset > smaller than our bootloader, we increment the load offset to the > next 2MB. > > This fixes a boot failure for Xen AArch64. > > Signed-off-by: Stewart Hildebrand <stewart.hildebrand@dornerworks.com> > --- > hw/arm/boot.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/hw/arm/boot.c b/hw/arm/boot.c > index 20c71d7d96..559ddbcd53 100644 > --- a/hw/arm/boot.c > +++ b/hw/arm/boot.c > @@ -919,6 +919,16 @@ static uint64_t load_aarch64_image(const char *filename, hwaddr mem_base, > memcpy(&hdrvals, buffer + ARM64_TEXT_OFFSET_OFFSET, sizeof(hdrvals)); > if (hdrvals[1] != 0) { > kernel_load_offset = le64_to_cpu(hdrvals[0]); > + > + /* For the virt board, we write our startup "bootloader" at the very > + * bottom of RAM, so that bit can't be used for the image. To avoid > + * overlap in case the image requests to be loaded at an offset > + * smaller than our bootloader, we increment the load offset to the > + * next 2MB. > + */ > + if (kernel_load_offset < FIXUP_MAX) { I don't understand how this is related to FIXUP_MAX... > + kernel_load_offset += 2 << 20; You can use += 2 * MiB; which is easier to review. > + } > } > } > >
Hi Philippe, On Monday, October 15, 2018 6:05 PM, Philippe Mathieu-Daudé wrote: > Hi Stewart, > > On 15/10/2018 23:26, Stewart Hildebrand wrote: > > + /* For the virt board, we write our startup "bootloader" at the very > > + * bottom of RAM, so that bit can't be used for the image. To avoid > > + * overlap in case the image requests to be loaded at an offset > > + * smaller than our bootloader, we increment the load offset to the > > + * next 2MB. > > + */ > > + if (kernel_load_offset < FIXUP_MAX) { > > I don't understand how this is related to FIXUP_MAX... You're right, my apologies, it's not directly related. write_bootloader() calculates the size of the bootloader like so: len = 0; while (insns[len].fixup != FIXUP_TERMINATOR) { len++; } The size of the bootloader then would be len * sizeof(uint32_t) It would be nice not to have to repeat that logic in load_aarch64_image(). I'll send out a v2 after I take some time to wrap my head around it... > > > + kernel_load_offset += 2 << 20; > > You can use += 2 * MiB; which is easier to review. OK, I will include this in v2.
On Tue, 16 Oct 2018 01:19:35 +0000 Stewart Hildebrand <Stewart.Hildebrand@dornerworks.com> wrote: Hi, Stewart, thanks a lot for picking this up! > On Monday, October 15, 2018 6:05 PM, Philippe Mathieu-Daudé wrote: > > Hi Stewart, > > > > On 15/10/2018 23:26, Stewart Hildebrand wrote: > > > + /* For the virt board, we write our startup > > > "bootloader" at the very > > > + * bottom of RAM, so that bit can't be used for the > > > image. To avoid > > > + * overlap in case the image requests to be loaded > > > at an offset > > > + * smaller than our bootloader, we increment the > > > load offset to the > > > + * next 2MB. > > > + */ > > > + if (kernel_load_offset < FIXUP_MAX) { > > > > I don't understand how this is related to FIXUP_MAX... > > You're right, my apologies, it's not directly related. > write_bootloader() calculates the size of the bootloader like so: > len = 0; > while (insns[len].fixup != FIXUP_TERMINATOR) { > len++; > } > > The size of the bootloader then would be len * sizeof(uint32_t) > > It would be nice not to have to repeat that logic in > load_aarch64_image(). I'll send out a v2 after I take some time to > wrap my head around it... I wonder if this could be done much easier, since TEXT_OFFSET must actually be page aligned. So all we would need is an *upper bound* for the bootloader size, as long as that is below 4K we wouldn't even loose anything. Cheers, Andre. > > > > > + kernel_load_offset += 2 << 20; > > > > You can use += 2 * MiB; which is easier to review. > > OK, I will include this in v2.
On 16 October 2018 at 11:02, Andre Przywara <andre.przywara@arm.com> wrote: > On Tue, 16 Oct 2018 01:19:35 +0000 > Stewart Hildebrand <Stewart.Hildebrand@dornerworks.com> wrote: > > Hi, > > Stewart, thanks a lot for picking this up! > >> On Monday, October 15, 2018 6:05 PM, Philippe Mathieu-Daudé wrote: >> > Hi Stewart, >> > >> > On 15/10/2018 23:26, Stewart Hildebrand wrote: >> > > + /* For the virt board, we write our startup >> > > "bootloader" at the very >> > > + * bottom of RAM, so that bit can't be used for the >> > > image. To avoid >> > > + * overlap in case the image requests to be loaded >> > > at an offset >> > > + * smaller than our bootloader, we increment the >> > > load offset to the >> > > + * next 2MB. >> > > + */ >> > > + if (kernel_load_offset < FIXUP_MAX) { >> > >> > I don't understand how this is related to FIXUP_MAX... >> >> You're right, my apologies, it's not directly related. >> write_bootloader() calculates the size of the bootloader like so: >> len = 0; >> while (insns[len].fixup != FIXUP_TERMINATOR) { >> len++; >> } >> >> The size of the bootloader then would be len * sizeof(uint32_t) >> >> It would be nice not to have to repeat that logic in >> load_aarch64_image(). I'll send out a v2 after I take some time to >> wrap my head around it... > > I wonder if this could be done much easier, since TEXT_OFFSET must > actually be page aligned. So all we would need is an *upper bound* for > the bootloader size, as long as that is below 4K we wouldn't even loose > anything. Yes, I think it would be fine to just set a max bootloader size of 4K and check against that. (If so, assert() in write_bootloader that we did stay below the max.) thanks -- PMM
diff --git a/hw/arm/boot.c b/hw/arm/boot.c index 20c71d7d96..559ddbcd53 100644 --- a/hw/arm/boot.c +++ b/hw/arm/boot.c @@ -919,6 +919,16 @@ static uint64_t load_aarch64_image(const char *filename, hwaddr mem_base, memcpy(&hdrvals, buffer + ARM64_TEXT_OFFSET_OFFSET, sizeof(hdrvals)); if (hdrvals[1] != 0) { kernel_load_offset = le64_to_cpu(hdrvals[0]); + + /* For the virt board, we write our startup "bootloader" at the very + * bottom of RAM, so that bit can't be used for the image. To avoid + * overlap in case the image requests to be loaded at an offset + * smaller than our bootloader, we increment the load offset to the + * next 2MB. + */ + if (kernel_load_offset < FIXUP_MAX) { + kernel_load_offset += 2 << 20; + } } }
"The Image must be placed text_offset bytes from a 2MB aligned base address anywhere in usable system RAM and called there." For the virt board, we write our startup bootloader at the very bottom of RAM, so that bit can't be used for the image. To avoid overlap in case the image requests to be loaded at an offset smaller than our bootloader, we increment the load offset to the next 2MB. This fixes a boot failure for Xen AArch64. Signed-off-by: Stewart Hildebrand <stewart.hildebrand@dornerworks.com> --- hw/arm/boot.c | 10 ++++++++++ 1 file changed, 10 insertions(+)