diff mbox series

[qboot] Place setup_data at location specified by host

Message ID 20220916133603.693135-1-Jason@zx2c4.com (mailing list archive)
State New, archived
Headers show
Series [qboot] Place setup_data at location specified by host | expand

Commit Message

Jason A. Donenfeld Sept. 16, 2022, 1:36 p.m. UTC 
QEMU places setup_data at a particular location, which cannot be
relocated due to it containing self references in absolute address
terms. For this reason, it supplies the intended location in
FW_CFG_SETUP_ADDR, which is what SeaBIOS uses. So use this too in qboot.
This also has the effect of removing the 8k limit on the copied size,
since the header is copied to the right location from the beginning.

Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Ard Biesheuvel <ardb@kernel.org>
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 fw_cfg.c            | 13 ++++++++-----
 include/linuxboot.h |  1 -
 linuxboot.c         |  4 +---
 3 files changed, 9 insertions(+), 9 deletions(-)

Comments

Paolo Bonzini Sept. 19, 2022, 1:35 p.m. UTC | #1 
On Fri, Sep 16, 2022 at 3:42 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> QEMU places setup_data at a particular location, which cannot be
> relocated due to it containing self references in absolute address
> terms. For this reason, it supplies the intended location in
> FW_CFG_SETUP_ADDR, which is what SeaBIOS uses.

(Technically not SeaBIOS, but rather the option rom provided in QEMU).

> So use this too in qboot.
> This also has the effect of removing the 8k limit on the copied size,
> since the header is copied to the right location from the beginning.

This was "intentional" to make qboot more similar to the edk2 linuxboot code.
At the time it seemed like a good idea; of course it was not.

If I understand correctly the bad situation cannot happen because
QEMU and fw_cfg share the same code to decide on the setup_addr.
Unlike with the UEFI handover protocol, qboot always boots the
real mode stub. Still it's a nice fix to remove the 8K limit.

Patch pushed to qboot.git, and the submodule update will be in the
next pull request.

Paolo
diff mbox series

Patch

diff --git a/fw_cfg.c b/fw_cfg.c
index 4b920cf..f3d9605 100644
--- a/fw_cfg.c
+++ b/fw_cfg.c
@@ -269,6 +269,7 @@  void boot_from_fwcfg(void)
 {
 	struct linuxboot_args args;
 	uint32_t kernel_size;
+	enum { HEADER_PEEK_SIZE = 8192 };
 
 	fw_cfg_select(FW_CFG_CMDLINE_SIZE);
 	args.cmdline_size = fw_cfg_readl_le();
@@ -282,15 +283,17 @@  void boot_from_fwcfg(void)
 	kernel_size = fw_cfg_readl_le();
 	fw_cfg_select(FW_CFG_SETUP_SIZE);
 	args.vmlinuz_size = kernel_size + fw_cfg_readl_le();
+	fw_cfg_select(FW_CFG_SETUP_ADDR);
+	args.setup_addr = (void *)fw_cfg_readl_le();
 
 	if (!args.vmlinuz_size)
 		return;
 
 	fw_cfg_select(FW_CFG_SETUP_DATA);
-	fw_cfg_read(args.header, sizeof(args.header));
+	fw_cfg_read(args.setup_addr, HEADER_PEEK_SIZE);
 
 	if (!parse_bzimage(&args)) {
-		uint8_t *header = args.header;
+		uint8_t *header = args.setup_addr;
 
 		if (ldl_p(header) == 0x464c457f)  /* ELF magic */
 			boot_pvh_from_fw_cfg();
@@ -298,9 +301,9 @@  void boot_from_fwcfg(void)
 	}
 
 	/* SETUP_DATA already selected */
-	if (args.setup_size > sizeof(args.header))
-		fw_cfg_read(args.setup_addr + sizeof(args.header),
-			    args.setup_size - sizeof(args.header));
+	if (args.setup_size > HEADER_PEEK_SIZE)
+		fw_cfg_read(args.setup_addr + HEADER_PEEK_SIZE,
+			    args.setup_size - HEADER_PEEK_SIZE);
 
 	fw_cfg_select(FW_CFG_KERNEL_DATA);
 	fw_cfg_read_entry(FW_CFG_KERNEL_DATA, args.kernel_addr, kernel_size);
diff --git a/include/linuxboot.h b/include/linuxboot.h
index 6e865f0..e04c234 100644
--- a/include/linuxboot.h
+++ b/include/linuxboot.h
@@ -10,7 +10,6 @@  struct linuxboot_args {
 
 	/* Input */
 	uint32_t cmdline_size, vmlinuz_size, initrd_size;
-	uint8_t header[8192];
 };
 
 bool parse_bzimage(struct linuxboot_args *args);
diff --git a/linuxboot.c b/linuxboot.c
index 251bcb6..bdb01ee 100644
--- a/linuxboot.c
+++ b/linuxboot.c
@@ -11,7 +11,7 @@  struct hvm_start_info start_info = {0};
 
 bool parse_bzimage(struct linuxboot_args *args)
 {
-	uint8_t *header = args->header;
+	uint8_t *header = args->setup_addr;
 
 	uint32_t real_addr, cmdline_addr, prot_addr, initrd_addr;
 	uint32_t setup_size;
@@ -84,7 +84,6 @@  bool parse_bzimage(struct linuxboot_args *args)
 	args->setup_size = (setup_size+1)*512;
 	args->kernel_size = args->vmlinuz_size - setup_size;
 	args->initrd_addr = (void *)initrd_addr;
-	args->setup_addr = (void *)real_addr;
 	args->kernel_addr = (void *)prot_addr;
 	args->cmdline_addr = (void *)cmdline_addr;
 	return true;
@@ -92,7 +91,6 @@  bool parse_bzimage(struct linuxboot_args *args)
 
 void boot_bzimage(struct linuxboot_args *args)
 {
-	memcpy(args->setup_addr, args->header, sizeof(args->header));
 #ifdef BENCHMARK_HACK
 	/* Exit just before getting to vmlinuz, so that it is easy
 	 * to time/profile the firmware.