[PATCH v3] Requisite paches for xSplice v3 (not yet posted).

[PATCH v3] Requisite paches for xSplice v3 (not yet posted).

[Konrad Rzeszutek Wilk]

This mismatch of patches came about as I was redoing the
xSplice patches based on reviews and splitting and/or doing some
extra things.

The first patch:

 [PATCH v3 1/5] hypervisor/arm/keyhandler: Declare struct

Allows one to include the keyhandler.h (ARM one) file without having 
extra header file dependencies.

 [PATCH v3 2/5] arm/config: Declare ELFSIZE_[32|64] respectively.

This allows us to use the Elf_ macros - instead of having to do
Elf32_ or Elf64_.

 [PATCH v3 3/5] build: remove .config from /boot when uninstalling.

A little fix.
 [PATCH v3 4/5] mkelf32: Remove the 32-bit hypervisor support.
 [PATCH v3 5/5] mkelf32: Close those file descriptors in the error

And those were found when I was making the xen.gz able to
contain the ELF_NOTE.

Please review at your convience.

 xen/Makefile                 |   1 +
 xen/arch/x86/boot/mkelf32.c  | 110 ++++++++++++++++---------------------------
 xen/include/asm-arm/config.h |   2 +
 xen/include/xen/keyhandler.h |   1 +
 4 files changed, 45 insertions(+), 69 deletions(-)

Konrad Rzeszutek Wilk (5):
      hypervisor/arm/keyhandler: Declare struct cpu_user_regs;
      arm/config: Declare ELFSIZE_[32|64] respectively.
      build: remove .config from /boot when uninstalling.
      mkelf32: Remove the 32-bit hypervisor support.
      mkelf32: Close those file descriptors in the error paths.

[PATCH v3 1/5] hypervisor/arm/keyhandler: Declare struct cpu_user_regs;

[Konrad Rzeszutek Wilk]

in the keyhandler.h file. Otherwise on ARM builds if we
just use the keyhandler file - the compile will fail.

CC: ian.campbell@citrix.com
CC: wei.liu2@citrix.com
CC: stefano.stabellini@citrix.com
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 xen/include/xen/keyhandler.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/xen/include/xen/keyhandler.h b/xen/include/xen/keyhandler.h
index 06c05c8..e361558 100644
--- a/xen/include/xen/keyhandler.h
+++ b/xen/include/xen/keyhandler.h
@@ -19,6 +19,7 @@
  */
 typedef void (keyhandler_fn_t)(unsigned char key);
 
+struct cpu_user_regs;
 /*
  * Callback type for irq_keyhandler.
  *
-- 
2.4.3

[PATCH v3 2/5] arm/config: Declare ELFSIZE_[32|64] respectively.

[Konrad Rzeszutek Wilk]

Otherwise any code that tries to use Elf_* macros instead of
Elf32_ or Elf_64 fails to compile.

CC: ian.campbell@citrix.com
CC: wei.liu2@citrix.com
CC: stefano.stabellini@citrix.com
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 xen/include/asm-arm/config.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/xen/include/asm-arm/config.h b/xen/include/asm-arm/config.h
index bd832df..4ea66bf 100644
--- a/xen/include/asm-arm/config.h
+++ b/xen/include/asm-arm/config.h
@@ -15,8 +15,10 @@
 
 #if defined(CONFIG_ARM_64)
 # define LONG_BYTEORDER 3
+# define ELFSIZE 64
 #else
 # define LONG_BYTEORDER 2
+# define ELFSIZE 32
 #endif
 
 #define BYTES_PER_LONG (1 << LONG_BYTEORDER)
-- 
2.4.3

[PATCH v3 3/5] build: remove .config from /boot when uninstalling.

[Konrad Rzeszutek Wilk]

c/s 361b4f9f0f0d4adc19df428e224a7b8fa62cd392
"build: save generated xen .config" forgot to remove
the config file when uninstalling.

CC: ian.campbell@citrix.com
CC: jbeulich@suse.com
CC: cardoe@cardoe.com
CC: ian.jackson@eu.citrix.com
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 xen/Makefile | 1 +
 1 file changed, 1 insertion(+)

diff --git a/xen/Makefile b/xen/Makefile
index 8b530c2..5d98bcb 100644
--- a/xen/Makefile
+++ b/xen/Makefile
@@ -81,6 +81,7 @@ _uninstall: D=$(DESTDIR)
 _uninstall: T=$(notdir $(TARGET))
 _uninstall: Z=$(CONFIG_XEN_INSTALL_SUFFIX)
 _uninstall:
+	rm -f $(D)$(BOOT_DIR)/$(T)-$(XEN_FULLVERSION).config
 	rm -f $(D)$(BOOT_DIR)/$(T)-$(XEN_FULLVERSION)$(Z)
 	rm -f $(D)$(BOOT_DIR)/$(T)-$(XEN_VERSION).$(XEN_SUBVERSION)$(Z)
 	rm -f $(D)$(BOOT_DIR)/$(T)-$(XEN_VERSION)$(Z)
-- 
2.4.3

[PATCH v3 4/5] mkelf32: Remove the 32-bit hypervisor support.

[Konrad Rzeszutek Wilk]

We do not compile 32-bit hypervisor anymore so the code for
the ELFCLASS32 is effectively dead.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 xen/arch/x86/boot/mkelf32.c | 88 +++++++++++++++------------------------------
 1 file changed, 28 insertions(+), 60 deletions(-)

diff --git a/xen/arch/x86/boot/mkelf32.c b/xen/arch/x86/boot/mkelf32.c
index b369222..993a7ee 100644
--- a/xen/arch/x86/boot/mkelf32.c
+++ b/xen/arch/x86/boot/mkelf32.c
@@ -3,7 +3,7 @@
  * 
  * Usage: elf-prefix <in-image> <out-image> <load-base>
  * 
- * Converts an Elf32 or Elf64 executable binary <in-image> into a simple Elf32
+ * Converts an Elf64 executable binary <in-image> into a simple Elf32
  * image <out-image> comprising a single chunk to be loaded at <load-base>. 
  */
 
@@ -235,7 +235,6 @@ int main(int argc, char **argv)
     int        bytes, todo, i;
 
     Elf32_Ehdr in32_ehdr;
-    Elf32_Phdr in32_phdr;
 
     Elf64_Ehdr in64_ehdr;
     Elf64_Phdr in64_phdr;
@@ -271,70 +270,39 @@ int main(int argc, char **argv)
     big_endian = (*(u16 *)in32_ehdr.e_ident == ((ELFMAG0 << 8) | ELFMAG1));
 
     endianadjust_ehdr32(&in32_ehdr);
-    switch ( in32_ehdr.e_ident[EI_CLASS] )
+    if ( in32_ehdr.e_ident[EI_CLASS] != ELFCLASS64 )
     {
-    case ELFCLASS32:
-        if ( in32_ehdr.e_phentsize != sizeof(in32_phdr) )
-        {
-            fprintf(stderr, "Bad program header size (%d != %d).\n",
-                    (int)in32_ehdr.e_phentsize, (int)sizeof(in32_phdr));
-            return 1;
-        }
-
-        if ( in32_ehdr.e_phnum != 1 )
-        {
-            fprintf(stderr, "Expect precisely 1 program header; found %d.\n",
-                    (int)in32_ehdr.e_phnum);
-            return 1;
-        }
-
-        (void)lseek(infd, in32_ehdr.e_phoff, SEEK_SET);
-        do_read(infd, &in32_phdr, sizeof(in32_phdr));
-        endianadjust_phdr32(&in32_phdr);
-
-        (void)lseek(infd, in32_phdr.p_offset, SEEK_SET);
-        dat_siz = (u32)in32_phdr.p_filesz;
-
-        /* Do not use p_memsz: it does not include BSS alignment padding. */
-        /*mem_siz = (u32)in32_phdr.p_memsz;*/
-        mem_siz = (u32)(final_exec_addr - in32_phdr.p_vaddr);
-        break;
-
-    case ELFCLASS64:
-        (void)lseek(infd, 0, SEEK_SET);
-        do_read(infd, &in64_ehdr, sizeof(in64_ehdr));
-        endianadjust_ehdr64(&in64_ehdr);
-
-        if ( in64_ehdr.e_phentsize != sizeof(in64_phdr) )
-        {
-            fprintf(stderr, "Bad program header size (%d != %d).\n",
-                    (int)in64_ehdr.e_phentsize, (int)sizeof(in64_phdr));
-            return 1;
-        }
+        fprintf(stderr, "Bad program header class - we only do 64-bit!.\n");
+        return 1;
+    }
+    (void)lseek(infd, 0, SEEK_SET);
+    do_read(infd, &in64_ehdr, sizeof(in64_ehdr));
+    endianadjust_ehdr64(&in64_ehdr);
 
-        if ( in64_ehdr.e_phnum != 1 )
-        {
-            fprintf(stderr, "Expect precisly 1 program header; found %d.\n",
-                    (int)in64_ehdr.e_phnum);
-            return 1;
-        }
+    if ( in64_ehdr.e_phentsize != sizeof(in64_phdr) )
+    {
+        fprintf(stderr, "Bad program header size (%d != %d).\n",
+                (int)in64_ehdr.e_phentsize, (int)sizeof(in64_phdr));
+        return 1;
+    }
 
-        (void)lseek(infd, in64_ehdr.e_phoff, SEEK_SET);
-        do_read(infd, &in64_phdr, sizeof(in64_phdr));
-        endianadjust_phdr64(&in64_phdr);
+    if ( in64_ehdr.e_phnum != 1 )
+    {
+        fprintf(stderr, "Expect precisly 1 program header; found %d.\n",
+                (int)in64_ehdr.e_phnum);
+        return 1;
+    }
 
-        (void)lseek(infd, in64_phdr.p_offset, SEEK_SET);
-        dat_siz = (u32)in64_phdr.p_filesz;
+    (void)lseek(infd, in64_ehdr.e_phoff, SEEK_SET);
+    do_read(infd, &in64_phdr, sizeof(in64_phdr));
+    endianadjust_phdr64(&in64_phdr);
 
-        /* Do not use p_memsz: it does not include BSS alignment padding. */
-        /*mem_siz = (u32)in64_phdr.p_memsz;*/
-        mem_siz = (u32)(final_exec_addr - in64_phdr.p_vaddr);
-        break;
+    (void)lseek(infd, in64_phdr.p_offset, SEEK_SET);
+    dat_siz = (u32)in64_phdr.p_filesz;
 
-    default:
-        fprintf(stderr, "Input image must be a 32- or 64-bit Elf image.\n");
-        return 1;
-    }
+    /* Do not use p_memsz: it does not include BSS alignment padding. */
+    /*mem_siz = (u32)in64_phdr.p_memsz;*/
+    mem_siz = (u32)(final_exec_addr - in64_phdr.p_vaddr);
 
     /*
      * End the image on a page boundary. This gets round alignment bugs
-- 
2.4.3

[PATCH v3 5/5] mkelf32: Close those file descriptors in the error paths.

[Konrad Rzeszutek Wilk]

While we are operating here we may as well fix some of the
file descriptor leaks.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 xen/arch/x86/boot/mkelf32.c | 28 ++++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)

diff --git a/xen/arch/x86/boot/mkelf32.c b/xen/arch/x86/boot/mkelf32.c
index 993a7ee..890ae6d 100644
--- a/xen/arch/x86/boot/mkelf32.c
+++ b/xen/arch/x86/boot/mkelf32.c
@@ -230,9 +230,9 @@ int main(int argc, char **argv)
     u64        final_exec_addr;
     u32        loadbase, dat_siz, mem_siz;
     char      *inimage, *outimage;
-    int        infd, outfd;
+    int        infd, outfd = -1;
     char       buffer[1024];
-    int        bytes, todo, i;
+    int        bytes, todo, i, rc = 1;
 
     Elf32_Ehdr in32_ehdr;
 
@@ -256,7 +256,7 @@ int main(int argc, char **argv)
     {
         fprintf(stderr, "Failed to open input image '%s': %d (%s).\n",
                 inimage, errno, strerror(errno));
-        return 1;
+        goto out;
     }
 
     do_read(infd, &in32_ehdr, sizeof(in32_ehdr));
@@ -264,7 +264,7 @@ int main(int argc, char **argv)
          (in32_ehdr.e_ident[EI_DATA] != ELFDATA2LSB) )
     {
         fprintf(stderr, "Input image must be a little-endian Elf image.\n");
-        return 1;
+        goto out;
     }
 
     big_endian = (*(u16 *)in32_ehdr.e_ident == ((ELFMAG0 << 8) | ELFMAG1));
@@ -273,7 +273,7 @@ int main(int argc, char **argv)
     if ( in32_ehdr.e_ident[EI_CLASS] != ELFCLASS64 )
     {
         fprintf(stderr, "Bad program header class - we only do 64-bit!.\n");
-        return 1;
+        goto out;
     }
     (void)lseek(infd, 0, SEEK_SET);
     do_read(infd, &in64_ehdr, sizeof(in64_ehdr));
@@ -283,14 +283,14 @@ int main(int argc, char **argv)
     {
         fprintf(stderr, "Bad program header size (%d != %d).\n",
                 (int)in64_ehdr.e_phentsize, (int)sizeof(in64_phdr));
-        return 1;
+        goto out;
     }
 
     if ( in64_ehdr.e_phnum != 1 )
     {
         fprintf(stderr, "Expect precisly 1 program header; found %d.\n",
                 (int)in64_ehdr.e_phnum);
-        return 1;
+        goto out;
     }
 
     (void)lseek(infd, in64_ehdr.e_phoff, SEEK_SET);
@@ -327,7 +327,7 @@ int main(int argc, char **argv)
     {
         fprintf(stderr, "Failed to open output image '%s': %d (%s).\n",
                 outimage, errno, strerror(errno));
-        return 1;
+        goto out;
     }
 
     endianadjust_ehdr32(&out_ehdr);
@@ -339,7 +339,7 @@ int main(int argc, char **argv)
     if ( (bytes = RAW_OFFSET - sizeof(out_ehdr) - sizeof(out_phdr)) < 0 )
     {
         fprintf(stderr, "Header overflow.\n");
-        return 1;
+        goto out;
     }
     do_write(outfd, buffer, bytes);
 
@@ -358,10 +358,14 @@ int main(int argc, char **argv)
     do_write(outfd, out_shstrtab, sizeof(out_shstrtab));
     do_write(outfd, buffer, 4-((sizeof(out_shstrtab)+dat_siz)&3));
 
-    close(infd);
-    close(outfd);
+    rc = 0;
+out:
+    if ( infd != -1 )
+        close(infd);
+    if ( outfd != -1 )
+        close(outfd);
 
-    return 0;
+    return rc;
 }
 
 /*
-- 
2.4.3

Re: [PATCH v3 1/5] hypervisor/arm/keyhandler: Declare struct cpu_user_regs;

[Jan Beulich]

>>> On 12.02.16 at 04:08, <konrad.wilk@oracle.com> wrote:
> in the keyhandler.h file. Otherwise on ARM builds if we
> just use the keyhandler file - the compile will fail.
> 
> CC: ian.campbell@citrix.com 
> CC: wei.liu2@citrix.com 
> CC: stefano.stabellini@citrix.com 
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

See commit d1d181328d.

Jan

Re: [PATCH v3 2/5] arm/config: Declare ELFSIZE_[32|64] respectively.

[Stefano Stabellini]

On Thu, 11 Feb 2016, Konrad Rzeszutek Wilk wrote:
> Otherwise any code that tries to use Elf_* macros instead of
> Elf32_ or Elf_64 fails to compile.
> 
> CC: ian.campbell@citrix.com
> CC: wei.liu2@citrix.com
> CC: stefano.stabellini@citrix.com
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>  xen/include/asm-arm/config.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/xen/include/asm-arm/config.h b/xen/include/asm-arm/config.h
> index bd832df..4ea66bf 100644
> --- a/xen/include/asm-arm/config.h
> +++ b/xen/include/asm-arm/config.h
> @@ -15,8 +15,10 @@
>  
>  #if defined(CONFIG_ARM_64)
>  # define LONG_BYTEORDER 3
> +# define ELFSIZE 64
>  #else
>  # define LONG_BYTEORDER 2
> +# define ELFSIZE 32
>  #endif

I wonder if we should use ELF64 on ARM32 too, for simplicity (x86 only
uses ELF64) and because ARM32 is LPAE.

Re: [PATCH v3 1/5] hypervisor/arm/keyhandler: Declare struct cpu_user_regs;

[Stefano Stabellini]

On Thu, 11 Feb 2016, Konrad Rzeszutek Wilk wrote:
> in the keyhandler.h file. Otherwise on ARM builds if we
> just use the keyhandler file - the compile will fail.
> 
> CC: ian.campbell@citrix.com
> CC: wei.liu2@citrix.com
> CC: stefano.stabellini@citrix.com
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>  xen/include/xen/keyhandler.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/xen/include/xen/keyhandler.h b/xen/include/xen/keyhandler.h
> index 06c05c8..e361558 100644
> --- a/xen/include/xen/keyhandler.h
> +++ b/xen/include/xen/keyhandler.h
> @@ -19,6 +19,7 @@
>   */
>  typedef void (keyhandler_fn_t)(unsigned char key);
>  
> +struct cpu_user_regs;
>  /*
>   * Callback type for irq_keyhandler.
>   *

I think that the right fix would be to #include <asm/processor.h>.

Re: [PATCH v3 1/5] hypervisor/arm/keyhandler: Declare struct cpu_user_regs;

[Jan Beulich]

>>> On 12.02.16 at 12:37, <stefano.stabellini@eu.citrix.com> wrote:
> On Thu, 11 Feb 2016, Konrad Rzeszutek Wilk wrote:
>> --- a/xen/include/xen/keyhandler.h
>> +++ b/xen/include/xen/keyhandler.h
>> @@ -19,6 +19,7 @@
>>   */
>>  typedef void (keyhandler_fn_t)(unsigned char key);
>>  
>> +struct cpu_user_regs;
>>  /*
>>   * Callback type for irq_keyhandler.
>>   *
> 
> I think that the right fix would be to #include <asm/processor.h>.

I disagree - for one this isn't where the structure gets defined,
and then this is the "include everything everywhere" attitude
which tends to needlessly slow down builds (avoiding the need
to include everything everywhere is actually one of the
purposes of such forward declarations, which allows going as
far as having the full definition in just a single source file).

Jan

Re: [PATCH v3 1/5] hypervisor/arm/keyhandler: Declare struct cpu_user_regs;

[Stefano Stabellini]

On Fri, 12 Feb 2016, Jan Beulich wrote:
> >>> On 12.02.16 at 12:37, <stefano.stabellini@eu.citrix.com> wrote:
> > On Thu, 11 Feb 2016, Konrad Rzeszutek Wilk wrote:
> >> --- a/xen/include/xen/keyhandler.h
> >> +++ b/xen/include/xen/keyhandler.h
> >> @@ -19,6 +19,7 @@
> >>   */
> >>  typedef void (keyhandler_fn_t)(unsigned char key);
> >>  
> >> +struct cpu_user_regs;
> >>  /*
> >>   * Callback type for irq_keyhandler.
> >>   *
> > 
> > I think that the right fix would be to #include <asm/processor.h>.
> 
> I disagree - for one this isn't where the structure gets defined,

Ah, sorry, it is on ARM (actually to be precise, the header files are
asm-arm/arm64/processor.h and asm-arm/arm32/processor.h, included by
asm-arm/processor.h depending on the architecture). I see now that the
corresponding x86 header is actually arch-x86/xen.h.


> and then this is the "include everything everywhere" attitude
> which tends to needlessly slow down builds (avoiding the need
> to include everything everywhere is actually one of the
> purposes of such forward declarations, which allows going as
> far as having the full definition in just a single source file).

Fair enough, but keyhandler.h directly uses struct cpu_user_regs as
parameter in two functions, so I think it would be right to include an
header file with the definition. It's too bad that the names for the ARM
headers and the x86 headers don't match. Given that, I understand why
Konrad went with this solution instead, and I am OK with it.

Re: [PATCH v3 1/5] hypervisor/arm/keyhandler: Declare struct cpu_user_regs;

[Konrad Rzeszutek Wilk]

On Fri, Feb 12, 2016 at 01:51:28AM -0700, Jan Beulich wrote:
> >>> On 12.02.16 at 04:08, <konrad.wilk@oracle.com> wrote:
> > in the keyhandler.h file. Otherwise on ARM builds if we
> > just use the keyhandler file - the compile will fail.
> > 
> > CC: ian.campbell@citrix.com 
> > CC: wei.liu2@citrix.com 
> > CC: stefano.stabellini@citrix.com 
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 
> See commit d1d181328d.

Weird. When I did git rebase origin/staging it didn't
notice that commit. And it just applied it on top - so I had
two 'struct cpu_regs;' in the header file.

Re: [PATCH v3 3/5] build: remove .config from /boot when uninstalling.

[Doug Goldstein]

[-- Attachment #1.1: Type: text/plain, Size: 453 bytes --]

On 2/11/16 9:08 PM, Konrad Rzeszutek Wilk wrote:
> c/s 361b4f9f0f0d4adc19df428e224a7b8fa62cd392
> "build: save generated xen .config" forgot to remove
> the config file when uninstalling.
> 
> CC: ian.campbell@citrix.com
> CC: jbeulich@suse.com
> CC: cardoe@cardoe.com
> CC: ian.jackson@eu.citrix.com
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Reviewed-by: Doug Goldstein <cardoe@cardoe.com>


-- 
Doug Goldstein


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 959 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v3 2/5] arm/config: Declare ELFSIZE_[32|64] respectively.

[Konrad Rzeszutek Wilk]

On Fri, Feb 12, 2016 at 11:26:10AM +0000, Stefano Stabellini wrote:
> On Thu, 11 Feb 2016, Konrad Rzeszutek Wilk wrote:
> > Otherwise any code that tries to use Elf_* macros instead of
> > Elf32_ or Elf_64 fails to compile.
> > 
> > CC: ian.campbell@citrix.com
> > CC: wei.liu2@citrix.com
> > CC: stefano.stabellini@citrix.com
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > ---
> >  xen/include/asm-arm/config.h | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/xen/include/asm-arm/config.h b/xen/include/asm-arm/config.h
> > index bd832df..4ea66bf 100644
> > --- a/xen/include/asm-arm/config.h
> > +++ b/xen/include/asm-arm/config.h
> > @@ -15,8 +15,10 @@
> >  
> >  #if defined(CONFIG_ARM_64)
> >  # define LONG_BYTEORDER 3
> > +# define ELFSIZE 64
> >  #else
> >  # define LONG_BYTEORDER 2
> > +# define ELFSIZE 32
> >  #endif
> 
> I wonder if we should use ELF64 on ARM32 too, for simplicity (x86 only
> uses ELF64) and because ARM32 is LPAE.

Done. And this resolves also the question Jan raised in the design
document about ARM32 and the ELF payload declaring the ELF only in
64-bit syntax.

Thanks! Updated the patch to be:
 
P.S.
It compiles without trouble.
>From 7756ddc1e2aa0be487df05ce76577c6fa15a75ce Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Date: Fri, 5 Feb 2016 10:44:45 -0500
Subject: [PATCH] arm/config: Declare ELFSIZE_64.

Otherwise any code that tries to use Elf_* macros would
require us to use Elf64_* types instead of the more
friendly Elf_ one.

This is OK to do since 32-bit ARM uses LPAE mode.

CC: ian.campbell@citrix.com
CC: wei.liu2@citrix.com
CC: stefano.stabellini@citrix.com
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 xen/include/asm-arm/config.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/xen/include/asm-arm/config.h b/xen/include/asm-arm/config.h
index bd832df..d5321b4 100644
--- a/xen/include/asm-arm/config.h
+++ b/xen/include/asm-arm/config.h
@@ -15,8 +15,10 @@
 
 #if defined(CONFIG_ARM_64)
 # define LONG_BYTEORDER 3
+# define ELFSIZE 64
 #else
 # define LONG_BYTEORDER 2
+# define ELFSIZE 64
 #endif
 
 #define BYTES_PER_LONG (1 << LONG_BYTEORDER)
-- 
2.4.3

Re: [PATCH v3 1/5] hypervisor/arm/keyhandler: Declare struct cpu_user_regs;

[Jan Beulich]

>>> On 12.02.16 at 13:46, <stefano.stabellini@eu.citrix.com> wrote:
> On Fri, 12 Feb 2016, Jan Beulich wrote:
>> and then this is the "include everything everywhere" attitude
>> which tends to needlessly slow down builds (avoiding the need
>> to include everything everywhere is actually one of the
>> purposes of such forward declarations, which allows going as
>> far as having the full definition in just a single source file).
> 
> Fair enough, but keyhandler.h directly uses struct cpu_user_regs as
> parameter in two functions, so I think it would be right to include an
> header file with the definition.

No, full definitions need only be visible when structure instances
are used, not when just pointers to them get declared. The sole
reason why the forward declaration is needed is because
otherwise declaration and definition of the function wouldn't
match, as without the forward declaration the scope of the
structure is that of the function (instead of the global scope).

Jan

Re: [PATCH v3 1/5] hypervisor/arm/keyhandler: Declare struct cpu_user_regs;

[Jan Beulich]

>>> On 12.02.16 at 15:06, <konrad.wilk@oracle.com> wrote:
> On Fri, Feb 12, 2016 at 01:51:28AM -0700, Jan Beulich wrote:
>> >>> On 12.02.16 at 04:08, <konrad.wilk@oracle.com> wrote:
>> > in the keyhandler.h file. Otherwise on ARM builds if we
>> > just use the keyhandler file - the compile will fail.
>> > 
>> > CC: ian.campbell@citrix.com 
>> > CC: wei.liu2@citrix.com 
>> > CC: stefano.stabellini@citrix.com 
>> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>> 
>> See commit d1d181328d.
> 
> Weird. When I did git rebase origin/staging it didn't
> notice that commit. And it just applied it on top - so I had
> two 'struct cpu_regs;' in the header file.

That's likely because I moved the declaration down a few lines
while applying.

Jan

Re: [PATCH v3 2/5] arm/config: Declare ELFSIZE_[32|64] respectively.

[Jan Beulich]

>>> On 12.02.16 at 15:17, <konrad.wilk@oracle.com> wrote:
> --- a/xen/include/asm-arm/config.h
> +++ b/xen/include/asm-arm/config.h
> @@ -15,8 +15,10 @@
>  
>  #if defined(CONFIG_ARM_64)
>  # define LONG_BYTEORDER 3
> +# define ELFSIZE 64
>  #else
>  # define LONG_BYTEORDER 2
> +# define ELFSIZE 64
>  #endif

Leaving the question - why twice instead of outside the #ifdef?

Jan

Re: [PATCH v3 2/5] arm/config: Declare ELFSIZE_[32|64] respectively.

[Stefano Stabellini]

On Fri, 12 Feb 2016, Jan Beulich wrote:
> >>> On 12.02.16 at 15:17, <konrad.wilk@oracle.com> wrote:
> > --- a/xen/include/asm-arm/config.h
> > +++ b/xen/include/asm-arm/config.h
> > @@ -15,8 +15,10 @@
> >  
> >  #if defined(CONFIG_ARM_64)
> >  # define LONG_BYTEORDER 3
> > +# define ELFSIZE 64
> >  #else
> >  # define LONG_BYTEORDER 2
> > +# define ELFSIZE 64
> >  #endif
> 
> Leaving the question - why twice instead of outside the #ifdef?

Right, please move it out of the #ifdef.

Re: [PATCH v3 2/5] arm/config: Declare ELFSIZE_[32|64] respectively.

[Konrad Rzeszutek Wilk]

On Fri, Feb 12, 2016 at 03:26:14PM +0000, Stefano Stabellini wrote:
> On Fri, 12 Feb 2016, Jan Beulich wrote:
> > >>> On 12.02.16 at 15:17, <konrad.wilk@oracle.com> wrote:
> > > --- a/xen/include/asm-arm/config.h
> > > +++ b/xen/include/asm-arm/config.h
> > > @@ -15,8 +15,10 @@
> > >  
> > >  #if defined(CONFIG_ARM_64)
> > >  # define LONG_BYTEORDER 3
> > > +# define ELFSIZE 64
> > >  #else
> > >  # define LONG_BYTEORDER 2
> > > +# define ELFSIZE 64
> > >  #endif
> > 
> > Leaving the question - why twice instead of outside the #ifdef?
> 
> Right, please move it out of the #ifdef.

Done!

>From 32a062c119091f2f3f6a4c540a8098e97c273dd2 Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Date: Fri, 5 Feb 2016 10:44:45 -0500
Subject: [PATCH] arm/config: Declare ELFSIZE_64.

Otherwise any code that tries to use Elf_* macros would
require us to use Elf64_* types instead of the more
friendly Elf_ one.

This is OK to do since 32-bit ARM uses LPAE mode.

CC: ian.campbell@citrix.com
CC: wei.liu2@citrix.com
CC: stefano.stabellini@citrix.com
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 xen/include/asm-arm/config.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/xen/include/asm-arm/config.h b/xen/include/asm-arm/config.h
index bd832df..a1b968d 100644
--- a/xen/include/asm-arm/config.h
+++ b/xen/include/asm-arm/config.h
@@ -25,6 +25,9 @@
 /* xen_ulong_t is always 64 bits */
 #define BITS_PER_XEN_ULONG 64
 
+/* And ELF files are also 64-bit. */
+#define ELFSIZE 64
+
 #define CONFIG_PAGING_ASSISTANCE 1
 
 #define CONFIG_PAGING_LEVELS 3
-- 
2.1.0

Re: [PATCH v3 2/5] arm/config: Declare ELFSIZE_[32|64] respectively.

[Stefano Stabellini]

On Fri, 12 Feb 2016, Konrad Rzeszutek Wilk wrote:
> On Fri, Feb 12, 2016 at 03:26:14PM +0000, Stefano Stabellini wrote:
> > On Fri, 12 Feb 2016, Jan Beulich wrote:
> > > >>> On 12.02.16 at 15:17, <konrad.wilk@oracle.com> wrote:
> > > > --- a/xen/include/asm-arm/config.h
> > > > +++ b/xen/include/asm-arm/config.h
> > > > @@ -15,8 +15,10 @@
> > > >  
> > > >  #if defined(CONFIG_ARM_64)
> > > >  # define LONG_BYTEORDER 3
> > > > +# define ELFSIZE 64
> > > >  #else
> > > >  # define LONG_BYTEORDER 2
> > > > +# define ELFSIZE 64
> > > >  #endif
> > > 
> > > Leaving the question - why twice instead of outside the #ifdef?
> > 
> > Right, please move it out of the #ifdef.
> 
> Done!
> 
> >From 32a062c119091f2f3f6a4c540a8098e97c273dd2 Mon Sep 17 00:00:00 2001
> From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Date: Fri, 5 Feb 2016 10:44:45 -0500
> Subject: [PATCH] arm/config: Declare ELFSIZE_64.
> 
> Otherwise any code that tries to use Elf_* macros would
> require us to use Elf64_* types instead of the more
> friendly Elf_ one.
> 
> This is OK to do since 32-bit ARM uses LPAE mode.
> 
> CC: ian.campbell@citrix.com
> CC: wei.liu2@citrix.com
> CC: stefano.stabellini@citrix.com
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>


>  xen/include/asm-arm/config.h | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/xen/include/asm-arm/config.h b/xen/include/asm-arm/config.h
> index bd832df..a1b968d 100644
> --- a/xen/include/asm-arm/config.h
> +++ b/xen/include/asm-arm/config.h
> @@ -25,6 +25,9 @@
>  /* xen_ulong_t is always 64 bits */
>  #define BITS_PER_XEN_ULONG 64
>  
> +/* And ELF files are also 64-bit. */
> +#define ELFSIZE 64
> +
>  #define CONFIG_PAGING_ASSISTANCE 1
>  
>  #define CONFIG_PAGING_LEVELS 3
> -- 
> 2.1.0
> 

Re: [PATCH v3 2/5] arm/config: Declare ELFSIZE_[32|64] respectively.

[Konrad Rzeszutek Wilk]

On Fri, Feb 12, 2016 at 03:57:25PM +0000, Stefano Stabellini wrote:
> On Fri, 12 Feb 2016, Konrad Rzeszutek Wilk wrote:
> > On Fri, Feb 12, 2016 at 03:26:14PM +0000, Stefano Stabellini wrote:
> > > On Fri, 12 Feb 2016, Jan Beulich wrote:
> > > > >>> On 12.02.16 at 15:17, <konrad.wilk@oracle.com> wrote:
> > > > > --- a/xen/include/asm-arm/config.h
> > > > > +++ b/xen/include/asm-arm/config.h
> > > > > @@ -15,8 +15,10 @@
> > > > >  
> > > > >  #if defined(CONFIG_ARM_64)
> > > > >  # define LONG_BYTEORDER 3
> > > > > +# define ELFSIZE 64
> > > > >  #else
> > > > >  # define LONG_BYTEORDER 2
> > > > > +# define ELFSIZE 64
> > > > >  #endif
> > > > 
> > > > Leaving the question - why twice instead of outside the #ifdef?
> > > 
> > > Right, please move it out of the #ifdef.
> > 
> > Done!
> > 
> > >From 32a062c119091f2f3f6a4c540a8098e97c273dd2 Mon Sep 17 00:00:00 2001
> > From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > Date: Fri, 5 Feb 2016 10:44:45 -0500
> > Subject: [PATCH] arm/config: Declare ELFSIZE_64.
> > 
> > Otherwise any code that tries to use Elf_* macros would
> > require us to use Elf64_* types instead of the more
> > friendly Elf_ one.
> > 
> > This is OK to do since 32-bit ARM uses LPAE mode.
> > 
> > CC: ian.campbell@citrix.com
> > CC: wei.liu2@citrix.com
> > CC: stefano.stabellini@citrix.com
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 
> Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

applied. Thanks!
> 
> 
> >  xen/include/asm-arm/config.h | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/xen/include/asm-arm/config.h b/xen/include/asm-arm/config.h
> > index bd832df..a1b968d 100644
> > --- a/xen/include/asm-arm/config.h
> > +++ b/xen/include/asm-arm/config.h
> > @@ -25,6 +25,9 @@
> >  /* xen_ulong_t is always 64 bits */
> >  #define BITS_PER_XEN_ULONG 64
> >  
> > +/* And ELF files are also 64-bit. */
> > +#define ELFSIZE 64
> > +
> >  #define CONFIG_PAGING_ASSISTANCE 1
> >  
> >  #define CONFIG_PAGING_LEVELS 3
> > -- 
> > 2.1.0
> > 

Re: [PATCH v3 5/5] mkelf32: Close those file descriptors in the error paths.

[Jan Beulich]

>>> On 12.02.16 at 04:08, <konrad.wilk@oracle.com> wrote:
> While we are operating here we may as well fix some of the
> file descriptor leaks.

I'm not convinced. The added goto-s make the code uglier to read,
and this being a standalone utility there's not really any leak here.

Jan

Re: [PATCH v3 5/5] mkelf32: Close those file descriptors in the error paths.

[Ian Jackson]

Jan Beulich writes ("Re: [PATCH v3 5/5] mkelf32: Close those file descriptors in the error paths."):
> On 12.02.16 at 04:08, <konrad.wilk@oracle.com> wrote:
> > While we are operating here we may as well fix some of the
> > file descriptor leaks.
> 
> I'm not convinced. The added goto-s make the code uglier to read,
> and this being a standalone utility there's not really any leak here.

I don't buy this `uglier to read'.  What `return 1' does is make me
think `is some resource being leaked' and `do I need to audit this
thing'.

Ian.

Re: [PATCH v3 5/5] mkelf32: Close those file descriptors in the error paths.

[Jan Beulich]

>>> On 18.02.16 at 17:39, <Ian.Jackson@eu.citrix.com> wrote:
> Jan Beulich writes ("Re: [PATCH v3 5/5] mkelf32: Close those file descriptors 
> in the error paths."):
>> On 12.02.16 at 04:08, <konrad.wilk@oracle.com> wrote:
>> > While we are operating here we may as well fix some of the
>> > file descriptor leaks.
>> 
>> I'm not convinced. The added goto-s make the code uglier to read,
>> and this being a standalone utility there's not really any leak here.
> 
> I don't buy this `uglier to read'.  What `return 1' does is make me
> think `is some resource being leaked' and `do I need to audit this
> thing'.

Certainly a matter of taste to some degree - goto-s are always
ugly to read to my eyes. Irrespective of this I don't buy the leak
aspect for a non-library like, short running build utility. The close()
calls are just more code, with absolutely no added benefit to the
system the code runs on.

Jan

Re: [PATCH v3 5/5] mkelf32: Close those file descriptors in the error paths.

[Konrad Rzeszutek Wilk]

On Thu, Feb 18, 2016 at 09:45:30AM -0700, Jan Beulich wrote:
> >>> On 18.02.16 at 17:39, <Ian.Jackson@eu.citrix.com> wrote:
> > Jan Beulich writes ("Re: [PATCH v3 5/5] mkelf32: Close those file descriptors 
> > in the error paths."):
> >> On 12.02.16 at 04:08, <konrad.wilk@oracle.com> wrote:
> >> > While we are operating here we may as well fix some of the
> >> > file descriptor leaks.
> >> 
> >> I'm not convinced. The added goto-s make the code uglier to read,
> >> and this being a standalone utility there's not really any leak here.
> > 
> > I don't buy this `uglier to read'.  What `return 1' does is make me
> > think `is some resource being leaked' and `do I need to audit this
> > thing'.
> 
> Certainly a matter of taste to some degree - goto-s are always
> ugly to read to my eyes. Irrespective of this I don't buy the leak
> aspect for a non-library like, short running build utility. The close()
> calls are just more code, with absolutely no added benefit to the
> system the code runs on.

<chuckles>

If I turned them in:

	if (..blah..)
	{
		close(infd);
		return -1;
	}

would that satisfy you?

(Irrespective of the 'no added benefit to the system the code runs
on.').

> 
> Jan
> 

Re: [PATCH v3 5/5] mkelf32: Close those file descriptors in the error paths.

[Jan Beulich]

>>> On 18.02.16 at 22:12, <konrad.wilk@oracle.com> wrote:
> On Thu, Feb 18, 2016 at 09:45:30AM -0700, Jan Beulich wrote:
>> >>> On 18.02.16 at 17:39, <Ian.Jackson@eu.citrix.com> wrote:
>> > Jan Beulich writes ("Re: [PATCH v3 5/5] mkelf32: Close those file 
> descriptors 
>> > in the error paths."):
>> >> On 12.02.16 at 04:08, <konrad.wilk@oracle.com> wrote:
>> >> > While we are operating here we may as well fix some of the
>> >> > file descriptor leaks.
>> >> 
>> >> I'm not convinced. The added goto-s make the code uglier to read,
>> >> and this being a standalone utility there's not really any leak here.
>> > 
>> > I don't buy this `uglier to read'.  What `return 1' does is make me
>> > think `is some resource being leaked' and `do I need to audit this
>> > thing'.
>> 
>> Certainly a matter of taste to some degree - goto-s are always
>> ugly to read to my eyes. Irrespective of this I don't buy the leak
>> aspect for a non-library like, short running build utility. The close()
>> calls are just more code, with absolutely no added benefit to the
>> system the code runs on.
> 
> <chuckles>
> 
> If I turned them in:
> 
> 	if (..blah..)
> 	{
> 		close(infd);
> 		return -1;
> 	}
> 
> would that satisfy you?

Since presumably this would be on a number of error paths, I'm
afraid ...

> (Irrespective of the 'no added benefit to the system the code runs
> on.').

... this aspect would still be an relevant one.

Jan

Re: [PATCH v3 5/5] mkelf32: Close those file descriptors in the error paths.

[Ian Jackson]

Konrad Rzeszutek Wilk writes ("Re: [PATCH v3 5/5] mkelf32: Close those file descriptors in the error paths."):
> <chuckles>
> 
> If I turned them in:
> 
> 	if (..blah..)
> 	{
> 		close(infd);
> 		return -1;
> 	}
> 
> would that satisfy you?

I would strongly resist that.  That coding style is an error handling
antipattern.

Ian.

Re: [PATCH v3 2/5] arm/config: Declare ELFSIZE_[32|64] respectively.

[Julien Grall]

Hi Konrad,

Sorry for the late answer on this patch. I noticed the problem while I 
was reviewing your xSplice patch series.

On 12/02/2016 15:56, Konrad Rzeszutek Wilk wrote:
>  From 32a062c119091f2f3f6a4c540a8098e97c273dd2 Mon Sep 17 00:00:00 2001
> From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Date: Fri, 5 Feb 2016 10:44:45 -0500
> Subject: [PATCH] arm/config: Declare ELFSIZE_64.
>
> Otherwise any code that tries to use Elf_* macros would
> require us to use Elf64_* types instead of the more
> friendly Elf_ one.
>
> This is OK to do since 32-bit ARM uses LPAE mode.

That's not true. Some of structures have a different layout based on the 
file class (i.e ELFSIZE in Xen).

For 32-bit ARM, ELFCLASS32 (i.e 32-bit data types) will always be used. 
So we need to set ELFSIZE to 32.

Otherwise Xen won't be able to interpret correctly the ELF note holding 
the build-id (see [1]).

Regards,

[1] http://lists.xen.org/archives/html/xen-devel/2015-11/msg00632.html

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v3 2/5] arm/config: Declare ELFSIZE_[32|64] respectively.

[Konrad Rzeszutek Wilk]

On Wed, Mar 16, 2016 at 05:32:09PM +0000, Julien Grall wrote:
> Hi Konrad,
> 
> Sorry for the late answer on this patch. I noticed the problem while I was
> reviewing your xSplice patch series.
> 
> On 12/02/2016 15:56, Konrad Rzeszutek Wilk wrote:
> > From 32a062c119091f2f3f6a4c540a8098e97c273dd2 Mon Sep 17 00:00:00 2001
> >From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> >Date: Fri, 5 Feb 2016 10:44:45 -0500
> >Subject: [PATCH] arm/config: Declare ELFSIZE_64.
> >
> >Otherwise any code that tries to use Elf_* macros would
> >require us to use Elf64_* types instead of the more
> >friendly Elf_ one.
> >
> >This is OK to do since 32-bit ARM uses LPAE mode.
> 
> That's not true. Some of structures have a different layout based on the
> file class (i.e ELFSIZE in Xen).
> 
> For 32-bit ARM, ELFCLASS32 (i.e 32-bit data types) will always be used. So
> we need to set ELFSIZE to 32.

OK. Let me send out a patch to fix that. Thanks for the heads up!
> 
> Otherwise Xen won't be able to interpret correctly the ELF note holding the
> build-id (see [1]).
> 
> Regards,
> 
> [1] http://lists.xen.org/archives/html/xen-devel/2015-11/msg00632.html
> 
> -- 
> Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v3 2/5] arm/config: Declare ELFSIZE_[32|64] respectively.

[Julien Grall]

Hi Konrad

On 16/03/2016 17:52, Konrad Rzeszutek Wilk wrote:
> On Wed, Mar 16, 2016 at 05:32:09PM +0000, Julien Grall wrote:
>> Sorry for the late answer on this patch. I noticed the problem while I was
>> reviewing your xSplice patch series.
>>
>> On 12/02/2016 15:56, Konrad Rzeszutek Wilk wrote:
>>>  From 32a062c119091f2f3f6a4c540a8098e97c273dd2 Mon Sep 17 00:00:00 2001
>>> From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>>> Date: Fri, 5 Feb 2016 10:44:45 -0500
>>> Subject: [PATCH] arm/config: Declare ELFSIZE_64.
>>>
>>> Otherwise any code that tries to use Elf_* macros would
>>> require us to use Elf64_* types instead of the more
>>> friendly Elf_ one.
>>>
>>> This is OK to do since 32-bit ARM uses LPAE mode.
>>
>> That's not true. Some of structures have a different layout based on the
>> file class (i.e ELFSIZE in Xen).
>>
>> For 32-bit ARM, ELFCLASS32 (i.e 32-bit data types) will always be used. So
>> we need to set ELFSIZE to 32.
>
> OK. Let me send out a patch to fix that. Thanks for the heads up!

With this change, do you need to revise the answer to the question Jan 
raised in the design document about ARM32 and the ELF payload declaring 
the ELF only 64-bit syntax?

Regards,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v3 2/5] arm/config: Declare ELFSIZE_[32|64] respectively.

[Konrad Rzeszutek Wilk]

On Wed, Mar 16, 2016 at 06:08:06PM +0000, Julien Grall wrote:
> Hi Konrad
> 
> On 16/03/2016 17:52, Konrad Rzeszutek Wilk wrote:
> >On Wed, Mar 16, 2016 at 05:32:09PM +0000, Julien Grall wrote:
> >>Sorry for the late answer on this patch. I noticed the problem while I was
> >>reviewing your xSplice patch series.
> >>
> >>On 12/02/2016 15:56, Konrad Rzeszutek Wilk wrote:
> >>> From 32a062c119091f2f3f6a4c540a8098e97c273dd2 Mon Sep 17 00:00:00 2001
> >>>From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> >>>Date: Fri, 5 Feb 2016 10:44:45 -0500
> >>>Subject: [PATCH] arm/config: Declare ELFSIZE_64.
> >>>
> >>>Otherwise any code that tries to use Elf_* macros would
> >>>require us to use Elf64_* types instead of the more
> >>>friendly Elf_ one.
> >>>
> >>>This is OK to do since 32-bit ARM uses LPAE mode.
> >>
> >>That's not true. Some of structures have a different layout based on the
> >>file class (i.e ELFSIZE in Xen).
> >>
> >>For 32-bit ARM, ELFCLASS32 (i.e 32-bit data types) will always be used. So
> >>we need to set ELFSIZE to 32.
> >
> >OK. Let me send out a patch to fix that. Thanks for the heads up!
> 
> With this change, do you need to revise the answer to the question Jan
> raised in the design document about ARM32 and the ELF payload declaring the
> ELF only 64-bit syntax?

Yes I will. Thank you for raising that as I completly forgot it!

I think we can still keep the same structure and size. But instead of
using ELF types in the design (and in the code) I will swap them to
proper uintXX_t types.

> 
> Regards,
> 
> -- 
> Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v3 2/5] arm/config: Declare ELFSIZE_[32|64] respectively.

[Konrad Rzeszutek Wilk]

On Wed, Mar 16, 2016 at 03:21:51PM -0400, Konrad Rzeszutek Wilk wrote:
> On Wed, Mar 16, 2016 at 06:08:06PM +0000, Julien Grall wrote:
> > Hi Konrad
> > 
> > On 16/03/2016 17:52, Konrad Rzeszutek Wilk wrote:
> > >On Wed, Mar 16, 2016 at 05:32:09PM +0000, Julien Grall wrote:
> > >>Sorry for the late answer on this patch. I noticed the problem while I was
> > >>reviewing your xSplice patch series.
> > >>
> > >>On 12/02/2016 15:56, Konrad Rzeszutek Wilk wrote:
> > >>> From 32a062c119091f2f3f6a4c540a8098e97c273dd2 Mon Sep 17 00:00:00 2001
> > >>>From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > >>>Date: Fri, 5 Feb 2016 10:44:45 -0500
> > >>>Subject: [PATCH] arm/config: Declare ELFSIZE_64.
> > >>>
> > >>>Otherwise any code that tries to use Elf_* macros would
> > >>>require us to use Elf64_* types instead of the more
> > >>>friendly Elf_ one.
> > >>>
> > >>>This is OK to do since 32-bit ARM uses LPAE mode.
> > >>
> > >>That's not true. Some of structures have a different layout based on the
> > >>file class (i.e ELFSIZE in Xen).
> > >>
> > >>For 32-bit ARM, ELFCLASS32 (i.e 32-bit data types) will always be used. So
> > >>we need to set ELFSIZE to 32.
> > >
> > >OK. Let me send out a patch to fix that. Thanks for the heads up!
> > 
> > With this change, do you need to revise the answer to the question Jan
> > raised in the design document about ARM32 and the ELF payload declaring the
> > ELF only 64-bit syntax?
> 
> Yes I will. Thank you for raising that as I completly forgot it!
> 
> I think we can still keep the same structure and size. But instead of
> using ELF types in the design (and in the code) I will swap them to
> proper uintXX_t types.

And here is the patch. The change for uintXX_t type worked out - same
size and offset as on 64-bit. Thought I am tempted to add some
more BUILD_BUG checks.

From 7007f1a3fa3a77a725f529420c7aea0e8ebdc9fa Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Date: Wed, 16 Mar 2016 16:08:25 -0400
Subject: [PATCH v4 01/35] arm/config: Declare ELFSIZE_[32|64]

The commit bcfaea685d38c08e5eb90797512ab80f0bc69d0c
"arm/config: Declare ELFSIZE_64" was not correct.

For 32-bit ARM, ELFCLASS32 (i.e. 32-bit data types) will always
be used so we need to set ELFSIZE to 32.

Reported-by: Julien Grall <julien.grall@arm.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

---
Cc: Stefano Stabellini <stefano.stabellini@citrix.com>
Cc: Julien Grall <julien.grall@arm.com>
--
---
 xen/include/asm-arm/config.h | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/xen/include/asm-arm/config.h b/xen/include/asm-arm/config.h
index 7ceb5c5..2d11b62 100644
--- a/xen/include/asm-arm/config.h
+++ b/xen/include/asm-arm/config.h
@@ -9,8 +9,10 @@
 
 #if defined(CONFIG_ARM_64)
 # define LONG_BYTEORDER 3
+# define ELFSIZE 64
 #else
 # define LONG_BYTEORDER 2
+# define ELFSIZE 32
 #endif
 
 #define BYTES_PER_LONG (1 << LONG_BYTEORDER)
@@ -20,9 +22,6 @@
 /* xen_ulong_t is always 64 bits */
 #define BITS_PER_XEN_ULONG 64
 
-/* And ELF files are also 64-bit. */
-#define ELFSIZE 64
-
 #define CONFIG_PAGING_ASSISTANCE 1
 
 #define CONFIG_PAGING_LEVELS 3
-- 
2.5.0

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v3 2/5] arm/config: Declare ELFSIZE_[32|64] respectively.

[Julien Grall]

Hi Konrad,

On 17/03/16 01:16, Konrad Rzeszutek Wilk wrote:
> And here is the patch. The change for uintXX_t type worked out - same
> size and offset as on 64-bit. Thought I am tempted to add some
> more BUILD_BUG checks.
>
>  From 7007f1a3fa3a77a725f529420c7aea0e8ebdc9fa Mon Sep 17 00:00:00 2001
> From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Date: Wed, 16 Mar 2016 16:08:25 -0400
> Subject: [PATCH v4 01/35] arm/config: Declare ELFSIZE_[32|64]
>
> The commit bcfaea685d38c08e5eb90797512ab80f0bc69d0c
> "arm/config: Declare ELFSIZE_64" was not correct.
>
> For 32-bit ARM, ELFCLASS32 (i.e. 32-bit data types) will always
> be used so we need to set ELFSIZE to 32.
>
> Reported-by: Julien Grall <julien.grall@arm.com>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Acked-by: Julien Grall <julien.grall@arm.com>

Regards,

>
> ---
> Cc: Stefano Stabellini <stefano.stabellini@citrix.com>
> Cc: Julien Grall <julien.grall@arm.com>
> --
> ---
>   xen/include/asm-arm/config.h | 5 ++---
>   1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/xen/include/asm-arm/config.h b/xen/include/asm-arm/config.h
> index 7ceb5c5..2d11b62 100644
> --- a/xen/include/asm-arm/config.h
> +++ b/xen/include/asm-arm/config.h
> @@ -9,8 +9,10 @@
>
>   #if defined(CONFIG_ARM_64)
>   # define LONG_BYTEORDER 3
> +# define ELFSIZE 64
>   #else
>   # define LONG_BYTEORDER 2
> +# define ELFSIZE 32
>   #endif
>
>   #define BYTES_PER_LONG (1 << LONG_BYTEORDER)
> @@ -20,9 +22,6 @@
>   /* xen_ulong_t is always 64 bits */
>   #define BITS_PER_XEN_ULONG 64
>
> -/* And ELF files are also 64-bit. */
> -#define ELFSIZE 64
> -
>   #define CONFIG_PAGING_ASSISTANCE 1
>
>   #define CONFIG_PAGING_LEVELS 3
>

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v3 2/5] arm/config: Declare ELFSIZE_[32|64] respectively.

[Konrad Rzeszutek Wilk]

On Thu, Mar 17, 2016 at 11:04:29AM +0000, Julien Grall wrote:
> Hi Konrad,
> 
> On 17/03/16 01:16, Konrad Rzeszutek Wilk wrote:
> >And here is the patch. The change for uintXX_t type worked out - same
> >size and offset as on 64-bit. Thought I am tempted to add some
> >more BUILD_BUG checks.
> >
> > From 7007f1a3fa3a77a725f529420c7aea0e8ebdc9fa Mon Sep 17 00:00:00 2001
> >From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> >Date: Wed, 16 Mar 2016 16:08:25 -0400
> >Subject: [PATCH v4 01/35] arm/config: Declare ELFSIZE_[32|64]
> >
> >The commit bcfaea685d38c08e5eb90797512ab80f0bc69d0c
> >"arm/config: Declare ELFSIZE_64" was not correct.
> >
> >For 32-bit ARM, ELFCLASS32 (i.e. 32-bit data types) will always
> >be used so we need to set ELFSIZE to 32.
> >
> >Reported-by: Julien Grall <julien.grall@arm.com>
> >Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 
> Acked-by: Julien Grall <julien.grall@arm.com>

Thank you. Patch applied.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel