| Message ID | 20170223122025.10420-2-cornelia.huck@de.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 23.02.2017 13:20, Cornelia Huck wrote: > From: Farhan Ali <alifm@linux.vnet.ibm.com> > > The current QEMU ROM infrastructure rejects late loading of ROMs. > And ELFs are currently loaded as ROM, this prevents delayed loading > of ELFs. So when loading ELF, allow the user to specify if ELF should > be loaded as ROM or not. > > If an ELF is not loaded as ROM, then they are not restored on a > guest reboot/reset and so its upto the user to handle the reloading. > > Signed-off-by: Farhan Ali <alifm@linux.vnet.ibm.com> > Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com> > Cc: Peter Maydell <peter.maydell@linaro.org> > Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com> > --- > hw/core/loader.c | 17 +++++++++++++++-- > include/hw/elf_ops.h | 13 +++++++++---- > include/hw/loader.h | 13 ++++++++++++- > 3 files changed, 36 insertions(+), 7 deletions(-) > > diff --git a/hw/core/loader.c b/hw/core/loader.c > index ee5abd6eb7..9d1af1f6f3 100644 > --- a/hw/core/loader.c > +++ b/hw/core/loader.c > @@ -435,6 +435,19 @@ int load_elf_as(const char *filename, > uint64_t *highaddr, int big_endian, int elf_machine, > int clear_lsb, int data_swab, AddressSpace *as) > { > + return load_elf_ram(filename, translate_fn, translate_opaque, > + pentry, lowaddr, highaddr, big_endian, elf_machine, > + clear_lsb, data_swab, as, true); > +} > + > +/* return < 0 if error, otherwise the number of bytes loaded in memory */ > +int load_elf_ram(const char *filename, > + uint64_t (*translate_fn)(void *, uint64_t), > + void *translate_opaque, uint64_t *pentry, uint64_t *lowaddr, > + uint64_t *highaddr, int big_endian, int elf_machine, > + int clear_lsb, int data_swab, AddressSpace *as, > + bool load_rom) > +{ > int fd, data_order, target_data_order, must_swab, ret = ELF_LOAD_FAILED; > uint8_t e_ident[EI_NIDENT]; <bikeshedpaintingsofeelfreetoignoreme> The patch looks basically fine to me, but I think it's a little bit confusing to have a function called load_elf_ram() which can also be used to load ROMs with a load_rom=1 parameter. If I read "load_elf_ram()", I'd expect a function that can only read ELFs to RAM. So what about adding the "load_rom" parameter to load_elf_as() instead and then making load_elf_ram() a wrapper function to that one with load_rom=0 ? AFAICS there is only one additional caller to load_elf_as (in the generic-loader), so the additional effort here should be OK, I think. </bikeshedpaintingsofeelfreetoignoreme> Thomas
On Sat, 25 Feb 2017 07:05:27 +0100 Thomas Huth <thuth@redhat.com> wrote: > On 23.02.2017 13:20, Cornelia Huck wrote: > > From: Farhan Ali <alifm@linux.vnet.ibm.com> > > > > The current QEMU ROM infrastructure rejects late loading of ROMs. > > And ELFs are currently loaded as ROM, this prevents delayed loading > > of ELFs. So when loading ELF, allow the user to specify if ELF should > > be loaded as ROM or not. > > > > If an ELF is not loaded as ROM, then they are not restored on a > > guest reboot/reset and so its upto the user to handle the reloading. > > > > Signed-off-by: Farhan Ali <alifm@linux.vnet.ibm.com> > > Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com> > > Cc: Peter Maydell <peter.maydell@linaro.org> > > Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com> > > --- > > hw/core/loader.c | 17 +++++++++++++++-- > > include/hw/elf_ops.h | 13 +++++++++---- > > include/hw/loader.h | 13 ++++++++++++- > > 3 files changed, 36 insertions(+), 7 deletions(-) > > > > diff --git a/hw/core/loader.c b/hw/core/loader.c > > index ee5abd6eb7..9d1af1f6f3 100644 > > --- a/hw/core/loader.c > > +++ b/hw/core/loader.c > > @@ -435,6 +435,19 @@ int load_elf_as(const char *filename, > > uint64_t *highaddr, int big_endian, int elf_machine, > > int clear_lsb, int data_swab, AddressSpace *as) > > { > > + return load_elf_ram(filename, translate_fn, translate_opaque, > > + pentry, lowaddr, highaddr, big_endian, elf_machine, > > + clear_lsb, data_swab, as, true); > > +} > > + > > +/* return < 0 if error, otherwise the number of bytes loaded in memory */ > > +int load_elf_ram(const char *filename, > > + uint64_t (*translate_fn)(void *, uint64_t), > > + void *translate_opaque, uint64_t *pentry, uint64_t *lowaddr, > > + uint64_t *highaddr, int big_endian, int elf_machine, > > + int clear_lsb, int data_swab, AddressSpace *as, > > + bool load_rom) > > +{ > > int fd, data_order, target_data_order, must_swab, ret = ELF_LOAD_FAILED; > > uint8_t e_ident[EI_NIDENT]; > > <bikeshedpaintingsofeelfreetoignoreme> > > The patch looks basically fine to me, but I think it's a little bit > confusing to have a function called load_elf_ram() which can also be > used to load ROMs with a load_rom=1 parameter. If I read > "load_elf_ram()", I'd expect a function that can only read ELFs to RAM. > So what about adding the "load_rom" parameter to load_elf_as() instead > and then making load_elf_ram() a wrapper function to that one with > load_rom=0 ? AFAICS there is only one additional caller to load_elf_as > (in the generic-loader), so the additional effort here should be OK, I > think. > > </bikeshedpaintingsofeelfreetoignoreme> > > Thomas > I think both approaches are fine, but I'd prefer to keep the bikeshed colour to avoid a re-spin :)
On Thu, 23 Feb 2017 13:20:21 +0100 Cornelia Huck <cornelia.huck@de.ibm.com> wrote: > From: Farhan Ali <alifm@linux.vnet.ibm.com> > > The current QEMU ROM infrastructure rejects late loading of ROMs. > And ELFs are currently loaded as ROM, this prevents delayed loading > of ELFs. So when loading ELF, allow the user to specify if ELF should > be loaded as ROM or not. > > If an ELF is not loaded as ROM, then they are not restored on a > guest reboot/reset and so its upto the user to handle the reloading. > > Signed-off-by: Farhan Ali <alifm@linux.vnet.ibm.com> > Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com> > Cc: Peter Maydell <peter.maydell@linaro.org> > Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com> > --- > hw/core/loader.c | 17 +++++++++++++++-- > include/hw/elf_ops.h | 13 +++++++++---- > include/hw/loader.h | 13 ++++++++++++- > 3 files changed, 36 insertions(+), 7 deletions(-) > Any further acks for this? Or, put differently, any objections to me sending this in a pullreq for 2.9? > diff --git a/hw/core/loader.c b/hw/core/loader.c > index ee5abd6eb7..9d1af1f6f3 100644 > --- a/hw/core/loader.c > +++ b/hw/core/loader.c > @@ -435,6 +435,19 @@ int load_elf_as(const char *filename, > uint64_t *highaddr, int big_endian, int elf_machine, > int clear_lsb, int data_swab, AddressSpace *as) > { > + return load_elf_ram(filename, translate_fn, translate_opaque, > + pentry, lowaddr, highaddr, big_endian, elf_machine, > + clear_lsb, data_swab, as, true); > +} > + > +/* return < 0 if error, otherwise the number of bytes loaded in memory */ > +int load_elf_ram(const char *filename, > + uint64_t (*translate_fn)(void *, uint64_t), > + void *translate_opaque, uint64_t *pentry, uint64_t *lowaddr, > + uint64_t *highaddr, int big_endian, int elf_machine, > + int clear_lsb, int data_swab, AddressSpace *as, > + bool load_rom) > +{ > int fd, data_order, target_data_order, must_swab, ret = ELF_LOAD_FAILED; > uint8_t e_ident[EI_NIDENT]; > > @@ -473,11 +486,11 @@ int load_elf_as(const char *filename, > if (e_ident[EI_CLASS] == ELFCLASS64) { > ret = load_elf64(filename, fd, translate_fn, translate_opaque, must_swab, > pentry, lowaddr, highaddr, elf_machine, clear_lsb, > - data_swab, as); > + data_swab, as, load_rom); > } else { > ret = load_elf32(filename, fd, translate_fn, translate_opaque, must_swab, > pentry, lowaddr, highaddr, elf_machine, clear_lsb, > - data_swab, as); > + data_swab, as, load_rom); > } > > fail: > diff --git a/include/hw/elf_ops.h b/include/hw/elf_ops.h > index 25659b93be..a172a6068a 100644 > --- a/include/hw/elf_ops.h > +++ b/include/hw/elf_ops.h > @@ -264,7 +264,7 @@ static int glue(load_elf, SZ)(const char *name, int fd, > int must_swab, uint64_t *pentry, > uint64_t *lowaddr, uint64_t *highaddr, > int elf_machine, int clear_lsb, int data_swab, > - AddressSpace *as) > + AddressSpace *as, bool load_rom) > { > struct elfhdr ehdr; > struct elf_phdr *phdr = NULL, *ph; > @@ -403,10 +403,15 @@ static int glue(load_elf, SZ)(const char *name, int fd, > *pentry = ehdr.e_entry - ph->p_vaddr + ph->p_paddr; > } > > - snprintf(label, sizeof(label), "phdr #%d: %s", i, name); > + if (load_rom) { > + snprintf(label, sizeof(label), "phdr #%d: %s", i, name); > > - /* rom_add_elf_program() seize the ownership of 'data' */ > - rom_add_elf_program(label, data, file_size, mem_size, addr, as); > + /* rom_add_elf_program() seize the ownership of 'data' */ > + rom_add_elf_program(label, data, file_size, mem_size, addr, as); > + } else { > + cpu_physical_memory_write(addr, data, file_size); > + g_free(data); > + } > > total_size += mem_size; > if (addr < low) > diff --git a/include/hw/loader.h b/include/hw/loader.h > index 0dbd8d6bf3..3df4d73398 100644 > --- a/include/hw/loader.h > +++ b/include/hw/loader.h > @@ -65,7 +65,7 @@ int load_image_gzipped(const char *filename, hwaddr addr, uint64_t max_sz); > #define ELF_LOAD_WRONG_ENDIAN -4 > const char *load_elf_strerror(int error); > > -/** load_elf_as: > +/** load_elf_ram: > * @filename: Path of ELF file > * @translate_fn: optional function to translate load addresses > * @translate_opaque: opaque data passed to @translate_fn > @@ -81,6 +81,7 @@ const char *load_elf_strerror(int error); > * words and 3 for within doublewords. > * @as: The AddressSpace to load the ELF to. The value of address_space_memory > * is used if nothing is supplied here. > + * @load_rom : Load ELF binary as ROM > * > * Load an ELF file's contents to the emulated system's address space. > * Clients may optionally specify a callback to perform address > @@ -93,6 +94,16 @@ const char *load_elf_strerror(int error); > * If @elf_machine is EM_NONE then the machine type will be read from the > * ELF header and no checks will be carried out against the machine type. > */ > +int load_elf_ram(const char *filename, > + uint64_t (*translate_fn)(void *, uint64_t), > + void *translate_opaque, uint64_t *pentry, uint64_t *lowaddr, > + uint64_t *highaddr, int big_endian, int elf_machine, > + int clear_lsb, int data_swab, AddressSpace *as, > + bool load_rom); > + > +/** load_elf_as: > + * Same as load_elf_ram(), but always loads the elf as ROM > + */ > int load_elf_as(const char *filename, > uint64_t (*translate_fn)(void *, uint64_t), > void *translate_opaque, uint64_t *pentry, uint64_t *lowaddr,
diff --git a/hw/core/loader.c b/hw/core/loader.c index ee5abd6eb7..9d1af1f6f3 100644 --- a/hw/core/loader.c +++ b/hw/core/loader.c @@ -435,6 +435,19 @@ int load_elf_as(const char *filename, uint64_t *highaddr, int big_endian, int elf_machine, int clear_lsb, int data_swab, AddressSpace *as) { + return load_elf_ram(filename, translate_fn, translate_opaque, + pentry, lowaddr, highaddr, big_endian, elf_machine, + clear_lsb, data_swab, as, true); +} + +/* return < 0 if error, otherwise the number of bytes loaded in memory */ +int load_elf_ram(const char *filename, + uint64_t (*translate_fn)(void *, uint64_t), + void *translate_opaque, uint64_t *pentry, uint64_t *lowaddr, + uint64_t *highaddr, int big_endian, int elf_machine, + int clear_lsb, int data_swab, AddressSpace *as, + bool load_rom) +{ int fd, data_order, target_data_order, must_swab, ret = ELF_LOAD_FAILED; uint8_t e_ident[EI_NIDENT]; @@ -473,11 +486,11 @@ int load_elf_as(const char *filename, if (e_ident[EI_CLASS] == ELFCLASS64) { ret = load_elf64(filename, fd, translate_fn, translate_opaque, must_swab, pentry, lowaddr, highaddr, elf_machine, clear_lsb, - data_swab, as); + data_swab, as, load_rom); } else { ret = load_elf32(filename, fd, translate_fn, translate_opaque, must_swab, pentry, lowaddr, highaddr, elf_machine, clear_lsb, - data_swab, as); + data_swab, as, load_rom); } fail: diff --git a/include/hw/elf_ops.h b/include/hw/elf_ops.h index 25659b93be..a172a6068a 100644 --- a/include/hw/elf_ops.h +++ b/include/hw/elf_ops.h @@ -264,7 +264,7 @@ static int glue(load_elf, SZ)(const char *name, int fd, int must_swab, uint64_t *pentry, uint64_t *lowaddr, uint64_t *highaddr, int elf_machine, int clear_lsb, int data_swab, - AddressSpace *as) + AddressSpace *as, bool load_rom) { struct elfhdr ehdr; struct elf_phdr *phdr = NULL, *ph; @@ -403,10 +403,15 @@ static int glue(load_elf, SZ)(const char *name, int fd, *pentry = ehdr.e_entry - ph->p_vaddr + ph->p_paddr; } - snprintf(label, sizeof(label), "phdr #%d: %s", i, name); + if (load_rom) { + snprintf(label, sizeof(label), "phdr #%d: %s", i, name); - /* rom_add_elf_program() seize the ownership of 'data' */ - rom_add_elf_program(label, data, file_size, mem_size, addr, as); + /* rom_add_elf_program() seize the ownership of 'data' */ + rom_add_elf_program(label, data, file_size, mem_size, addr, as); + } else { + cpu_physical_memory_write(addr, data, file_size); + g_free(data); + } total_size += mem_size; if (addr < low) diff --git a/include/hw/loader.h b/include/hw/loader.h index 0dbd8d6bf3..3df4d73398 100644 --- a/include/hw/loader.h +++ b/include/hw/loader.h @@ -65,7 +65,7 @@ int load_image_gzipped(const char *filename, hwaddr addr, uint64_t max_sz); #define ELF_LOAD_WRONG_ENDIAN -4 const char *load_elf_strerror(int error); -/** load_elf_as: +/** load_elf_ram: * @filename: Path of ELF file * @translate_fn: optional function to translate load addresses * @translate_opaque: opaque data passed to @translate_fn @@ -81,6 +81,7 @@ const char *load_elf_strerror(int error); * words and 3 for within doublewords. * @as: The AddressSpace to load the ELF to. The value of address_space_memory * is used if nothing is supplied here. + * @load_rom : Load ELF binary as ROM * * Load an ELF file's contents to the emulated system's address space. * Clients may optionally specify a callback to perform address @@ -93,6 +94,16 @@ const char *load_elf_strerror(int error); * If @elf_machine is EM_NONE then the machine type will be read from the * ELF header and no checks will be carried out against the machine type. */ +int load_elf_ram(const char *filename, + uint64_t (*translate_fn)(void *, uint64_t), + void *translate_opaque, uint64_t *pentry, uint64_t *lowaddr, + uint64_t *highaddr, int big_endian, int elf_machine, + int clear_lsb, int data_swab, AddressSpace *as, + bool load_rom); + +/** load_elf_as: + * Same as load_elf_ram(), but always loads the elf as ROM + */ int load_elf_as(const char *filename, uint64_t (*translate_fn)(void *, uint64_t), void *translate_opaque, uint64_t *pentry, uint64_t *lowaddr,