Re: [PATCH] loader: support loading large files (>=2GB)

[Alistair Francis]

On Tue, May 31, 2022 at 12:59 AM Philippe Mathieu-Daudé via
<qemu-devel@nongnu.org> wrote:
>
> Hi Peter,
>
> On 28/4/22 01:07, Peter Collingbourne wrote:
> > Currently the loader uses int as the return type for various APIs
> > that deal with file sizes, which leads to an error if the file
> > size is >=2GB, as it ends up being interpreted as a negative error
> > code. Furthermore, we do not tolerate short reads, which are possible
> > at least on Linux when attempting to read such large files in one
> > syscall.
> >
> > Fix the first problem by switching to 64-bit types for file sizes,
> > and fix the second by introducing a loop around the read syscall.
>
> Hmm maybe worth rebasing on this patch from Jamie which also
> fixes the format on 32-bit hosts:
> https://lore.kernel.org/qemu-devel/20211111141141.3295094-2-jamie@nuviainc.com/
>
> (Personally I prefer to read ssize_t while reviewing instead
> of int64_t).

I agree with ssize_t as well, I have applied the patch from Jamie
which had fallen through the cracks.

If you can rebase this on top of the RISC-V queue and re-send it I'll
apply the other changes

Alistair

>
> While here, please have a look at this other patch from Jamie:
> https://lore.kernel.org/qemu-devel/20211111141141.3295094-3-jamie@nuviainc.com/
>
> Also, Cc'ing the maintainer:
>
> $ ./scripts/get_maintainer.pl -f hw/core/generic-loader.c
> Alistair Francis <alistair@alistair23.me> (maintainer:Generic Loader)
>
> > Signed-off-by: Peter Collingbourne <pcc@google.com>
> > ---
> >   hw/core/generic-loader.c |  2 +-
> >   hw/core/loader.c         | 44 ++++++++++++++++++++++++----------------
> >   include/hw/loader.h      | 13 ++++++------
> >   3 files changed, 34 insertions(+), 25 deletions(-)
> >
> > diff --git a/hw/core/generic-loader.c b/hw/core/generic-loader.c
> > index c666545aa0..0891fa73c3 100644
> > --- a/hw/core/generic-loader.c
> > +++ b/hw/core/generic-loader.c
> > @@ -67,7 +67,7 @@ static void generic_loader_realize(DeviceState *dev, 
> > Error **errp)
> >       GenericLoaderState *s = GENERIC_LOADER(dev);
> >       hwaddr entry;
> >       int big_endian;
> > -    int size = 0;
> > +    int64_t size = 0;
> >
> >       s->set_pc = false;
> >
> > diff --git a/hw/core/loader.c b/hw/core/loader.c
> > index ca2f2431fb..d07c79c400 100644
> > --- a/hw/core/loader.c
> > +++ b/hw/core/loader.c
> > @@ -115,17 +115,17 @@ ssize_t read_targphys(const char *name,
> >       return did;
> >   }
> >
> > -int load_image_targphys(const char *filename,
> > -                        hwaddr addr, uint64_t max_sz)
> > +int64_t load_image_targphys(const char *filename,
> > +                            hwaddr addr, uint64_t max_sz)
> >   {
> >       return load_image_targphys_as(filename, addr, max_sz, NULL);
> >   }
> >
> >   /* return the size or -1 if error */
> > -int load_image_targphys_as(const char *filename,
> > -                           hwaddr addr, uint64_t max_sz, AddressSpace *as)
> > +int64_t load_image_targphys_as(const char *filename,
> > +                               hwaddr addr, uint64_t max_sz, AddressSpace 
> > *as)
> >   {
> > -    int size;
> > +    int64_t size;
> >
> >       size = get_image_size(filename);
> >       if (size < 0 || size > max_sz) {
> > @@ -139,9 +139,9 @@ int load_image_targphys_as(const char *filename,
> >       return size;
> >   }
> >
> > -int load_image_mr(const char *filename, MemoryRegion *mr)
> > +int64_t load_image_mr(const char *filename, MemoryRegion *mr)
> >   {
> > -    int size;
> > +    int64_t size;
> >
> >       if (!memory_access_is_direct(mr, false)) {
> >           /* Can only load an image into RAM or ROM */
> > @@ -963,7 +963,8 @@ int rom_add_file(const char *file, const char *fw_dir,
> >   {
> >       MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
> >       Rom *rom;
> > -    int rc, fd = -1;
> > +    int fd = -1;
> > +    size_t bytes_read = 0;
> >       char devpath[100];
> >
> >       if (as && mr) {
> > @@ -1003,11 +1004,17 @@ int 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=%d (expected 
> > %zd)\n",
> > -                rom->name, rc, rom->datasize);
> > -        goto err;
> > +    while (bytes_read < rom->datasize) {
> > +        ssize_t rc =
> > +            read(fd, rom->data + bytes_read, rom->datasize - bytes_read);
> > +        if (rc <= 0) {
> > +            fprintf(stderr,
> > +                    "rom: file %-20s: read error: rc=%zd at position %zd "
> > +                    "(expected size %zd)\n",
> > +                    rom->name, rc, bytes_read, rom->datasize);
> > +            goto err;
> > +        }
> > +        bytes_read += rc;
> >       }
> >       close(fd);
> >       rom_insert(rom);
> > @@ -1671,7 +1678,7 @@ typedef struct {
> >       HexLine line;
> >       uint8_t *bin_buf;
> >       hwaddr *start_addr;
> > -    int total_size;
> > +    int64_t total_size;
> >       uint32_t next_address_to_write;
> >       uint32_t current_address;
> >       uint32_t current_rom_index;
> > @@ -1767,8 +1774,8 @@ static int handle_record_type(HexParser *parser)
> >   }
> >
> >   /* return size or -1 if error */
> > -static int parse_hex_blob(const char *filename, hwaddr *addr, uint8_t 
> > *hex_blob,
> > -                          size_t hex_blob_size, AddressSpace *as)
> > +static int64_t parse_hex_blob(const char *filename, hwaddr *addr, uint8_t 
> > *hex_blob,
> > +                              size_t hex_blob_size, AddressSpace *as)
> >   {
> >       bool in_process = false; /* avoid re-enter and
> >                                 * check whether record begin with ':' */
> > @@ -1832,11 +1839,12 @@ out:
> >   }
> >
> >   /* return size or -1 if error */
> > -int load_targphys_hex_as(const char *filename, hwaddr *entry, AddressSpace 
> > *as)
> > +int64_t load_targphys_hex_as(const char *filename, hwaddr *entry,
> > +                             AddressSpace *as)
> >   {
> >       gsize hex_blob_size;
> >       gchar *hex_blob;
> > -    int total_size = 0;
> > +    int64_t total_size = 0;
> >
> >       if (!g_file_get_contents(filename, &hex_blob, &hex_blob_size, NULL)) {
> >           return -1;
> > diff --git a/include/hw/loader.h b/include/hw/loader.h
> > index 5572108ba5..7b09705940 100644
> > --- a/include/hw/loader.h
> > +++ b/include/hw/loader.h
> > @@ -40,8 +40,8 @@ ssize_t load_image_size(const char *filename, void *addr, 
> > size_t size);
> >    *
> >    * Returns the size of the loaded image on success, -1 otherwise.
> >    */
> > -int load_image_targphys_as(const char *filename,
> > -                           hwaddr addr, uint64_t max_sz, AddressSpace *as);
> > +int64_t load_image_targphys_as(const char *filename,
> > +                               hwaddr addr, uint64_t max_sz, AddressSpace 
> > *as);
> >
> >   /**load_targphys_hex_as:
> >    * @filename: Path to the .hex file
> > @@ -53,14 +53,15 @@ int load_image_targphys_as(const char *filename,
> >    *
> >    * Returns the size of the loaded .hex file on success, -1 otherwise.
> >    */
> > -int load_targphys_hex_as(const char *filename, hwaddr *entry, AddressSpace 
> > *as);
> > +int64_t load_targphys_hex_as(const char *filename, hwaddr *entry,
> > +                             AddressSpace *as);
> >
> >   /** load_image_targphys:
> >    * Same as load_image_targphys_as(), but doesn't allow the caller to 
> > specify
> >    * an AddressSpace.
> >    */
> > -int load_image_targphys(const char *filename, hwaddr,
> > -                        uint64_t max_sz);
> > +int64_t load_image_targphys(const char *filename, hwaddr,
> > +                            uint64_t max_sz);
> >
> >   /**
> >    * load_image_mr: load an image into a memory region
> > @@ -73,7 +74,7 @@ int load_image_targphys(const char *filename, hwaddr,
> >    * If the file is larger than the memory region's size the call will fail.
> >    * Returns -1 on failure, or the size of the file.
> >    */
> > -int load_image_mr(const char *filename, MemoryRegion *mr);
> > +int64_t load_image_mr(const char *filename, MemoryRegion *mr);
> >
> >   /* This is the limit on the maximum uncompressed image size that
> >    * load_image_gzipped_buffer() and load_image_gzipped() will read. It 
> > prevents
>
>