| Message ID | 20240627020207.630125-1-gregorhaas1997@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | hw/core/loader: allow loading larger ROMs | expand |
Hi Xingtao, > Can you reproduce this issue? Absolutely! I encountered this when trying to load an OpenSBI payload firmware using the bios option for the QEMU RISC-V virt board. These payload firmwares bundle the entire next boot stage, which in my case is a build of the Linux kernel (which is a standard configuration, supported by tools such as Buildroot [1]). My kernel (configured with the default 64-bit RISC-V configuration) comes in at 9.8M, which is copied into the OpenSBI firmware of final size 10M. Then, I run the following QEMU command: qemu-system-riscv64 -machine virt -m 4G -nographic -bios fw_payload.bin and get the following output: rom: file fw_payload.bin: read error: rc=2147479552 (expected 2303760392) qemu-system-riscv64: could not load firmware 'fw_payload.bin' This is from my development machine, running Arch Linux with kernel 6.9.6 and root filesystem ZFS 2.2.4. Please let me know if you'd like me to make a minimal reproducer for this, or if you need any more information. Thanks, Gregor [1] https://github.com/buildroot/buildroot/blob/master/boot/opensbi/Config.in#L95 On Wed, Jun 26, 2024 at 11:11 PM Xingtao Yao (Fujitsu) < yaoxt.fnst@fujitsu.com> wrote: > Hi, Gregor > > > > The read() syscall is not guaranteed to return all data from a file. The > > default ROM loader implementation currently does not take this into > account, > > instead failing if all bytes are not read at once. This change wraps the > > read() syscall in a do/while loop to ensure all bytes of the ROM are > read. > Can you reproduce this issue? > > Thanks > Xingtao >
Hi Xingtao, Thank you for reproducing this -- I agree with your conclusion and will send a v2 patchset momentarily. Thank you, Gregor On Thu, Jun 27, 2024 at 5:44 PM Xingtao Yao (Fujitsu) < yaoxt.fnst@fujitsu.com> wrote: > Hi, Gregor > > > > >rom: file fw_payload.bin: read error: rc=2147479552 (expected 2303760392) > >qemu-system-riscv64: could not load firmware 'fw_payload.bin' > > Thanks, I was able to reproduce the problem when the images size is > larger than 2147479552. > > > > I found that in my test environment, the maximum value returned by a read > operation is 2147479552, > > which was affected by the operating system. > > > > We can find this limitation in the man page: > > NOTES > > The types size_t and ssize_t are, respectively, unsigned and > signed integer data types specified by POSIX.1. > > > > On Linux, read() (and similar system calls) will transfer at most > 0x7ffff000 (2,147,479,552) bytes, returning the number of bytes actually > transferred. (This is true on both > > 32-bit and 64-bit systems.) > > > > > > > + do { > > > + rc = read(fd, &rom->data[sz], rom->datasize); > > > + if (rc == -1) { > > > + fprintf(stderr, "rom: file %-20s: read error: %s\n", > > > + rom->name, strerror(errno)); > > > + goto err; > > > + } > > > + sz += rc; > > > + } while (sz != rom->datasize); > > I think we can use load_image_size() instead. > > > > > > > > > > *From:* Gregor Haas <gregorhaas1997@gmail.com> > *Sent:* Friday, June 28, 2024 1:35 AM > *To:* Yao, Xingtao/姚 幸涛 <yaoxt.fnst@fujitsu.com> > *Cc:* qemu-devel@nongnu.org; philmd@linaro.org; > richard.henderson@linaro.org > *Subject:* Re: [PATCH] hw/core/loader: allow loading larger ROMs > > > > Hi Xingtao, > > > Can you reproduce this issue? > Absolutely! I encountered this when trying to load an OpenSBI payload > firmware using the bios option for the QEMU RISC-V virt board. These > payload firmwares bundle the entire next boot stage, which in my case is a > build of the Linux kernel (which is a standard configuration, supported by > tools such as Buildroot [1]). My kernel (configured with the default 64-bit > RISC-V configuration) comes in at 9.8M, which is copied into the OpenSBI > firmware of final size 10M. Then, I run the following QEMU command: > > qemu-system-riscv64 -machine virt -m 4G -nographic -bios fw_payload.bin > > and get the following output: > > rom: file fw_payload.bin: read error: rc=2147479552 (expected 2303760392) > qemu-system-riscv64: could not load firmware 'fw_payload.bin' > > This is from my development machine, running Arch Linux with kernel 6.9.6 > and root filesystem ZFS 2.2.4. Please let me know if you'd like me to make > a minimal reproducer for this, or if you need any more information. > > Thanks, > Gregor > > [1] > https://github.com/buildroot/buildroot/blob/master/boot/opensbi/Config.in#L95 > > > > On Wed, Jun 26, 2024 at 11:11 PM Xingtao Yao (Fujitsu) < > yaoxt.fnst@fujitsu.com> wrote: > > Hi, Gregor > > > > The read() syscall is not guaranteed to return all data from a file. The > > default ROM loader implementation currently does not take this into > account, > > instead failing if all bytes are not read at once. This change wraps the > > read() syscall in a do/while loop to ensure all bytes of the ROM are > read. > Can you reproduce this issue? > > Thanks > Xingtao > >
diff --git a/hw/core/loader.c b/hw/core/loader.c index 2f8105d7de..afa26dccb1 100644 --- a/hw/core/loader.c +++ b/hw/core/loader.c @@ -1075,7 +1075,7 @@ ssize_t rom_add_file(const char *file, const char *fw_dir, { MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine()); Rom *rom; - ssize_t rc; + ssize_t rc, sz = 0; int fd = -1; char devpath[100]; @@ -1116,12 +1116,15 @@ ssize_t rom_add_file(const char *file, const char *fw_dir, rom->datasize = rom->romsize; rom->data = g_malloc0(rom->datasize); lseek(fd, 0, SEEK_SET); - rc = read(fd, rom->data, rom->datasize); - if (rc != rom->datasize) { - fprintf(stderr, "rom: file %-20s: read error: rc=%zd (expected %zd)\n", - rom->name, rc, rom->datasize); - goto err; - } + do { + rc = read(fd, &rom->data[sz], rom->datasize); + if (rc == -1) { + fprintf(stderr, "rom: file %-20s: read error: %s\n", + rom->name, strerror(errno)); + goto err; + } + sz += rc; + } while (sz != rom->datasize); close(fd); rom_insert(rom); if (rom->fw_file && fw_cfg) {
The read() syscall is not guaranteed to return all data from a file. The default ROM loader implementation currently does not take this into account, instead failing if all bytes are not read at once. This change wraps the read() syscall in a do/while loop to ensure all bytes of the ROM are read. Signed-off-by: Gregor Haas <gregorhaas1997@gmail.com> --- hw/core/loader.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-)