Re: [Qemu-devel] [PATCH v1 16/17] loader: Add data swap option to load-elf

[Peter Crosthwaite]

On Sun, Feb 28, 2016 at 7:28 AM, Peter Maydell  wrote:
> On 27 February 2016 at 23:14, Peter Crosthwaite
>  wrote:
>> On Tue, Jan 19, 2016 at 9:53 AM, Peter Maydell  
>> wrote:
>>> Can we have a doc comment so we have something that defines what
>>> values data_swab accepts? (it's not just a bool).
>>>
>>
>> This is difficult to capture without writing to whole documentation
>> for load_elf. So here goes:
>
> Thanks; a couple of typos below, but otherwise looks good.
>
>
>> /** load_elf:
>>  * @filename: Path of ELF file
>>  * @translate_fn: optional function to translate load addresses
>>  * @translate_opaque: opaque data passed to @translate_fn
>>  * @pentry: Populated with program entry point. Ignored if NULL.
>>  * @lowaddr: Populated with lowest loaded address. Ignored if NULL.
>>  * @highaddr: Populated with highest loaded address. Ignored if NULL.
>>  * @bigendian: Expected ELF endianness. 0 for LE otherwise BE
>>  * @elf_machine: Expected ELF machine type
>>  * @clear_lsb: Set to mask off LSB of addresses (Some arch's use this
>
> Can we just write out "architectures" here?
>
>>for non-address data)
>>  * @data_swab: Set to order of byte swapping for data. 0 for no swap, 1
>>  * for swapping bytes within halfwords, 2 for bytes within
>>  * words and 3 for within doublewords.
>>  *
>>  * Load an ELF file's contents to the emulated systems address space.
>
> "system's"
>
>>  * Clients may optionally specify a callback to perform address
>>  * translations. @pentry, @lowaddr and @highaddr are optional pointers
>>  * which will be populated with various load information. @bigendian and
>>  * @elf_machine give the expected endianness and machine for the ELF the
>>  * load will fail it the target ELF does not match. Some architectures
>
> "if"
>
>>  * have some arch-specific behaviours that come into effect when their
>
> "architecture"
>

All fixed. Thanks for the pre-review.

Regards,
Peter

>>  * particular values for @elf_machine are set.
>>  */
>
> thanks
> -- PMM

Re: [Qemu-devel] [PATCH v1 16/17] loader: Add data swap option to load-elf

[Peter Maydell]

On 27 February 2016 at 23:14, Peter Crosthwaite
 wrote:
> On Tue, Jan 19, 2016 at 9:53 AM, Peter Maydell  
> wrote:
>> Can we have a doc comment so we have something that defines what
>> values data_swab accepts? (it's not just a bool).
>>
>
> This is difficult to capture without writing to whole documentation
> for load_elf. So here goes:

Thanks; a couple of typos below, but otherwise looks good.


> /** load_elf:
>  * @filename: Path of ELF file
>  * @translate_fn: optional function to translate load addresses
>  * @translate_opaque: opaque data passed to @translate_fn
>  * @pentry: Populated with program entry point. Ignored if NULL.
>  * @lowaddr: Populated with lowest loaded address. Ignored if NULL.
>  * @highaddr: Populated with highest loaded address. Ignored if NULL.
>  * @bigendian: Expected ELF endianness. 0 for LE otherwise BE
>  * @elf_machine: Expected ELF machine type
>  * @clear_lsb: Set to mask off LSB of addresses (Some arch's use this

Can we just write out "architectures" here?

>for non-address data)
>  * @data_swab: Set to order of byte swapping for data. 0 for no swap, 1
>  * for swapping bytes within halfwords, 2 for bytes within
>  * words and 3 for within doublewords.
>  *
>  * Load an ELF file's contents to the emulated systems address space.

"system's"

>  * Clients may optionally specify a callback to perform address
>  * translations. @pentry, @lowaddr and @highaddr are optional pointers
>  * which will be populated with various load information. @bigendian and
>  * @elf_machine give the expected endianness and machine for the ELF the
>  * load will fail it the target ELF does not match. Some architectures

"if"

>  * have some arch-specific behaviours that come into effect when their

"architecture"

>  * particular values for @elf_machine are set.
>  */

thanks
-- PMM

Re: [Qemu-devel] [PATCH v1 16/17] loader: Add data swap option to load-elf

[Peter Crosthwaite]

On Tue, Jan 19, 2016 at 9:53 AM, Peter Maydell  wrote:
> On 18 January 2016 at 07:12, Peter Crosthwaite
>  wrote:
>> Some CPUs are of an opposite data-endianness to other components in the
>> system. Sometimes elfs have the data sections layed out with this CPU
>> data-endianess accounting for when loaded via the CPU, byte swaps
>> (relative to other system components) will occur.
>>
>> The leading example, is ARM's BE32 mode, which is is basically LE with
>> address manipulation on half-word and byte accesses to access the
>> hw/byte reversed address. This means that word data is invariant
>> accross LE and BE32. This also means that instructions are still LE.
>> The expectation is that the elf will be loaded via the CPU in this
>> endianness scheme, which means the data in the elf is reversed at
>> compile time.
>>
>> As QEMU loads via the system memory directly, rather than the CPU, we
>> need a mechanism to reverse elf data endianness to implement this
>> possibility.
>>
>> Signed-off-by: Peter Crosthwaite 
>
>> diff --git a/include/hw/loader.h b/include/hw/loader.h
>> index 33067f8..e542575 100644
>> --- a/include/hw/loader.h
>> +++ b/include/hw/loader.h
>> @@ -35,7 +35,7 @@ const char *load_elf_strerror(int error);
>>  int load_elf(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 clear_lsb, int data_swab);
>>  void load_elf_hdr(const char *filename, void *hdr, bool *is64, Error 
>> **errp);
>>  int load_aout(const char *filename, hwaddr addr, int max_sz,
>>int bswap_needed, hwaddr target_page_size);
>
> Can we have a doc comment so we have something that defines what
> values data_swab accepts? (it's not just a bool).
>

This is difficult to capture without writing to whole documentation
for load_elf. So here goes:

/** load_elf:
 * @filename: Path of ELF file
 * @translate_fn: optional function to translate load addresses
 * @translate_opaque: opaque data passed to @translate_fn
 * @pentry: Populated with program entry point. Ignored if NULL.
 * @lowaddr: Populated with lowest loaded address. Ignored if NULL.
 * @highaddr: Populated with highest loaded address. Ignored if NULL.
 * @bigendian: Expected ELF endianness. 0 for LE otherwise BE
 * @elf_machine: Expected ELF machine type
 * @clear_lsb: Set to mask off LSB of addresses (Some arch's use this
   for non-address data)
 * @data_swab: Set to order of byte swapping for data. 0 for no swap, 1
 * for swapping bytes within halfwords, 2 for bytes within
 * words and 3 for within doublewords.
 *
 * Load an ELF file's contents to the emulated systems address space.
 * Clients may optionally specify a callback to perform address
 * translations. @pentry, @lowaddr and @highaddr are optional pointers
 * which will be populated with various load information. @bigendian and
 * @elf_machine give the expected endianness and machine for the ELF the
 * load will fail it the target ELF does not match. Some architectures
 * have some arch-specific behaviours that come into effect when their
 * particular values for @elf_machine are set.
 */

Regards,
Peter

> Otherwise
> Reviewed-by: Peter Maydell 
>
> thanks
> -- PMM

Re: [Qemu-devel] [PATCH v1 16/17] loader: Add data swap option to load-elf

[Peter Maydell]

On 18 January 2016 at 07:12, Peter Crosthwaite
 wrote:
> Some CPUs are of an opposite data-endianness to other components in the
> system. Sometimes elfs have the data sections layed out with this CPU
> data-endianess accounting for when loaded via the CPU, byte swaps
> (relative to other system components) will occur.
>
> The leading example, is ARM's BE32 mode, which is is basically LE with
> address manipulation on half-word and byte accesses to access the
> hw/byte reversed address. This means that word data is invariant
> accross LE and BE32. This also means that instructions are still LE.
> The expectation is that the elf will be loaded via the CPU in this
> endianness scheme, which means the data in the elf is reversed at
> compile time.
>
> As QEMU loads via the system memory directly, rather than the CPU, we
> need a mechanism to reverse elf data endianness to implement this
> possibility.
>
> Signed-off-by: Peter Crosthwaite 

> diff --git a/include/hw/loader.h b/include/hw/loader.h
> index 33067f8..e542575 100644
> --- a/include/hw/loader.h
> +++ b/include/hw/loader.h
> @@ -35,7 +35,7 @@ const char *load_elf_strerror(int error);
>  int load_elf(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 clear_lsb, int data_swab);
>  void load_elf_hdr(const char *filename, void *hdr, bool *is64, Error **errp);
>  int load_aout(const char *filename, hwaddr addr, int max_sz,
>int bswap_needed, hwaddr target_page_size);

Can we have a doc comment so we have something that defines what
values data_swab accepts? (it's not just a bool).

Otherwise
Reviewed-by: Peter Maydell 

thanks
-- PMM

[Qemu-devel] [PATCH v1 16/17] loader: Add data swap option to load-elf

[Peter Crosthwaite]

Some CPUs are of an opposite data-endianness to other components in the
system. Sometimes elfs have the data sections layed out with this CPU
data-endianess accounting for when loaded via the CPU, byte swaps
(relative to other system components) will occur.

The leading example, is ARM's BE32 mode, which is is basically LE with
address manipulation on half-word and byte accesses to access the
hw/byte reversed address. This means that word data is invariant
accross LE and BE32. This also means that instructions are still LE.
The expectation is that the elf will be loaded via the CPU in this
endianness scheme, which means the data in the elf is reversed at
compile time.

As QEMU loads via the system memory directly, rather than the CPU, we
need a mechanism to reverse elf data endianness to implement this
possibility.

Signed-off-by: Peter Crosthwaite 
---

 hw/alpha/dp264.c   |  4 ++--
 hw/arm/armv7m.c|  2 +-
 hw/arm/boot.c  |  2 +-
 hw/core/loader.c   |  9 ++---
 hw/cris/boot.c |  2 +-
 hw/i386/multiboot.c|  3 ++-
 hw/lm32/lm32_boards.c  |  4 ++--
 hw/lm32/milkymist.c|  2 +-
 hw/m68k/an5206.c   |  2 +-
 hw/m68k/dummy_m68k.c   |  2 +-
 hw/m68k/mcf5208.c  |  2 +-
 hw/microblaze/boot.c   |  4 ++--
 hw/mips/mips_fulong2e.c|  2 +-
 hw/mips/mips_malta.c   |  2 +-
 hw/mips/mips_mipssim.c |  2 +-
 hw/mips/mips_r4k.c |  2 +-
 hw/moxie/moxiesim.c|  3 ++-
 hw/openrisc/openrisc_sim.c |  3 ++-
 hw/pci-host/prep.c |  2 +-
 hw/ppc/e500.c  |  2 +-
 hw/ppc/mac_newworld.c  |  5 +++--
 hw/ppc/mac_oldworld.c  |  5 +++--
 hw/ppc/ppc440_bamboo.c |  3 ++-
 hw/ppc/spapr.c |  6 --
 hw/ppc/virtex_ml507.c  |  3 ++-
 hw/s390x/ipl.c |  4 ++--
 hw/sparc/leon3.c   |  2 +-
 hw/sparc/sun4m.c   |  4 ++--
 hw/sparc64/sun4u.c |  4 ++--
 hw/tricore/tricore_testboard.c |  2 +-
 hw/xtensa/sim.c|  4 ++--
 hw/xtensa/xtfpga.c |  2 +-
 include/hw/elf_ops.h   | 22 +-
 include/hw/loader.h|  2 +-
 34 files changed, 78 insertions(+), 46 deletions(-)

diff --git a/hw/alpha/dp264.c b/hw/alpha/dp264.c
index 27bdaa1..df071fa 100644
--- a/hw/alpha/dp264.c
+++ b/hw/alpha/dp264.c
@@ -109,7 +109,7 @@ static void clipper_init(MachineState *machine)
 }
 size = load_elf(palcode_filename, cpu_alpha_superpage_to_phys,
 NULL, _entry, _low, _high,
-0, EM_ALPHA, 0);
+0, EM_ALPHA, 0, 0);
 if (size < 0) {
 hw_error("could not load palcode '%s'\n", palcode_filename);
 exit(1);
@@ -129,7 +129,7 @@ static void clipper_init(MachineState *machine)
 
 size = load_elf(kernel_filename, cpu_alpha_superpage_to_phys,
 NULL, _entry, _low, _high,
-0, EM_ALPHA, 0);
+0, EM_ALPHA, 0, 0);
 if (size < 0) {
 hw_error("could not load kernel '%s'\n", kernel_filename);
 exit(1);
diff --git a/hw/arm/armv7m.c b/hw/arm/armv7m.c
index a80d2ad..d721e5b 100644
--- a/hw/arm/armv7m.c
+++ b/hw/arm/armv7m.c
@@ -210,7 +210,7 @@ DeviceState *armv7m_init(MemoryRegion *system_memory, int 
mem_size, int num_irq,
 
 if (kernel_filename) {
 image_size = load_elf(kernel_filename, NULL, NULL, , ,
-  NULL, big_endian, EM_ARM, 1);
+  NULL, big_endian, EM_ARM, 1, 0);
 if (image_size < 0) {
 image_size = load_image_targphys(kernel_filename, 0, mem_size);
 lowaddr = 0;
diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index 75f69bf..0de4269 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -700,7 +700,7 @@ static void arm_load_kernel_notify(Notifier *notifier, void 
*data)
 /* Assume that raw images are linux kernels, and ELF images are not.  */
 kernel_size = load_elf(info->kernel_filename, NULL, NULL, _entry,
_low_addr, _high_addr, big_endian,
-   elf_machine, 1);
+   elf_machine, 1, 0);
 if (kernel_size > 0 && have_dtb(info)) {
 /* If there is still some room left at the base of RAM, try and put
  * the DTB there like we do for images loaded with -bios or -pflash.
diff --git a/hw/core/loader.c b/hw/core/loader.c
index 28da8e2..bee6cf5 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -382,7 +382,8 @@ fail:
 /* return < 0 if error, otherwise the number of bytes loaded in memory */
 int load_elf(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