[POWERPC/IA64] Updates required due to loader changes

[POWERPC/IA64] Updates required due to loader changes

[Ian Campbell]

[-- Attachment #1: Type: text/plain, Size: 1134 bytes --]

Hi guys,

I have made a change to xen-unstable which introduce the use of ELF
notes for passing Xen specific information from the kernel to the domain
builder, this is a replacement for the __xen_guest section string which
stood no chance of being accepted by Linux upstream. I think it's a much
nicer interface in its own right as well.

The domU builder retains support for the __xen_guest stuff for backward
compatibility however the dom0 builder now only supports the ELF notes.
I don't think either of you were using actually __xen_guest stuff anyway
-- IA64 seems to fake out an empty one while powerpc checks for the
presence but never looks inside.

I think the attached patches are all that is necessary to make the new
stuff work for you but I am unable to test them[0]. If you could confirm
for me that they do the job I'll commit them.

The changesets numbers are 11235-11239. They are held in the staging
tree at the moment but should propagate to the public tree once they
have passed regression testing.

Thanks,
Ian.

[0] ia64 doesn't build for me and cross-tool can't even produce a
powerpc compiler on my system :-(

[-- Attachment #2: elf-notes.ia64.diff --]
[-- Type: text/x-patch, Size: 797 bytes --]

diff -r 58b5141c8309 xen/arch/ia64/xen/domain.c
--- a/xen/arch/ia64/xen/domain.c	Wed Aug 23 14:43:48 2006 +0100
+++ b/xen/arch/ia64/xen/domain.c	Tue Aug 22 18:51:41 2006 +0100
@@ -901,17 +901,14 @@ int construct_dom0(struct domain *d,
 	    return rc;
 
 #ifdef VALIDATE_VT
-	/* Temp workaround */
-	if (running_on_sim)
-	    dsi.xen_section_string = (char *)1;
-
+	/* Checking running_on_sim is a temporary workaround */
 	/* Check whether dom0 is vti domain */
-	if ((!vmx_enabled) && !dsi.xen_section_string) {
+	if (!vmx_enabled && !running_on_sim) {
 	    printk("Lack of hardware support for unmodified vmx dom0\n");
 	    panic("");
 	}
 
-	if (vmx_enabled && !dsi.xen_section_string) {
+	if (vmx_enabled && !running_on_sim) {
 	    printk("Dom0 is vmx domain!\n");
 	    vmx_dom0 = 1;
 	}

[-- Attachment #3: elf-notes.powerpc.diff --]
[-- Type: text/x-patch, Size: 611 bytes --]

diff -r 58b5141c8309 xen/arch/powerpc/domain_build.c
--- a/xen/arch/powerpc/domain_build.c	Wed Aug 23 14:43:48 2006 +0100
+++ b/xen/arch/powerpc/domain_build.c	Tue Aug 22 12:29:12 2006 +0100
@@ -137,10 +137,6 @@ int construct_dom0(struct domain *d,
     dsi.v_kernend = RM_MASK(dsi.v_kernend, 42);
     dsi.v_kernentry = RM_MASK(dsi.v_kernentry, 42);
 
-    if (dsi.xen_section_string == NULL) {
-        printk("Not a Xen-ELF image: '__xen_guest' section not found.\n");
-        return -EINVAL;
-    }
     printk("*** LOADING DOMAIN 0 ***\n");
 
     /* By default DOM0 is allocated all available memory. */

[-- Attachment #4: Type: text/plain, Size: 138 bytes --]

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

Re: [POWERPC/IA64] Updates required due to loader changes

[Ian Campbell]

On Wed, 2006-08-23 at 15:00 +0100, Ian Campbell wrote:
> Hi guys,
> 
> I have made a change to xen-unstable which introduce the use of ELF
> notes for passing Xen specific information from the kernel to the domain
> builder, this is a replacement for the __xen_guest section string which
> stood no chance of being accepted by Linux upstream. I think it's a much
> nicer interface in its own right as well.
> 
> The domU builder retains support for the __xen_guest stuff for backward
> compatibility however the dom0 builder now only supports the ELF notes.

Change of plan ;-)

Since this is one of the few things which would prevent you using a
3.0.2 dom0 on a 3.0.3 hypervisor I'm going to add the backwards compat
stuff to the dom0 builder.

Changes will probably needed to ia64 and powerpc but I'll get back to
you on that...

Ian.

Re: [POWERPC/IA64] Updates required due to loader changes

[Tristan Gingold]

Le Mercredi 23 Août 2006 16:36, Ian Campbell a écrit :
> On Wed, 2006-08-23 at 15:00 +0100, Ian Campbell wrote:
> > Hi guys,
> >
> > I have made a change to xen-unstable which introduce the use of ELF
> > notes for passing Xen specific information from the kernel to the domain
> > builder, this is a replacement for the __xen_guest section string which
> > stood no chance of being accepted by Linux upstream. I think it's a much
> > nicer interface in its own right as well.
> >
> > The domU builder retains support for the __xen_guest stuff for backward
> > compatibility however the dom0 builder now only supports the ELF notes.
>
> Change of plan ;-)
>
> Since this is one of the few things which would prevent you using a
> 3.0.2 dom0 on a 3.0.3 hypervisor I'm going to add the backwards compat
> stuff to the dom0 builder.
>
> Changes will probably needed to ia64 and powerpc but I'll get back to
> you on that...
Hi,

on ia64 we never used __xen_guest.  The code you proposed to change is inside
#ifdef VALIDATE_VT which is never enabled.

Tristan.

Re: [POWERPC/IA64] Updates required due to loader changes

[Ian Campbell]

On Wed, 2006-08-23 at 15:36 +0100, Ian Campbell wrote:
> Since this is one of the few things which would prevent you using a
> 3.0.2 dom0 on a 3.0.3 hypervisor I'm going to add the backwards compat
> stuff to the dom0 builder.
> 
> Changes will probably needed to ia64 and powerpc but I'll get back to
> you on that...

I've now done this, it should come through as changesets 11244-11246.

The accessor functions which abstract the elf notes vs. __xen_guest
stuff should now be OK with both being absent on !x86 so I think the
patches I sent earlier are still correct, but please confirm.

Does it make sense to add a token set of ELF notes to your kernels? I
think XEN_VERSION, GUEST_OS and GUEST_VERSION would do the job...

Many thanks,
Ian.

Re: [POWERPC/IA64] Updates required due to loader changes

[Hollis Blanchard]

On Wed, 2006-08-23 at 18:44 +0100, Ian Campbell wrote:
> On Wed, 2006-08-23 at 15:36 +0100, Ian Campbell wrote:
> > Since this is one of the few things which would prevent you using a
> > 3.0.2 dom0 on a 3.0.3 hypervisor I'm going to add the backwards compat
> > stuff to the dom0 builder.
> > 
> > Changes will probably needed to ia64 and powerpc but I'll get back to
> > you on that...
> 
> I've now done this, it should come through as changesets 11244-11246.

You may not realize this, but those numbers you're using are specific to
a particular tree. Instead, please use the changeset hash, because that
refers to the same changeset no matter what tree it's in.

At any rate, it seems these patches have not been moved out of the
private staging tree, so I really can't comment on them.

> The accessor functions which abstract the elf notes vs. __xen_guest
> stuff should now be OK with both being absent on !x86 so I think the
> patches I sent earlier are still correct, but please confirm.
> 
> Does it make sense to add a token set of ELF notes to your kernels? I
> think XEN_VERSION, GUEST_OS and GUEST_VERSION would do the job...

I don't have any objection in principle.

-- 
Hollis Blanchard
IBM Linux Technology Center

Re: [POWERPC/IA64] Updates required due to loader changes

[Ian Campbell]

On Wed, 2006-08-23 at 13:09 -0500, Hollis Blanchard wrote:
> On Wed, 2006-08-23 at 18:44 +0100, Ian Campbell wrote:
> > On Wed, 2006-08-23 at 15:36 +0100, Ian Campbell wrote:
> > > Since this is one of the few things which would prevent you using a
> > > 3.0.2 dom0 on a 3.0.3 hypervisor I'm going to add the backwards compat
> > > stuff to the dom0 builder.
> > > 
> > > Changes will probably needed to ia64 and powerpc but I'll get back to
> > > you on that...
> > 
> > I've now done this, it should come through as changesets 11244-11246.
> 
> You may not realize this, but those numbers you're using are specific to
> a particular tree. Instead, please use the changeset hash, because that
> refers to the same changeset no matter what tree it's in.

Sorry, I did know that I just forgot... At any rate the original set I
was talking about were:
      * 11235:62b7b5f3029f
      * 11236:7ca72a1c4182
      * 11237:d57b174adfd6
      * 11238:faadbf5ba8d6
      * 11239:58b5141c8309

And the second set (re-adding __xen_guest to dom0) were:
      * 11244:cc006f78cbe2
      * 11245:2eb8efcc70d1
      * 11246:551495fa7b3e

(I left the cset number in too so you can correlate with my earlier
mail).

> At any rate, it seems these patches have not been moved out of the
> private staging tree, so I really can't comment on them.

Sure, this was more of a heads up that they were coming ;-)

Ian.

Re: [POWERPC/IA64] Updates required due to loader changes

[Ian Campbell]

On Wed, 2006-08-23 at 16:49 +0200, Tristan Gingold wrote:
> on ia64 we never used __xen_guest.  The code you proposed to change is inside
> #ifdef VALIDATE_VT which is never enabled.

OK thanks.

There was something like:
        #ifdef __ia64__
        	dsi->xe_guest_string = "";
        #endif
in xc_elf_load.c too but that is now gone too...

Ian.

Re: [POWERPC/IA64] Updates required due to loader changes

[Hollis Blanchard]

On Wed, 2006-08-23 at 15:00 +0100, Ian Campbell wrote:
> 
> The changesets numbers are 11235-11239. They are held in the staging
> tree at the moment but should propagate to the public tree once they
> have passed regression testing. 

In the future it would be really nice to get our comments *before* you
check in the code.

> diff -r 58b5141c8309 xen/arch/powerpc/domain_build.c
> --- a/xen/arch/powerpc/domain_build.c   Wed Aug 23 14:43:48 2006 +0100
> +++ b/xen/arch/powerpc/domain_build.c   Tue Aug 22 12:29:12 2006 +0100
> @@ -137,10 +137,6 @@ int construct_dom0(struct domain *d,
>      dsi.v_kernend = RM_MASK(dsi.v_kernend, 42);
>      dsi.v_kernentry = RM_MASK(dsi.v_kernentry, 42);
>  
> -    if (dsi.xen_section_string == NULL) {
> -        printk("Not a Xen-ELF image: '__xen_guest' section not found.\n");
> -        return -EINVAL;
> -    }
>      printk("*** LOADING DOMAIN 0 ***\n");
>  
>      /* By default DOM0 is allocated all available memory. */

Yeah, PowerPC really isn't doing anything with this right now.

-- 
Hollis Blanchard
IBM Linux Technology Center

Re: [POWERPC/IA64] Updates required due to loader changes

[Horms]

On Wed, 23 Aug 2006 15:00:28 +0100, Ian Campbell wrote:
> [-- text/plain, encoding 7bit, charset: ISO-8859-1, 28 lines --]
> 
> Hi guys,
> 
> I have made a change to xen-unstable which introduce the use of ELF
> notes for passing Xen specific information from the kernel to the domain
> builder, this is a replacement for the __xen_guest section string which
> stood no chance of being accepted by Linux upstream. I think it's a much
> nicer interface in its own right as well.
> 
> The domU builder retains support for the __xen_guest stuff for backward
> compatibility however the dom0 builder now only supports the ELF notes.
> I don't think either of you were using actually __xen_guest stuff anyway
> -- IA64 seems to fake out an empty one while powerpc checks for the
> presence but never looks inside.
> 
> I think the attached patches are all that is necessary to make the new
> stuff work for you but I am unable to test them[0]. If you could confirm
> for me that they do the job I'll commit them.
> 
> The changesets numbers are 11235-11239. They are held in the staging
> tree at the moment but should propagate to the public tree once they
> have passed regression testing.

Sorry for being so naieve, but where is the staging tree?

I included elf notes infastructure in the kexec patches that I have
posted to this list serveral times. It sounds like there is probably
some infastructure overlap. I'd like to take a look at what you have
done to see if it fits the needs of kexec so we can avoid code duplication.

Hopefully the size of the kexec patchset can be reduced.
And someone might even review or test it :-)

-- 
Horms
  H: http://www.vergenet.net/~horms/
  W: http://www.valinux.co.jp/en/

Re: [POWERPC/IA64] Updates required due to loader changes

[Ian Campbell]

On Thu, 2006-08-24 at 11:17 +0900, Horms wrote:
> Sorry for being so naieve, but where is the staging tree?

It's an internal tree which gets regression tested and then copied
automatically to the public xen-unstable tree.

The code in question is in the xen-unstable tree now.

> I included elf notes infastructure in the kexec patches that I have
> posted to this list serveral times. It sounds like there is probably
> some infastructure overlap. I'd like to take a look at what you have
> done to see if it fits the needs of kexec so we can avoid code duplication.

I didn't know you guys had ELF infrastructure in the kexec patches, I
hope my stuff is useful for you...

Ian.

Re: [POWERPC/IA64] Updates required due to loader changes

[Horms]

On Thu, Aug 24, 2006 at 08:12:02AM +0100, Ian Campbell wrote:
> On Thu, 2006-08-24 at 11:17 +0900, Horms wrote:
> > Sorry for being so naieve, but where is the staging tree?
> 
> It's an internal tree which gets regression tested and then copied
> automatically to the public xen-unstable tree.
> 
> The code in question is in the xen-unstable tree now.
> 
> > I included elf notes infastructure in the kexec patches that I have
> > posted to this list serveral times. It sounds like there is probably
> > some infastructure overlap. I'd like to take a look at what you have
> > done to see if it fits the needs of kexec so we can avoid code duplication.
> 
> I didn't know you guys had ELF infrastructure in the kexec patches, I
> hope my stuff is useful for you...

Hi Ian,

I took a look over the changes, and unforunately there doesn't seem
to be much overlap. This is for two main reasons:

  1. kexec is mainly concerned with crash notes, rather than
     generic notes (I was a bit confused when I read your original post).

  2. kexec creates these notes from within the hypervisor,
     but I beleive that your code has linux create the notes,
     while the hypervisor just reads them.

I'd really appreciate it if you could take a moment to cast your eyes
over my elf code. 

Below is the x86 code to fill in the crash notes, and below that
is include/xen/elfcore.h. This comprises the bulk of the elf code
that I have. I can come up with a full patch if you are interested.

The x86 fill-in code was basically copied from i386 linux kexec,
if there is room for improvement there, then its probably something
that also wants fixing in Linux.

The include/xen/elfcore.h. portion is based on linux, translated
to the xen style of elf code - which apparently does not have
the same origin as the linux code. I'd appreciate feedback on
the style here.

-- 
Horms
  H: http://www.vergenet.net/~horms/
  W: http://www.valinux.co.jp/en/

=== begin x86 fill-in snippet ===
static u32 *append_elf_note(u32 *buf, char *name, unsigned type, void *data,
							       size_t data_len)
{
	Elf_Note note;

	note.namesz = strlen(name) + 1;
	note.descsz = data_len;
	note.type   = type;
	memcpy(buf, &note, sizeof(note));
	buf += (sizeof(note) +3)/4;
	memcpy(buf, name, note.namesz);
	buf += (note.namesz + 3)/4;
	memcpy(buf, data, note.descsz);
	buf += (note.descsz + 3)/4;

	return buf;
}

static void final_note(u32 *buf)
{
	Elf_Note note;

	note.namesz = 0;
	note.descsz = 0;
	note.type   = 0;
	memcpy(buf, &note, sizeof(note));
}
=== end x86 fill-in snippet ===

=== begin include/xen/elfcore.h ===
/******************************************************************************
 * include/xen/elfcore.h
 * 
 * Created By: Horms
 *
 * Based heavily on include/linux/elfcore.h from Linux 2.6.16
 * Naming scheeme based on include/xen/elf.h (not include/linux/elfcore.h)
 *
 */

#ifndef __ELFCOREC_H__
#define __ELFCOREC_H__

#include <xen/types.h>
#include <xen/elf.h>
#include <public/xen.h>

#define NT_PRSTATUS     1
#define NT_XEN_DOM0_CR3 0x10000001 /* XXX: Hopefully this is unused,
					   feel free to change to a 
					   better/different value */

typedef struct
{
    int signo;                       /* signal number */
    int code;                        /* extra code */
    int errno;                       /* errno */
} ELF_Signifo;

/* These seem to be the same length on all architectures on Linux */
typedef int ELF_Pid;
typedef struct {
	long tv_sec;
	long tv_usec;
} ELF_Timeval;
typedef unsigned long ELF_Greg;
#define ELF_NGREG (sizeof (struct cpu_user_regs) / sizeof(ELF_Greg))
typedef ELF_Greg ELF_Gregset[ELF_NGREG];

/*
 * Definitions to generate Intel SVR4-like core files.
 * These mostly have the same names as the SVR4 types with "elf_"
 * tacked on the front to prevent clashes with linux definitions,
 * and the typedef forms have been avoided.  This is mostly like
 * the SVR4 structure, but more Linuxy, with things that Linux does
 * not support and which gdb doesn't really use excluded.
 */
typedef struct
{
    ELF_Signifo pr_info;         /* Info associated with signal */
    short pr_cursig;             /* Current signal */
    unsigned long pr_sigpend;    /* Set of pending signals */
    unsigned long pr_sighold;    /* Set of held signals */
    ELF_Pid pr_pid;
    ELF_Pid pr_ppid;
    ELF_Pid pr_pgrp;
    ELF_Pid pr_sid;
    ELF_Timeval pr_utime;        /* User time */
    ELF_Timeval pr_stime;        /* System time */
    ELF_Timeval pr_cutime;       /* Cumulative user time */
    ELF_Timeval pr_cstime;       /* Cumulative system time */
    ELF_Gregset pr_reg;          /* GP registers */
    int pr_fpvalid;              /* True if math co-processor being used.  */
} ELF_Prstatus;

#endif /* __ELFCOREC_H__ */

/*
 * Local variables:
 * mode: C
 * c-set-style: "BSD"
 * c-basic-offset: 4
 * tab-width: 4
 * indent-tabs-mode: nil
 * End:
 */
=== end include/xen/elfcore.h ===

Re: [POWERPC/IA64] Updates required due to loader changes

[Ian Campbell]

On Thu, 2006-08-24 at 17:11 +0900, Horms wrote:
> 
>   2. kexec creates these notes from within the hypervisor,
>      but I beleive that your code has linux create the notes,
>      while the hypervisor just reads them. 

That's right, the notes I'm using are included statically in the kernel
at compile time and are used by the domain builder to create the correct
environment for that kernel. Previously the same data was included as a
string in a special __xen_guest section and the notes are a pretty
direct translation of that.

> I'd really appreciate it if you could take a moment to cast your eyes
> over my elf code. 

I'm not realy an expert on ELF files but FWIW your append_elf_note()
looks like it creates the right note layout to me.

I'm not sure what final_note() is for since I don't think notes are
"NULL-terminated" but rather are constrained by the size of the ELF
section or segment that they are in. Also it could just be a memset ;-)

Ian.

Re: [POWERPC/IA64] Updates required due to loader changes

[Alex Williamson]

On Wed, 2006-08-23 at 13:09 -0500, Hollis Blanchard wrote:
> On Wed, 2006-08-23 at 18:44 +0100, Ian Campbell wrote:
> > Does it make sense to add a token set of ELF notes to your kernels? I
> > think XEN_VERSION, GUEST_OS and GUEST_VERSION would do the job...
> 
> I don't have any objection in principle.

   I don't have any objection to it either.  As Tristan mentioned for
ia64, we don't actually build the code in the proposed ia64 patch, so I
think we'll be fine for the moment.  One problem though is that
readnote.c will fail to build on all non-x86 architectures.  The patch
below makes it build, but I haven't actually tested the functionality.
PPC may want something similar.  Ian, could you commit this?  Thanks,

	Alex

Signed-off-by: Alex Williamson <alex.williamson@hp.com>
---

diff -r d5eb5205ff35 tools/xcutils/readnotes.c
--- a/tools/xcutils/readnotes.c	Thu Aug 24 16:25:49 2006 +0100
+++ b/tools/xcutils/readnotes.c	Thu Aug 24 14:12:45 2006 -0600
@@ -22,7 +22,7 @@ typedef Elf32_Nhdr Elf_Nhdr;
 typedef Elf32_Nhdr Elf_Nhdr;
 typedef Elf32_Half Elf_Half;
 typedef Elf32_Word Elf_Word;
-#elif defined(__x86_64__)
+#elif defined(__x86_64__) || defined(__ia64__)
 typedef Elf64_Ehdr Elf_Ehdr;
 typedef Elf64_Nhdr Elf_Nhdr;
 typedef Elf64_Half Elf_Half;

Re: [POWERPC/IA64] Updates required due to loader changes

[Hollis Blanchard]

On Thu, 2006-08-24 at 14:16 -0600, Alex Williamson wrote:
> 
> diff -r d5eb5205ff35 tools/xcutils/readnotes.c
> --- a/tools/xcutils/readnotes.c	Thu Aug 24 16:25:49 2006 +0100
> +++ b/tools/xcutils/readnotes.c	Thu Aug 24 14:12:45 2006 -0600
> @@ -22,7 +22,7 @@ typedef Elf32_Nhdr Elf_Nhdr;
>  typedef Elf32_Nhdr Elf_Nhdr;
>  typedef Elf32_Half Elf_Half;
>  typedef Elf32_Word Elf_Word;
> -#elif defined(__x86_64__)
> +#elif defined(__x86_64__) || defined(__ia64__)
>  typedef Elf64_Ehdr Elf_Ehdr;
>  typedef Elf64_Nhdr Elf_Nhdr;
>  typedef Elf64_Half Elf_Half;

Are we doing this again? Ian, please use ELFSIZE, as seen in the
following files:
	tools/libxc/xg_private.h
	tools/libxc/xc_elf.h
	tools/libxc/xc_load_elf.c
	config/powerpc64.mk

-- 
Hollis Blanchard
IBM Linux Technology Center

Re: [POWERPC/IA64] Updates required due to loader changes

[Horms]

On Thu, 24 Aug 2006 15:29:48 -0500, Hollis Blanchard wrote:
> On Thu, 2006-08-24 at 14:16 -0600, Alex Williamson wrote:
>> 
>> diff -r d5eb5205ff35 tools/xcutils/readnotes.c
>> --- a/tools/xcutils/readnotes.c       Thu Aug 24 16:25:49 2006 +0100
>> +++ b/tools/xcutils/readnotes.c       Thu Aug 24 14:12:45 2006 -0600
>> @@ -22,7 +22,7 @@ typedef Elf32_Nhdr Elf_Nhdr;
>>  typedef Elf32_Nhdr Elf_Nhdr;
>>  typedef Elf32_Half Elf_Half;
>>  typedef Elf32_Word Elf_Word;
>> -#elif defined(__x86_64__)
>> +#elif defined(__x86_64__) || defined(__ia64__)
>>  typedef Elf64_Ehdr Elf_Ehdr;
>>  typedef Elf64_Nhdr Elf_Nhdr;
>>  typedef Elf64_Half Elf_Half;
> 
> Are we doing this again? Ian, please use ELFSIZE, as seen in the
> following files:
>        tools/libxc/xg_private.h
>        tools/libxc/xc_elf.h
>        tools/libxc/xc_load_elf.c
>        config/powerpc64.mk
> 

Is the patch below what you had in mind?

I'm a little unsure about where the definition of ELFSIZE comes from.
tools/libxc/xg_private.h seems to take pains to make sure it is defined,
but tools/libxc/xc_elf.h, tools/libxc/xc_load_elf.c (and
xen/include/xen/elf.h), do not seem to.

-- 
Horms
  H: http://www.vergenet.net/~horms/
  W: http://www.valinux.co.jp/en/

Use ELFSIZE in tools/xcutils/readnotes.c

Signed-Off-By: Simon Horman <horms@verge.net.au>

--- 0001/tools/xcutils/readnotes.c
+++ work/tools/xcutils/readnotes.c	2006-08-25 12:17:37.000000000 +0900
@@ -17,18 +17,16 @@
 #define ELFNOTE_DESC(_n_) (ELFNOTE_NAME(_n_) + (((_n_)->n_namesz+3)&~3))
 #define ELFNOTE_NEXT(_n_) (ELFNOTE_DESC(_n_) + (((_n_)->n_descsz+3)&~3))
 
-#if defined(__i386__)
+#if defined(ELFSIZE) && (ELFSIZE == 32)
 typedef Elf32_Ehdr Elf_Ehdr;
 typedef Elf32_Nhdr Elf_Nhdr;
 typedef Elf32_Half Elf_Half;
 typedef Elf32_Word Elf_Word;
-#elif defined(__x86_64__)
+#elif defined(ELFSIZE) && (ELFSIZE == 64)
 typedef Elf64_Ehdr Elf_Ehdr;
 typedef Elf64_Nhdr Elf_Nhdr;
 typedef Elf64_Half Elf_Half;
 typedef Elf64_Word Elf_Word;
-#else
-#error "Unknown architecture"
 #endif
 
 static void print_string_note(const char *prefix, Elf_Nhdr *note)

Re: [POWERPC/IA64] Updates required due to loader changes

[Ian Campbell]

On Fri, 2006-08-25 at 12:21 +0900, Horms wrote:
> On Thu, 24 Aug 2006 15:29:48 -0500, Hollis Blanchard wrote:
> > On Thu, 2006-08-24 at 14:16 -0600, Alex Williamson wrote:
> >> 
> >> diff -r d5eb5205ff35 tools/xcutils/readnotes.c
> >> --- a/tools/xcutils/readnotes.c       Thu Aug 24 16:25:49 2006 +0100
> >> +++ b/tools/xcutils/readnotes.c       Thu Aug 24 14:12:45 2006 -0600
> >> @@ -22,7 +22,7 @@ typedef Elf32_Nhdr Elf_Nhdr;
> >>  typedef Elf32_Nhdr Elf_Nhdr;
> >>  typedef Elf32_Half Elf_Half;
> >>  typedef Elf32_Word Elf_Word;
> >> -#elif defined(__x86_64__)
> >> +#elif defined(__x86_64__) || defined(__ia64__)
> >>  typedef Elf64_Ehdr Elf_Ehdr;
> >>  typedef Elf64_Nhdr Elf_Nhdr;
> >>  typedef Elf64_Half Elf_Half;
> > 
> > Are we doing this again? Ian, please use ELFSIZE, as seen in the
> > following files:
> >        tools/libxc/xg_private.h
> >        tools/libxc/xc_elf.h
> >        tools/libxc/xc_load_elf.c
> >        config/powerpc64.mk
> > 
> 
> Is the patch below what you had in mind?

Nearly, we need to define ELFSIZE if it isn't already since not all
architectures define it.

I checked in the below. I got rid of Elf_Ehdr while I was there since it
is only used the e_ident field which is unsigned char [] for both
ELFSIZEs. As it happens all the remaining types are the same on 32 and
64 but nevermind...

Ian.

changeset:   11256:9091331dfb353212781622f3c9020492cb049178
tag:         tip
user:        Ian Campbell <ian.campbell@xensource.com>
date:        Fri Aug 25 10:39:24 2006 +0100
files:       tools/xcutils/readnotes.c
description:
[TOOLS] Use ELFSIZE to pick the ELF structures to use in readnotes.c

We can remove Elf_Ehdr since it is only used for e_ident which is an
unsigned char array.

Signed-off-by: Ian Campbell <ian.campbell@xensource.com>


diff -r 23a0a408edb9f0675eaec0493c9063c19a14b9cb -r 9091331dfb353212781622f3c9020492cb049178 tools/xcutils/readnotes.c
--- a/tools/xcutils/readnotes.c	Fri Aug 25 10:06:24 2006 +0100
+++ b/tools/xcutils/readnotes.c	Fri Aug 25 10:39:24 2006 +0100
@@ -17,18 +17,25 @@
 #define ELFNOTE_DESC(_n_) (ELFNOTE_NAME(_n_) + (((_n_)->n_namesz+3)&~3))
 #define ELFNOTE_NEXT(_n_) (ELFNOTE_DESC(_n_) + (((_n_)->n_descsz+3)&~3))
 
-#if defined(__i386__)
-typedef Elf32_Ehdr Elf_Ehdr;
+#ifndef ELFSIZE
+#include <limits.h>
+#if UINT_MAX == ULONG_MAX
+#define ELFSIZE 32
+#else
+#define ELFSIZE 64
+#endif
+#endif
+
+#if (ELFSIZE == 32)
 typedef Elf32_Nhdr Elf_Nhdr;
 typedef Elf32_Half Elf_Half;
 typedef Elf32_Word Elf_Word;
-#elif defined(__x86_64__)
-typedef Elf64_Ehdr Elf_Ehdr;
+#elif (ELFSIZE == 64)
 typedef Elf64_Nhdr Elf_Nhdr;
 typedef Elf64_Half Elf_Half;
 typedef Elf64_Word Elf_Word;
 #else
-#error "Unknown architecture"
+#error "Unknown ELFSIZE"
 #endif
 
 static void print_string_note(const char *prefix, Elf_Nhdr *note)
@@ -54,18 +61,35 @@ static void print_numeric_note(const cha
 	}
 }
 
+static inline int is_elf(void *image)
+{
+	/*
+	 * Since we are only accessing the e_ident field we can
+	 * acccess the bytes directly without needing to figure out
+	 * which version of Elf*_Ehdr structure to use.
+	 */
+	const unsigned char *hdr = image;
+	return ( hdr[EI_MAG0] == ELFMAG0 &&
+		 hdr[EI_MAG1] == ELFMAG1 &&
+		 hdr[EI_MAG2] == ELFMAG2 &&
+		 hdr[EI_MAG3] == ELFMAG3 );
+}
+
 static inline unsigned char ehdr_class(void *image)
 {
-	Elf_Ehdr *ehdr = image;
-	switch (ehdr->e_ident[EI_CLASS])
-	{
-	case ELFCLASS32:
-	case ELFCLASS64:
-		return ehdr->e_ident[EI_CLASS];
-		break;
-	default:
-		fprintf(stderr, "Unknown ELF class %d\n",
-			ehdr->e_ident[EI_CLASS]);
+	/*
+	 * Since we are only accessing the e_ident field we can
+	 * acccess the bytes directly without needing to figure out
+	 * which version of Elf*_Ehdr structure to use.
+	 */
+	const unsigned char *hdr = image;
+	switch (hdr[EI_CLASS])
+	{
+	case ELFCLASS32:
+	case ELFCLASS64:
+		return hdr[EI_CLASS];
+	default:
+		fprintf(stderr, "Unknown ELF class %d\n", hdr[EI_CLASS]);
 		exit(1);
 	}
 }
@@ -198,7 +222,6 @@ int main(int argc, char **argv)
 	int fd,h;
 	void *image;
 	struct stat st;
-	Elf_Ehdr *ehdr;
 	Elf_Nhdr *note;
 
 	if (argc != 2)
@@ -228,11 +251,7 @@ int main(int argc, char **argv)
 		return 1;
 	}
 
-	ehdr = image;
-	if (ehdr->e_ident[EI_MAG0] != ELFMAG0 ||
-	    ehdr->e_ident[EI_MAG1] != ELFMAG1 ||
-	    ehdr->e_ident[EI_MAG2] != ELFMAG2 ||
-	    ehdr->e_ident[EI_MAG3] != ELFMAG3)
+	if ( !is_elf(image) )
 	{
 		fprintf(stderr, "File %s is not an ELF image\n", f);
 		return 1;

Re: [POWERPC/IA64] Updates required due to loader changes

[Hollis Blanchard]

On Fri, 2006-08-25 at 10:42 +0100, Ian Campbell wrote:
> 
> Nearly, we need to define ELFSIZE if it isn't already since not all
> architectures define it.

Yup, that allows us to override ELFSIZE on mixed architectures, but have
it default to the right thing elsewhere.

> I checked in the below. I got rid of Elf_Ehdr while I was there since it
> is only used the e_ident field which is unsigned char [] for both
> ELFSIZEs. As it happens all the remaining types are the same on 32 and
> 64 but nevermind...
> 
> Ian.
> 
> changeset:   11256:9091331dfb353212781622f3c9020492cb049178
> tag:         tip
> user:        Ian Campbell <ian.campbell@xensource.com>
> date:        Fri Aug 25 10:39:24 2006 +0100
> files:       tools/xcutils/readnotes.c
> description:
> [TOOLS] Use ELFSIZE to pick the ELF structures to use in readnotes.c
> 
> We can remove Elf_Ehdr since it is only used for e_ident which is an
> unsigned char array.
> 
> Signed-off-by: Ian Campbell <ian.campbell@xensource.com>

Thanks Ian!

-- 
Hollis Blanchard
IBM Linux Technology Center

Re: [POWERPC/IA64] Updates required due to loader changes

[Horms]

On Fri, Aug 25, 2006 at 10:42:25AM +0100, Ian Campbell wrote:
> On Fri, 2006-08-25 at 12:21 +0900, Horms wrote:
> > On Thu, 24 Aug 2006 15:29:48 -0500, Hollis Blanchard wrote:
> > > On Thu, 2006-08-24 at 14:16 -0600, Alex Williamson wrote:
> > >> 
> > >> diff -r d5eb5205ff35 tools/xcutils/readnotes.c
> > >> --- a/tools/xcutils/readnotes.c       Thu Aug 24 16:25:49 2006 +0100
> > >> +++ b/tools/xcutils/readnotes.c       Thu Aug 24 14:12:45 2006 -0600
> > >> @@ -22,7 +22,7 @@ typedef Elf32_Nhdr Elf_Nhdr;
> > >>  typedef Elf32_Nhdr Elf_Nhdr;
> > >>  typedef Elf32_Half Elf_Half;
> > >>  typedef Elf32_Word Elf_Word;
> > >> -#elif defined(__x86_64__)
> > >> +#elif defined(__x86_64__) || defined(__ia64__)
> > >>  typedef Elf64_Ehdr Elf_Ehdr;
> > >>  typedef Elf64_Nhdr Elf_Nhdr;
> > >>  typedef Elf64_Half Elf_Half;
> > > 
> > > Are we doing this again? Ian, please use ELFSIZE, as seen in the
> > > following files:
> > >        tools/libxc/xg_private.h
> > >        tools/libxc/xc_elf.h
> > >        tools/libxc/xc_load_elf.c
> > >        config/powerpc64.mk
> > > 
> > 
> > Is the patch below what you had in mind?
> 
> Nearly, we need to define ELFSIZE if it isn't already since not all
> architectures define it.

I was wondering about that. Perhaps the defination you propose
could be moved somewhere central, like elf.h.

-- 
Horms
  H: http://www.vergenet.net/~horms/
  W: http://www.valinux.co.jp/en/