Re: [Qemu-devel] [PATCH v1 2/4] elf: move elf.h to elf/elf.h and split out types

2019-10-21 Thread Peter Maydell
On Mon, 21 Oct 2019 at 14:54, Laurent Vivier  wrote:
>
> Le 10/09/2019 à 21:34, Alex Bennée a écrit :
> > Most of the users of elf.h just want the standard Elf definitions. The
> > couple that want more than that want an expansion based on ELF_CLASS
> > which can be used for size agnostic code. The later is moved into
> > elf/elf-types.inc.h to make it clearer what it is for. While doing
> > that I also removed the whitespace damage.
> >

> The patch looks good, but why did you call the file "elf-types.inc.h"
> and not "elf-types.h"?

This is our usual convention for files (well, that or .inc.c)
which get included multiple times with different preprocessor
defines to specify how they behave, i.e. which aren't standard
"simple header with multiple-inclusion protection guards".

thanks
-- PMM



Re: [Qemu-devel] [PATCH v1 2/4] elf: move elf.h to elf/elf.h and split out types

2019-10-21 Thread Laurent Vivier
Le 10/09/2019 à 21:34, Alex Bennée a écrit :
> Most of the users of elf.h just want the standard Elf definitions. The
> couple that want more than that want an expansion based on ELF_CLASS
> which can be used for size agnostic code. The later is moved into
> elf/elf-types.inc.h to make it clearer what it is for. While doing
> that I also removed the whitespace damage.
> 
> Signed-off-by: Alex Bennée 
> ---
>  bsd-user/elfload.c   |  2 +-
>  contrib/elf2dmp/qemu_elf.h   |  2 +-
>  disas.c  |  2 +-
>  dump/dump.c  |  2 +-
>  dump/win_dump.c  |  2 +-
>  hw/alpha/dp264.c |  2 +-
>  hw/arm/armv7m.c  |  2 +-
>  hw/arm/boot.c|  2 +-
>  hw/core/loader.c |  3 +-
>  hw/cris/axis_dev88.c |  2 +-
>  hw/cris/boot.c   |  2 +-
>  hw/hppa/machine.c|  2 +-
>  hw/i386/multiboot.c  |  2 +-
>  hw/i386/pc.c |  2 +-
>  hw/lm32/lm32_boards.c|  2 +-
>  hw/lm32/milkymist.c  |  2 +-
>  hw/m68k/an5206.c |  2 +-
>  hw/m68k/mcf5208.c|  2 +-
>  hw/microblaze/boot.c |  2 +-
>  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  |  2 +-
>  hw/nios2/boot.c  |  2 +-
>  hw/openrisc/openrisc_sim.c   |  2 +-
>  hw/pci-host/prep.c   |  2 +-
>  hw/ppc/e500.c|  2 +-
>  hw/ppc/mac_newworld.c|  2 +-
>  hw/ppc/mac_oldworld.c|  2 +-
>  hw/ppc/ppc440_bamboo.c   |  2 +-
>  hw/ppc/prep.c|  2 +-
>  hw/ppc/sam460ex.c|  2 +-
>  hw/ppc/spapr.c   |  2 +-
>  hw/ppc/spapr_vio.c   |  2 +-
>  hw/ppc/virtex_ml507.c|  2 +-
>  hw/riscv/boot.c  |  2 +-
>  hw/s390x/ipl.c   |  2 +-
>  hw/sparc/leon3.c |  2 +-
>  hw/sparc/sun4m.c |  2 +-
>  hw/sparc64/sun4u.c   |  2 +-
>  hw/tricore/tricore_testboard.c   |  2 +-
>  hw/xtensa/sim.c  |  2 +-
>  hw/xtensa/xtfpga.c   |  2 +-
>  include/elf/elf-types.inc.h  | 63 
>  include/{ => elf}/elf.h  | 42 -
>  include/hw/core/generic-loader.h |  2 +-
>  linux-user/arm/cpu_loop.c|  2 +-
>  linux-user/elfload.c |  5 +--
>  linux-user/main.c|  2 +-
>  linux-user/mips/cpu_loop.c   |  2 +-
>  linux-user/riscv/cpu_loop.c  |  2 +-
>  target/arm/arch_dump.c   |  2 +-
>  target/i386/arch_dump.c  |  2 +-
>  target/ppc/arch_dump.c   |  2 +-
>  target/ppc/kvm.c |  2 +-
>  target/s390x/arch_dump.c |  2 +-
>  tcg/arm/tcg-target.inc.c |  2 +-
>  tcg/ppc/tcg-target.inc.c |  2 +-
>  tcg/s390/tcg-target.inc.c|  2 +-
>  tcg/tcg.c|  5 ++-
>  util/getauxval.c |  2 +-
>  62 files changed, 128 insertions(+), 104 deletions(-)
>  create mode 100644 include/elf/elf-types.inc.h

The patch looks good, but why did you call the file "elf-types.inc.h"
and not "elf-types.h"?

Reviewed-by: Laurent Vivier 

Thanks,
LAurent



Re: [Qemu-devel] [PATCH v1 2/4] elf: move elf.h to elf/elf.h and split out types

2019-09-14 Thread Richard Henderson
On 9/10/19 3:34 PM, Alex Bennée wrote:
> Most of the users of elf.h just want the standard Elf definitions. The
> couple that want more than that want an expansion based on ELF_CLASS
> which can be used for size agnostic code. The later is moved into
> elf/elf-types.inc.h to make it clearer what it is for. While doing
> that I also removed the whitespace damage.
> 
> Signed-off-by: Alex Bennée 
> ---

This patch is hard to follow because it moves and splits at the same time.

With this patch split into two,
Reviewed-by: Richard Henderson 


r~



Re: [Qemu-devel] [PATCH v1 2/4] elf: move elf.h to elf/elf.h and split out types

2019-09-11 Thread Alex Bennée


BALATON Zoltan  writes:

> On Tue, 10 Sep 2019, Alex Bennée wrote:
>> diff --git a/include/elf/elf-types.inc.h b/include/elf/elf-types.inc.h
>> new file mode 100644
>> index 000..35163adb2b5
>> --- /dev/null
>> +++ b/include/elf/elf-types.inc.h
>> @@ -0,0 +1,63 @@
>> +/*
>> + * Elf Type Specialisation
>> + *
>> + * Copyright (c) 2019
>> + * Written by Alex Bennée 
>> + *
>> + * This code is licensed under the GNU .
>
> You're missing end of licence sentence here. Also original file did
> not have copyright and licence header so you may want to fix that too
> or leave it out here as well for consistency,

The fault of my header macro - I'll try and fix it up when it's
expanding on the QEMU tree.

I'm going to assume that is should be the whole project license (GPL
v2). Most of the original file dates from 2003 and is Frabrice's commit
with the occasional commit mentioning be copied from Linux.

>
>> + */
>> +
>> +#ifndef _ELF_TYPES_INC_H_
>> +#define _ELF_TYPES_INC_H_
>> +
>> +#ifndef ELF_CLASS
>> +#error you must define ELF_CLASS before including elf-types.inc.h
>> +#else
>> +
>> +#if ELF_CLASS == ELFCLASS32
>> +
>> +#define elfhdr  elf32_hdr
>> +#define elf_phdrelf32_phdr
>> +#define elf_noteelf32_note
>> +#define elf_shdrelf32_shdr
>> +#define elf_sym elf32_sym
>> +#define elf_addr_t  Elf32_Off
>> +#define elf_relaelf32_rela
>> +
>> +#ifdef ELF_USES_RELOCA
>> +# define ELF_RELOC  Elf32_Rela
>> +#else
>> +# define ELF_RELOC  Elf32_Rel
>> +#endif
>> +
>> +#ifndef ElfW
>> +#  define ElfW(x)   Elf32_ ## x
>> +#  define ELFW(x)   ELF32_ ## x
>> +#endif
>> +
>> +#else /* ELF_CLASS == ELFCLASS64 */
>> +
>> +#define elfhdr  elf64_hdr
>> +#define elf_phdrelf64_phdr
>> +#define elf_noteelf64_note
>> +#define elf_shdrelf64_shdr
>> +#define elf_sym elf64_sym
>> +#define elf_addr_t  Elf64_Off
>> +#define elf_relaelf64_rela
>> +
>> +#ifdef ELF_USES_RELOCA
>> +# define ELF_RELOC  Elf64_Rela
>> +#else
>> +# define ELF_RELOC  Elf64_Rel
>> +#endif
>> +
>> +#ifndef ElfW
>> +#  define ElfW(x)   Elf64_ ## x
>> +#  define ELFW(x)   ELF64_ ## x
>> +#endif
>> +
>> +#endif /* ELF_CLASS == ELFCLASS64 */
>> +#endif /* ELF_CLASS */
>> +#else
>> +#error elf-types.inc.h should not be included twice in one compilation unit
>> +#endif /* _ELF_TYPES_INC_H_ */
>> diff --git a/include/elf.h b/include/elf/elf.h
>> similarity index 98%
>> rename from include/elf.h
>> rename to include/elf/elf.h
>> index 3501e0c8d03..2e264c1a7a0 100644
>> --- a/include/elf.h
>> +++ b/include/elf/elf.h
>> @@ -1696,49 +1696,7 @@ struct elf32_fdpic_loadmap {
>> };
>>
>> #ifdef ELF_CLASS
>> -#if ELF_CLASS == ELFCLASS32
>> -
>> -#define elfhdr  elf32_hdr
>> -#define elf_phdrelf32_phdr
>> -#define elf_noteelf32_note
>> -#define elf_shdrelf32_shdr
>> -#define elf_sym elf32_sym
>> -#define elf_addr_t  Elf32_Off
>> -#define elf_rela  elf32_rela
>> -
>> -#ifdef ELF_USES_RELOCA
>> -# define ELF_RELOC  Elf32_Rela
>> -#else
>> -# define ELF_RELOC  Elf32_Rel
>> -#endif
>> -
>> -#else
>> -
>> -#define elfhdr  elf64_hdr
>> -#define elf_phdrelf64_phdr
>> -#define elf_noteelf64_note
>> -#define elf_shdrelf64_shdr
>> -#define elf_sym elf64_sym
>> -#define elf_addr_t  Elf64_Off
>> -#define elf_rela  elf64_rela
>> -
>> -#ifdef ELF_USES_RELOCA
>> -# define ELF_RELOC  Elf64_Rela
>> -#else
>> -# define ELF_RELOC  Elf64_Rel
>> -#endif
>> -
>> -#endif /* ELF_CLASS */
>>
>> -#ifndef ElfW
>> -# if ELF_CLASS == ELFCLASS32
>> -#  define ElfW(x)  Elf32_ ## x
>> -#  define ELFW(x)  ELF32_ ## x
>> -# else
>> -#  define ElfW(x)  Elf64_ ## x
>> -#  define ELFW(x)  ELF64_ ## x
>> -# endif
>> -#endif
>>
>> #endif /* ELF_CLASS */
>
> Is there anything remaining in this #ifdef ELF_CLASS after this patch?
> If not why do you keep it?
>
> Regards,
> BALATON Zoltan


--
Alex Bennée



Re: [Qemu-devel] [PATCH v1 2/4] elf: move elf.h to elf/elf.h and split out types

2019-09-11 Thread BALATON Zoltan

On Tue, 10 Sep 2019, Alex Bennée wrote:

diff --git a/include/elf/elf-types.inc.h b/include/elf/elf-types.inc.h
new file mode 100644
index 000..35163adb2b5
--- /dev/null
+++ b/include/elf/elf-types.inc.h
@@ -0,0 +1,63 @@
+/*
+ * Elf Type Specialisation
+ *
+ * Copyright (c) 2019
+ * Written by Alex Bennée 
+ *
+ * This code is licensed under the GNU .


You're missing end of licence sentence here. Also original file did not 
have copyright and licence header so you may want to fix that too or leave 
it out here as well for consistency,



+ */
+
+#ifndef _ELF_TYPES_INC_H_
+#define _ELF_TYPES_INC_H_
+
+#ifndef ELF_CLASS
+#error you must define ELF_CLASS before including elf-types.inc.h
+#else
+
+#if ELF_CLASS == ELFCLASS32
+
+#define elfhdr  elf32_hdr
+#define elf_phdrelf32_phdr
+#define elf_noteelf32_note
+#define elf_shdrelf32_shdr
+#define elf_sym elf32_sym
+#define elf_addr_t  Elf32_Off
+#define elf_relaelf32_rela
+
+#ifdef ELF_USES_RELOCA
+# define ELF_RELOC  Elf32_Rela
+#else
+# define ELF_RELOC  Elf32_Rel
+#endif
+
+#ifndef ElfW
+#  define ElfW(x)   Elf32_ ## x
+#  define ELFW(x)   ELF32_ ## x
+#endif
+
+#else /* ELF_CLASS == ELFCLASS64 */
+
+#define elfhdr  elf64_hdr
+#define elf_phdrelf64_phdr
+#define elf_noteelf64_note
+#define elf_shdrelf64_shdr
+#define elf_sym elf64_sym
+#define elf_addr_t  Elf64_Off
+#define elf_relaelf64_rela
+
+#ifdef ELF_USES_RELOCA
+# define ELF_RELOC  Elf64_Rela
+#else
+# define ELF_RELOC  Elf64_Rel
+#endif
+
+#ifndef ElfW
+#  define ElfW(x)   Elf64_ ## x
+#  define ELFW(x)   ELF64_ ## x
+#endif
+
+#endif /* ELF_CLASS == ELFCLASS64 */
+#endif /* ELF_CLASS */
+#else
+#error elf-types.inc.h should not be included twice in one compilation unit
+#endif /* _ELF_TYPES_INC_H_ */
diff --git a/include/elf.h b/include/elf/elf.h
similarity index 98%
rename from include/elf.h
rename to include/elf/elf.h
index 3501e0c8d03..2e264c1a7a0 100644
--- a/include/elf.h
+++ b/include/elf/elf.h
@@ -1696,49 +1696,7 @@ struct elf32_fdpic_loadmap {
};

#ifdef ELF_CLASS
-#if ELF_CLASS == ELFCLASS32
-
-#define elfhdr elf32_hdr
-#define elf_phdr   elf32_phdr
-#define elf_note   elf32_note
-#define elf_shdr   elf32_shdr
-#define elf_symelf32_sym
-#define elf_addr_t Elf32_Off
-#define elf_rela  elf32_rela
-
-#ifdef ELF_USES_RELOCA
-# define ELF_RELOC  Elf32_Rela
-#else
-# define ELF_RELOC  Elf32_Rel
-#endif
-
-#else
-
-#define elfhdr elf64_hdr
-#define elf_phdr   elf64_phdr
-#define elf_note   elf64_note
-#define elf_shdr   elf64_shdr
-#define elf_symelf64_sym
-#define elf_addr_t Elf64_Off
-#define elf_rela  elf64_rela
-
-#ifdef ELF_USES_RELOCA
-# define ELF_RELOC  Elf64_Rela
-#else
-# define ELF_RELOC  Elf64_Rel
-#endif
-
-#endif /* ELF_CLASS */

-#ifndef ElfW
-# if ELF_CLASS == ELFCLASS32
-#  define ElfW(x)  Elf32_ ## x
-#  define ELFW(x)  ELF32_ ## x
-# else
-#  define ElfW(x)  Elf64_ ## x
-#  define ELFW(x)  ELF64_ ## x
-# endif
-#endif

#endif /* ELF_CLASS */


Is there anything remaining in this #ifdef ELF_CLASS after this patch? If 
not why do you keep it?


Regards,
BALATON Zoltan


Re: [Qemu-devel] [PATCH v1 2/4] elf: move elf.h to elf/elf.h and split out types

2019-09-10 Thread David Gibson
On Tue, Sep 10, 2019 at 08:34:06PM +0100, Alex Bennée wrote:
> Most of the users of elf.h just want the standard Elf definitions. The
> couple that want more than that want an expansion based on ELF_CLASS
> which can be used for size agnostic code. The later is moved into
> elf/elf-types.inc.h to make it clearer what it is for. While doing
> that I also removed the whitespace damage.
> 
> Signed-off-by: Alex Bennée 

ppc parts
Acked-by: David Gibson 

> ---
>  bsd-user/elfload.c   |  2 +-
>  contrib/elf2dmp/qemu_elf.h   |  2 +-
>  disas.c  |  2 +-
>  dump/dump.c  |  2 +-
>  dump/win_dump.c  |  2 +-
>  hw/alpha/dp264.c |  2 +-
>  hw/arm/armv7m.c  |  2 +-
>  hw/arm/boot.c|  2 +-
>  hw/core/loader.c |  3 +-
>  hw/cris/axis_dev88.c |  2 +-
>  hw/cris/boot.c   |  2 +-
>  hw/hppa/machine.c|  2 +-
>  hw/i386/multiboot.c  |  2 +-
>  hw/i386/pc.c |  2 +-
>  hw/lm32/lm32_boards.c|  2 +-
>  hw/lm32/milkymist.c  |  2 +-
>  hw/m68k/an5206.c |  2 +-
>  hw/m68k/mcf5208.c|  2 +-
>  hw/microblaze/boot.c |  2 +-
>  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  |  2 +-
>  hw/nios2/boot.c  |  2 +-
>  hw/openrisc/openrisc_sim.c   |  2 +-
>  hw/pci-host/prep.c   |  2 +-
>  hw/ppc/e500.c|  2 +-
>  hw/ppc/mac_newworld.c|  2 +-
>  hw/ppc/mac_oldworld.c|  2 +-
>  hw/ppc/ppc440_bamboo.c   |  2 +-
>  hw/ppc/prep.c|  2 +-
>  hw/ppc/sam460ex.c|  2 +-
>  hw/ppc/spapr.c   |  2 +-
>  hw/ppc/spapr_vio.c   |  2 +-
>  hw/ppc/virtex_ml507.c|  2 +-
>  hw/riscv/boot.c  |  2 +-
>  hw/s390x/ipl.c   |  2 +-
>  hw/sparc/leon3.c |  2 +-
>  hw/sparc/sun4m.c |  2 +-
>  hw/sparc64/sun4u.c   |  2 +-
>  hw/tricore/tricore_testboard.c   |  2 +-
>  hw/xtensa/sim.c  |  2 +-
>  hw/xtensa/xtfpga.c   |  2 +-
>  include/elf/elf-types.inc.h  | 63 
>  include/{ => elf}/elf.h  | 42 -
>  include/hw/core/generic-loader.h |  2 +-
>  linux-user/arm/cpu_loop.c|  2 +-
>  linux-user/elfload.c |  5 +--
>  linux-user/main.c|  2 +-
>  linux-user/mips/cpu_loop.c   |  2 +-
>  linux-user/riscv/cpu_loop.c  |  2 +-
>  target/arm/arch_dump.c   |  2 +-
>  target/i386/arch_dump.c  |  2 +-
>  target/ppc/arch_dump.c   |  2 +-
>  target/ppc/kvm.c |  2 +-
>  target/s390x/arch_dump.c |  2 +-
>  tcg/arm/tcg-target.inc.c |  2 +-
>  tcg/ppc/tcg-target.inc.c |  2 +-
>  tcg/s390/tcg-target.inc.c|  2 +-
>  tcg/tcg.c|  5 ++-
>  util/getauxval.c |  2 +-
>  62 files changed, 128 insertions(+), 104 deletions(-)
>  create mode 100644 include/elf/elf-types.inc.h
>  rename include/{ => elf}/elf.h (98%)
> 
> diff --git a/bsd-user/elfload.c b/bsd-user/elfload.c
> index 32378af7b2e..321ee98b86b 100644
> --- a/bsd-user/elfload.c
> +++ b/bsd-user/elfload.c
> @@ -509,7 +509,7 @@ static inline void init_thread(struct target_pt_regs 
> *regs, struct image_info *i
>  #define bswaptls(ptr) bswap32s(ptr)
>  #endif
>  
> -#include "elf.h"
> +#include "elf/elf.h"
>  
>  struct exec
>  {
> diff --git a/contrib/elf2dmp/qemu_elf.h b/contrib/elf2dmp/qemu_elf.h
> index b2f0d9cbc9b..060d148d7f0 100644
> --- a/contrib/elf2dmp/qemu_elf.h
> +++ b/contrib/elf2dmp/qemu_elf.h
> @@ -7,7 +7,7 @@
>  #ifndef ELF2DMP_QEMU_ELF_H
>  #define ELF2DMP_QEMU_ELF_H
>  
> -#include "elf.h"
> +#include "elf/elf.h"
>  
>  typedef struct QEMUCPUSegment {
>  uint32_t selector;
> diff --git a/disas.c b/disas.c
> index 3e2bfa572b1..6f2370cfda7 100644
> --- a/disas.c
> +++ b/disas.c
> @@ -1,7 +1,7 @@
>  /* General "disassemble this chunk" code.  Used for debugging. */
>  #include "qemu/osdep.h"
>  #include "disas/dis-asm.h"
> -#include "elf.h"
> +#include "elf/elf.h"
>  #include "qemu/qemu-print.h"
>  
>  #include "cpu.h"
> diff --git a/dump/dump.c b/dump/dump.c
> index 6fb6e1245ad..6b084a21a2a 100644
> --- a/dump/dump.c
> +++ b/dump/dump.c
> @@ -14,7 +14,7 @@
>  #include "qemu/osdep.h"
>  #include "qemu-common.h"
>  #include "qemu/cutils.h"
> -#include "elf.h"
> +#include "elf/elf.h"
>  #include "cpu.h"
>  #include "exec/hwaddr.h"
>  #include "monitor/monitor.h"
> diff --git a/dump/win_dump.c b/dump/win_dump.c
> index eda2a489742..8232c3cb6b3 100644
> --- a/dump/win_dump.c
> +++ b/dump/win_dump.c
> @@ -11,7 +11,7 @@
>