diff mbox

[1/5] elf-loader: Allow late loading of elf

Message ID 20170220141943.8426-2-cornelia.huck@de.ibm.com (mailing list archive)
State New, archived
Headers show

Commit Message

Cornelia Huck Feb. 20, 2017, 2:19 p.m. UTC
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(-)

Comments

Thomas Huth Feb. 20, 2017, 3:33 p.m. UTC | #1
On 20.02.2017 15:19, 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.

Could you maybe also explain here why you need such a delayed ELF
loading? Why can't you load the s390-netboot.img at the same time as
s390-ccw.img?

 Thomas
Cornelia Huck Feb. 21, 2017, 10:13 a.m. UTC | #2
(restored cc:s)

On Mon, 20 Feb 2017 16:33:36 +0100
Thomas Huth <thuth@redhat.com> wrote:

> On 20.02.2017 15:19, 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.
> 
> Could you maybe also explain here why you need such a delayed ELF
> loading? Why can't you load the s390-netboot.img at the same time as
> s390-ccw.img?
> 
>  Thomas
> 
> 

Why should we load the netboot image in all cases, and not just when we
actually need it?
Christian Borntraeger Feb. 21, 2017, 10:23 a.m. UTC | #3
On 02/20/2017 04:33 PM, Thomas Huth wrote:
> On 20.02.2017 15:19, 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.
> 
> Could you maybe also explain here why you need such a delayed ELF
> loading? Why can't you load the s390-netboot.img at the same time as
> s390-ccw.img?

Please read the cover letter for some details how to build such a netrom.
This is a simple variant to implement a standard compliant network boot 
today but using a kernel+busybox+scripts.
Long term we certainly want to have a look at implementing something in
the ccw bios, but this (tcp stack, virtio net, etc) will take some (a lot?)
more time. Having a ramdisk+kernel loaded  all the time would be extremely
wasteful.

This patch (1) has another advantage.
Right now we load the ccw bios all the time, even for -kernel xxx boot,
to allow the guest user to use chreipl to reboot from a disk. With this
in place we can rework our ccw bios loader to load the ccw bios lazily
as well.

Christian
Thomas Huth Feb. 24, 2017, 10:44 a.m. UTC | #4
On 21.02.2017 11:23, Christian Borntraeger wrote:
> On 02/20/2017 04:33 PM, Thomas Huth wrote:
>> On 20.02.2017 15:19, 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.
>>
>> Could you maybe also explain here why you need such a delayed ELF
>> loading? Why can't you load the s390-netboot.img at the same time as
>> s390-ccw.img?
> 
> Please read the cover letter for some details how to build such a netrom.

Sure, understood, but I still did not see an explanation why this can't
be loaded as "ROM", too / why it needs to be loaded "delayed"? Does the
image data need to be writable in memory? Or is the information not
available yet at that point in time, whether the user wants to do a
network boot or not? Don't get me wrong, I'm basically fine with this
patch, I'm just missing some explanation *why* you have to do it this way.

> Long term we certainly want to have a look at implementing something in
> the ccw bios, but this (tcp stack, virtio net, etc) will take some (a lot?)
> more time.

Just an idea: There's a complete TFTP-boot-over-virtio-net stack in
SLOF, written by IBM ... maybe you could use that for s390x, too?

> This patch (1) has another advantage.
> Right now we load the ccw bios all the time, even for -kernel xxx boot,
> to allow the guest user to use chreipl to reboot from a disk. With this
> in place we can rework our ccw bios loader to load the ccw bios lazily
> as well.

But this "RAM"-loader here has still the problem that the images are not
restored during reboot/reset, right? ... so before you're going to
implement that as well, let me ask another question: Did you already
have a look at the generic loader device? I think that might already
provide what you're trying to implement here, so your problem could
maybe be solved with some few lines of creating an instance of that
device instead (see
https://lists.gnu.org/archive/html/qemu-devel/2017-01/msg05366.html for
an example how to use that device).

 Thomas
Christian Borntraeger Feb. 24, 2017, 11:09 a.m. UTC | #5
On 02/24/2017 11:44 AM, Thomas Huth wrote:
> On 21.02.2017 11:23, Christian Borntraeger wrote:
>> On 02/20/2017 04:33 PM, Thomas Huth wrote:
>>> On 20.02.2017 15:19, 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.
>>>
>>> Could you maybe also explain here why you need such a delayed ELF
>>> loading? Why can't you load the s390-netboot.img at the same time as
>>> s390-ccw.img?
>>
>> Please read the cover letter for some details how to build such a netrom.
> 
> Sure, understood, but I still did not see an explanation why this can't
> be loaded as "ROM", too / why it needs to be loaded "delayed"? Does the
> image data need to be writable in memory? Or is the information not
> available yet at that point in time, whether the user wants to do a
> network boot or not? Don't get me wrong, I'm basically fine with this
> patch, I'm just missing some explanation *why* you have to do it this way.

As I already wrote, the rom will be big. kernel + ramdisk will take easily
10-30Mbytes. This is loaded twice (in the rom as data and into the guest)
So this will waste lets say 60Mbyte per guest for nothing.

You know yourself that on s390 we do have >100 guests in production environment.
Thomas Huth Feb. 24, 2017, 11:13 a.m. UTC | #6
On 24.02.2017 12:09, Christian Borntraeger wrote:
> On 02/24/2017 11:44 AM, Thomas Huth wrote:
>> On 21.02.2017 11:23, Christian Borntraeger wrote:
>>> On 02/20/2017 04:33 PM, Thomas Huth wrote:
>>>> On 20.02.2017 15:19, 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.
>>>>
>>>> Could you maybe also explain here why you need such a delayed ELF
>>>> loading? Why can't you load the s390-netboot.img at the same time as
>>>> s390-ccw.img?
>>>
>>> Please read the cover letter for some details how to build such a netrom.
>>
>> Sure, understood, but I still did not see an explanation why this can't
>> be loaded as "ROM", too / why it needs to be loaded "delayed"? Does the
>> image data need to be writable in memory? Or is the information not
>> available yet at that point in time, whether the user wants to do a
>> network boot or not? Don't get me wrong, I'm basically fine with this
>> patch, I'm just missing some explanation *why* you have to do it this way.
> 
> As I already wrote, the rom will be big. kernel + ramdisk will take easily
> 10-30Mbytes. This is loaded twice (in the rom as data and into the guest)
> So this will waste lets say 60Mbyte per guest for nothing.

OK, now that's a proper explanation, thanks! ... it would maybe be
helpful for future reviewers to include such a proper statement in the
patch description (or cover letter), too.

 Thomas
Farhan Ali Feb. 24, 2017, 2:21 p.m. UTC | #7
On 02/24/2017 05:44 AM, Thomas Huth wrote:
> On 21.02.2017 11:23, Christian Borntraeger wrote:
>> On 02/20/2017 04:33 PM, Thomas Huth wrote:
>>> On 20.02.2017 15:19, 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.
>>>
>>> Could you maybe also explain here why you need such a delayed ELF
>>> loading? Why can't you load the s390-netboot.img at the same time as
>>> s390-ccw.img?
>>
>> Please read the cover letter for some details how to build such a netrom.
>
> Sure, understood, but I still did not see an explanation why this can't
> be loaded as "ROM", too / why it needs to be loaded "delayed"? Does the
> image data need to be writable in memory? Or is the information not
> available yet at that point in time, whether the user wants to do a
> network boot or not? Don't get me wrong, I'm basically fine with this
> patch, I'm just missing some explanation *why* you have to do it this way.
>
>> Long term we certainly want to have a look at implementing something in
>> the ccw bios, but this (tcp stack, virtio net, etc) will take some (a lot?)
>> more time.
>
> Just an idea: There's a complete TFTP-boot-over-virtio-net stack in
> SLOF, written by IBM ... maybe you could use that for s390x, too?
>
>> This patch (1) has another advantage.
>> Right now we load the ccw bios all the time, even for -kernel xxx boot,
>> to allow the guest user to use chreipl to reboot from a disk. With this
>> in place we can rework our ccw bios loader to load the ccw bios lazily
>> as well.
>
> But this "RAM"-loader here has still the problem that the images are not
> restored during reboot/reset, right? ... so before you're going to
> implement that as well, let me ask another question: Did you already
> have a look at the generic loader device? I think that might already
> provide what you're trying to implement here, so your problem could
> maybe be solved with some few lines of creating an instance of that
> device instead (see
> https://lists.gnu.org/archive/html/qemu-devel/2017-01/msg05366.html for
> an example how to use that device).
>
>  Thomas
>

Hi Thomas,

I did think of the generic loader device, but doesn't the generic loader 
device also load ELF's as ROMs? This would again bring us back to the 
problem of ROMs cannot be loaded after initializations of the ROM. So if 
a user wants to switch boot device from disk to network device we still 
wouldn't be able to load the ROM?

Thanks
Farhan
Thomas Huth Feb. 24, 2017, 6:23 p.m. UTC | #8
On 24.02.2017 15:21, Farhan Ali wrote:
> 
> 
> On 02/24/2017 05:44 AM, Thomas Huth wrote:
>> On 21.02.2017 11:23, Christian Borntraeger wrote:
>>> On 02/20/2017 04:33 PM, Thomas Huth wrote:
>>>> On 20.02.2017 15:19, 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.
>>>>
>>>> Could you maybe also explain here why you need such a delayed ELF
>>>> loading? Why can't you load the s390-netboot.img at the same time as
>>>> s390-ccw.img?
>>>
>>> Please read the cover letter for some details how to build such a
>>> netrom.
>>
>> Sure, understood, but I still did not see an explanation why this can't
>> be loaded as "ROM", too / why it needs to be loaded "delayed"? Does the
>> image data need to be writable in memory? Or is the information not
>> available yet at that point in time, whether the user wants to do a
>> network boot or not? Don't get me wrong, I'm basically fine with this
>> patch, I'm just missing some explanation *why* you have to do it this
>> way.
>>
>>> Long term we certainly want to have a look at implementing something in
>>> the ccw bios, but this (tcp stack, virtio net, etc) will take some (a
>>> lot?)
>>> more time.
>>
>> Just an idea: There's a complete TFTP-boot-over-virtio-net stack in
>> SLOF, written by IBM ... maybe you could use that for s390x, too?
>>
>>> This patch (1) has another advantage.
>>> Right now we load the ccw bios all the time, even for -kernel xxx boot,
>>> to allow the guest user to use chreipl to reboot from a disk. With this
>>> in place we can rework our ccw bios loader to load the ccw bios lazily
>>> as well.
>>
>> But this "RAM"-loader here has still the problem that the images are not
>> restored during reboot/reset, right? ... so before you're going to
>> implement that as well, let me ask another question: Did you already
>> have a look at the generic loader device? I think that might already
>> provide what you're trying to implement here, so your problem could
>> maybe be solved with some few lines of creating an instance of that
>> device instead (see
>> https://lists.gnu.org/archive/html/qemu-devel/2017-01/msg05366.html for
>> an example how to use that device).
>>
>>  Thomas
>>
> 
> Hi Thomas,
> 
> I did think of the generic loader device, but doesn't the generic loader
> device also load ELF's as ROMs? This would again bring us back to the
> problem of ROMs cannot be loaded after initializations of the ROM. So if
> a user wants to switch boot device from disk to network device we still
> wouldn't be able to load the ROM?

OK, never mind, I just had another look at the generic loader code, and
it seems like the ELF loading is also only done there during the
realize() function, not during the reset() function. Sorry, I somehow
got that wrong when I looked at that code for the first time... :-/

 Thomas
diff mbox

Patch

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,