| Message ID | 1288623713-28062-3-git-send-email-agraf@suse.de |
|---|---|
| State | New |
| Headers | show |
On Mon, Nov 1, 2010 at 3:01 PM, Alexander Graf <agraf@suse.de> wrote: > --- > hw/elf_ops.h | 61 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++- > hw/loader.c | 7 ++++++ > hw/loader.h | 3 ++ > 3 files changed, 70 insertions(+), 1 deletions(-) > > diff --git a/hw/elf_ops.h b/hw/elf_ops.h > index 8b63dfc..645d058 100644 > --- a/hw/elf_ops.h > +++ b/hw/elf_ops.h > @@ -189,6 +189,44 @@ static int glue(load_symbols, SZ)(struct elfhdr *ehdr, int fd, int must_swab, > return -1; > } > > +static void glue(elf_read_notes, SZ)(uint8_t *data, int data_len, > + ElfHandlers *handlers, int must_swab) > +{ > + uint8_t *p = data; > + > + while ((ulong)&p[3] < (ulong)&data[data_len]) { Please use 'unsigned long'.
On 11/01/2010 04:01 PM, Alexander Graf wrote: > diff --git a/hw/loader.c b/hw/loader.c > index 50b43a0..cb430e0 100644 > --- a/hw/loader.c > +++ b/hw/loader.c > @@ -229,6 +229,11 @@ int load_aout(const char *filename, target_phys_addr_t addr, int max_sz, > > /* ELF loader */ > > +static void elf_default_note(void *opaque, uint8_t *name, uint32_t name_len, > + uint8_t *desc, uint32_t desc_len, uint32_t type) > +{ > +} > + > static uint64_t elf_default_translate(void *opaque, uint64_t addr) > { > return addr; > @@ -237,6 +242,8 @@ static uint64_t elf_default_translate(void *opaque, uint64_t addr) > ElfHandlers elf_default_handlers = { > .translate_fn = elf_default_translate, > .translate_opaque = NULL, > + .note_fn = elf_default_note, > + .note_opaque = NULL, Don't you have to add the definition to every user of translate_fn? Maybe it's better to guard calls through the pointers with an if. Paolo
Am 01.11.2010 19:29, schrieb Blue Swirl: > On Mon, Nov 1, 2010 at 3:01 PM, Alexander Graf<agraf@suse.de> wrote: > >> --- >> hw/elf_ops.h | 61 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++- >> hw/loader.c | 7 ++++++ >> hw/loader.h | 3 ++ >> 3 files changed, 70 insertions(+), 1 deletions(-) >> >> diff --git a/hw/elf_ops.h b/hw/elf_ops.h >> index 8b63dfc..645d058 100644 >> --- a/hw/elf_ops.h >> +++ b/hw/elf_ops.h >> @@ -189,6 +189,44 @@ static int glue(load_symbols, SZ)(struct elfhdr *ehdr, int fd, int must_swab, >> return -1; >> } >> >> +static void glue(elf_read_notes, SZ)(uint8_t *data, int data_len, >> + ElfHandlers *handlers, int must_swab) >> +{ >> + uint8_t *p = data; >> + >> + while ((ulong)&p[3]< (ulong)&data[data_len]) { >> > Please use 'unsigned long'. > Why is a type cast used here? I see no reason for it.
On 01.11.2010, at 14:41, Paolo Bonzini wrote: > On 11/01/2010 04:01 PM, Alexander Graf wrote: >> diff --git a/hw/loader.c b/hw/loader.c >> index 50b43a0..cb430e0 100644 >> --- a/hw/loader.c >> +++ b/hw/loader.c >> @@ -229,6 +229,11 @@ int load_aout(const char *filename, target_phys_addr_t addr, int max_sz, >> >> /* ELF loader */ >> >> +static void elf_default_note(void *opaque, uint8_t *name, uint32_t name_len, >> + uint8_t *desc, uint32_t desc_len, uint32_t type) >> +{ >> +} >> + >> static uint64_t elf_default_translate(void *opaque, uint64_t addr) >> { >> return addr; >> @@ -237,6 +242,8 @@ static uint64_t elf_default_translate(void *opaque, uint64_t addr) >> ElfHandlers elf_default_handlers = { >> .translate_fn = elf_default_translate, >> .translate_opaque = NULL, >> + .note_fn = elf_default_note, >> + .note_opaque = NULL, > > Don't you have to add the definition to every user of translate_fn? > > Maybe it's better to guard calls through the pointers with an if. All users either pass NULL as translate (which means they default to elf_default_translate) or initialize their structure with the values in elf_default_translate :) Alex
On 11/01/2010 07:52 PM, Alexander Graf wrote: >>> @@ -237,6 +242,8 @@ static uint64_t elf_default_translate(void *opaque, uint64_t addr) >>> ElfHandlers elf_default_handlers = { >>> .translate_fn = elf_default_translate, >>> .translate_opaque = NULL, >>> + .note_fn = elf_default_note, >>> + .note_opaque = NULL, >> >> Don't you have to add the definition to every user of translate_fn? >> >> Maybe it's better to guard calls through the pointers with an if. > > All users either pass NULL as translate (which means they default to > elf_default_translate) or initialize their structure with the values in > elf_default_translate :) But do the MIPS users initialize note_fn? Paolo
On 01.11.2010, at 15:43, Paolo Bonzini wrote: > On 11/01/2010 07:52 PM, Alexander Graf wrote: >>>> @@ -237,6 +242,8 @@ static uint64_t elf_default_translate(void *opaque, uint64_t addr) >>>> ElfHandlers elf_default_handlers = { >>>> .translate_fn = elf_default_translate, >>>> .translate_opaque = NULL, >>>> + .note_fn = elf_default_note, >>>> + .note_opaque = NULL, >>> >>> Don't you have to add the definition to every user of translate_fn? >>> >>> Maybe it's better to guard calls through the pointers with an if. >> >> All users either pass NULL as translate (which means they default to >> elf_default_translate) or initialize their structure with the values in >> elf_default_translate :) > > But do the MIPS users initialize note_fn? They should: @@ -106,8 +106,10 @@ static int64_t load_kernel (CPUState *env) ram_addr_t initrd_offset; uint32_t *prom_buf; long prom_size; + ElfHandlers handlers = elf_default_handlers; - if (load_elf(loaderparams.kernel_filename, cpu_mips_kseg0_to_phys, NULL, + handlers.translate_fn = cpu_mips_kseg0_to_phys; + if (load_elf(loaderparams.kernel_filename, &handlers, (uint64_t *)&kernel_entry, (uint64_t *)&kernel_low, (uint64_t *)&kernel_high, 0, ELF_MACHINE, 1) < 0) { fprintf(stderr, "qemu: could not load kernel '%s'\n", Unless my C foo is really bad, this means that handlers is initialized with the contents of elf_default_handlers :). And that's how every caller works. Alex
On 01.11.2010, at 14:42, Stefan Weil wrote: > Am 01.11.2010 19:29, schrieb Blue Swirl: >> On Mon, Nov 1, 2010 at 3:01 PM, Alexander Graf<agraf@suse.de> wrote: >> >>> --- >>> hw/elf_ops.h | 61 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++- >>> hw/loader.c | 7 ++++++ >>> hw/loader.h | 3 ++ >>> 3 files changed, 70 insertions(+), 1 deletions(-) >>> >>> diff --git a/hw/elf_ops.h b/hw/elf_ops.h >>> index 8b63dfc..645d058 100644 >>> --- a/hw/elf_ops.h >>> +++ b/hw/elf_ops.h >>> @@ -189,6 +189,44 @@ static int glue(load_symbols, SZ)(struct elfhdr *ehdr, int fd, int must_swab, >>> return -1; >>> } >>> >>> +static void glue(elf_read_notes, SZ)(uint8_t *data, int data_len, >>> + ElfHandlers *handlers, int must_swab) >>> +{ >>> + uint8_t *p = data; >>> + >>> + while ((ulong)&p[3]< (ulong)&data[data_len]) { >>> >> Please use 'unsigned long'. >> > > Why is a type cast used here? I see no reason for it. Pointers can't be compared, you have to cast them to values first. Alex
Am 01.11.2010 20:51, schrieb Alexander Graf: > On 01.11.2010, at 14:42, Stefan Weil wrote: > > >> Am 01.11.2010 19:29, schrieb Blue Swirl: >> >>> On Mon, Nov 1, 2010 at 3:01 PM, Alexander Graf<agraf@suse.de> wrote: >>> >>> >>>> --- >>>> hw/elf_ops.h | 61 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++- >>>> hw/loader.c | 7 ++++++ >>>> hw/loader.h | 3 ++ >>>> 3 files changed, 70 insertions(+), 1 deletions(-) >>>> >>>> diff --git a/hw/elf_ops.h b/hw/elf_ops.h >>>> index 8b63dfc..645d058 100644 >>>> --- a/hw/elf_ops.h >>>> +++ b/hw/elf_ops.h >>>> @@ -189,6 +189,44 @@ static int glue(load_symbols, SZ)(struct elfhdr *ehdr, int fd, int must_swab, >>>> return -1; >>>> } >>>> >>>> +static void glue(elf_read_notes, SZ)(uint8_t *data, int data_len, >>>> + ElfHandlers *handlers, int must_swab) >>>> +{ >>>> + uint8_t *p = data; >>>> + >>>> + while ((ulong)&p[3]< (ulong)&data[data_len]) { >>>> >>>> >>> Please use 'unsigned long'. >>> >>> >> Why is a type cast used here? I see no reason for it. >> > Pointers can't be compared, you have to cast them to values first. > > > Alex > No. Pointers of same type which are not void pointers can be compared. There is even a data type ptrdiff_t, so you can also compare their difference with zero. Regards, Stefan
On 01.11.2010, at 16:19, Stefan Weil wrote: > Am 01.11.2010 20:51, schrieb Alexander Graf: >> On 01.11.2010, at 14:42, Stefan Weil wrote: >> >> >>> Am 01.11.2010 19:29, schrieb Blue Swirl: >>> >>>> On Mon, Nov 1, 2010 at 3:01 PM, Alexander Graf<agraf@suse.de> wrote: >>>> >>>> >>>>> --- >>>>> hw/elf_ops.h | 61 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++- >>>>> hw/loader.c | 7 ++++++ >>>>> hw/loader.h | 3 ++ >>>>> 3 files changed, 70 insertions(+), 1 deletions(-) >>>>> >>>>> diff --git a/hw/elf_ops.h b/hw/elf_ops.h >>>>> index 8b63dfc..645d058 100644 >>>>> --- a/hw/elf_ops.h >>>>> +++ b/hw/elf_ops.h >>>>> @@ -189,6 +189,44 @@ static int glue(load_symbols, SZ)(struct elfhdr *ehdr, int fd, int must_swab, >>>>> return -1; >>>>> } >>>>> >>>>> +static void glue(elf_read_notes, SZ)(uint8_t *data, int data_len, >>>>> + ElfHandlers *handlers, int must_swab) >>>>> +{ >>>>> + uint8_t *p = data; >>>>> + >>>>> + while ((ulong)&p[3]< (ulong)&data[data_len]) { >>>>> >>>>> >>>> Please use 'unsigned long'. >>>> >>>> >>> Why is a type cast used here? I see no reason for it. >>> >> Pointers can't be compared, you have to cast them to values first. >> >> >> Alex >> > > No. Pointers of same type which are not void pointers can be compared. > > There is even a data type ptrdiff_t, so you can also compare their > difference with zero. Let's ask someone who definitely knows :). Michael, is code like char *x = a, *y = b; if (x < y) { ... } valid? Or do I first have to cast x and y to unsigned longs or uintptr_t? Alex
On 11/01/2010 08:48 PM, Alexander Graf wrote: > @@ -106,8 +106,10 @@ static int64_t load_kernel (CPUState *env) > ram_addr_t initrd_offset; > uint32_t *prom_buf; > long prom_size; > + ElfHandlers handlers = elf_default_handlers; > > - if (load_elf(loaderparams.kernel_filename, cpu_mips_kseg0_to_phys, NULL, > + handlers.translate_fn = cpu_mips_kseg0_to_phys; > + if (load_elf(loaderparams.kernel_filename,&handlers, > (uint64_t *)&kernel_entry, (uint64_t *)&kernel_low, > (uint64_t *)&kernel_high, 0, ELF_MACHINE, 1)< 0) { > fprintf(stderr, "qemu: could not load kernel '%s'\n", > > > Unless my C foo is really bad, this means that handlers is > initialized with the contents of elf_default_handlers :). And > that's how every caller works. Sorry, my mistake. Paolo
On 11/01/2010 10:17 PM, Alexander Graf wrote: > Let's ask someone who definitely knows:). LOL, hi Michael! :) > Michael, is code like > > char *x = a, *y = b; > if (x < y) { > ... > } > > valid? Or do I first have to cast x and y to unsigned longs or uintptr_t? It is, as long as x and y point into the same object (in your original code, data[0]...data[data_len] is the object). This instead char *x = a; long *y = b; if (x < y) { } should give a warning g2.c:1: warning: comparison of distinct pointer types lacks a cast but is also valid as long as x and y point into the same object. To quiet the warning you should _not_ cast x to long* however (unless you know it's properly aligned); casting y to char* instead is fine. Paolo
Am 01.11.2010 22:17, schrieb Alexander Graf: > > On 01.11.2010, at 16:19, Stefan Weil wrote: > >> Am 01.11.2010 20:51, schrieb Alexander Graf: >>> On 01.11.2010, at 14:42, Stefan Weil wrote: >>> >>> >>>> Am 01.11.2010 19:29, schrieb Blue Swirl: >>>> >>>>> On Mon, Nov 1, 2010 at 3:01 PM, Alexander Graf<agraf@suse.de> wrote: >>>>> >>>>> >>>>>> --- >>>>>> hw/elf_ops.h | 61 >>>>>> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++- >>>>>> hw/loader.c | 7 ++++++ >>>>>> hw/loader.h | 3 ++ >>>>>> 3 files changed, 70 insertions(+), 1 deletions(-) >>>>>> >>>>>> diff --git a/hw/elf_ops.h b/hw/elf_ops.h >>>>>> index 8b63dfc..645d058 100644 >>>>>> --- a/hw/elf_ops.h >>>>>> +++ b/hw/elf_ops.h >>>>>> @@ -189,6 +189,44 @@ static int glue(load_symbols, SZ)(struct >>>>>> elfhdr *ehdr, int fd, int must_swab, >>>>>> return -1; >>>>>> } >>>>>> >>>>>> +static void glue(elf_read_notes, SZ)(uint8_t *data, int data_len, >>>>>> + ElfHandlers *handlers, int must_swab) >>>>>> +{ >>>>>> + uint8_t *p = data; >>>>>> + >>>>>> + while ((ulong)&p[3]< (ulong)&data[data_len]) { >>>>>> >>>>>> >>>>> Please use 'unsigned long'. >>>>> >>>>> >>>> Why is a type cast used here? I see no reason for it. >>>> >>> Pointers can't be compared, you have to cast them to values first. >>> >>> >>> Alex >>> >> >> No. Pointers of same type which are not void pointers can be compared. >> >> There is even a data type ptrdiff_t, so you can also compare their >> difference with zero. > > Let's ask someone who definitely knows :). > > Michael, is code like > > char *x = a, *y = b; > if (x < y) { > ... > } > > valid? Or do I first have to cast x and y to unsigned longs or uintptr_t? > > > Alex > Hopefully C did not change for code like this during the last 20 years. Then your code is always valid, but will only return useful results if both a and b are derived from the same base pointer (plus an individual offset): char *base = any_value; a = base + offset_a; b = base + offset_b; Then x - y = a - b = (ptrdiff_t)(offset_a - offset_b) / (sizeof(*x); and (x < y) == (a < b) == (offset_a < offset_b); Regards, Stefan
Hi, On Mon, 1 Nov 2010, Alexander Graf wrote: > > No. Pointers of same type which are not void pointers can be compared. > > > > There is even a data type ptrdiff_t, so you can also compare their > > difference with zero. > > Let's ask someone who definitely knows :). > > Michael, is code like > > char *x = a, *y = b; > if (x < y) { > ... > } Pointers can be compared iff they point into the same object (including right after the object), so it depends on what a and b were above. This would be invalid for instance: int o1, o2; int *p1 = &o1, *p2 = &o2; if (p1 < p2) ... > valid? Or do I first have to cast x and y to unsigned longs or > uintptr_t? For doing a valid pointer comparison you don't have to cast anything. Casting doesn't make an invalid comparison valid. Ciao, Michael.
diff --git a/hw/elf_ops.h b/hw/elf_ops.h index 8b63dfc..645d058 100644 --- a/hw/elf_ops.h +++ b/hw/elf_ops.h @@ -189,6 +189,44 @@ static int glue(load_symbols, SZ)(struct elfhdr *ehdr, int fd, int must_swab, return -1; } +static void glue(elf_read_notes, SZ)(uint8_t *data, int data_len, + ElfHandlers *handlers, int must_swab) +{ + uint8_t *p = data; + + while ((ulong)&p[3] < (ulong)&data[data_len]) { + uint32_t *cur = (uint32_t *)p; + uint32_t namesz = cur[0]; + uint32_t descsz = cur[1]; + uint32_t type = cur[2]; + uint8_t *name; + uint8_t *desc; + + p += 3 * sizeof(uint32_t); + + if (must_swab) { + namesz = bswap32(namesz); + descsz = bswap32(descsz); + type = bswap32(type); + } + + namesz = (namesz + 3) & ~3; + descsz = (descsz + 3) & ~3; + + name = p; + p += namesz; + desc = p; + p += descsz; + + if ((ulong)p > (ulong)&data[data_len]) { + break; + } + + handlers->note_fn(handlers->note_opaque, name, namesz, desc, descsz, + type); + } +} + static int glue(load_elf, SZ)(const char *name, int fd, ElfHandlers *handlers, int must_swab, uint64_t *pentry, @@ -252,7 +290,8 @@ static int glue(load_elf, SZ)(const char *name, int fd, total_size = 0; for(i = 0; i < ehdr.e_phnum; i++) { ph = &phdr[i]; - if (ph->p_type == PT_LOAD) { + switch (ph->p_type) { + case PT_LOAD: mem_size = ph->p_memsz; /* XXX: avoid allocating */ data = qemu_mallocz(mem_size); @@ -278,6 +317,26 @@ static int glue(load_elf, SZ)(const char *name, int fd, qemu_free(data); data = NULL; + break; + + case PT_NOTE: + mem_size = ph->p_memsz; + if (!mem_size) { + break; + } + data = qemu_mallocz(mem_size); + if (ph->p_filesz > 0) { + if (lseek(fd, ph->p_offset, SEEK_SET) < 0) + goto fail; + if (read(fd, data, ph->p_filesz) != ph->p_filesz) + goto fail; + } + + glue(elf_read_notes, SZ)(data, ph->p_memsz, handlers, must_swab); + + qemu_free(data); + data = NULL; + break; } } qemu_free(phdr); diff --git a/hw/loader.c b/hw/loader.c index 50b43a0..cb430e0 100644 --- a/hw/loader.c +++ b/hw/loader.c @@ -229,6 +229,11 @@ int load_aout(const char *filename, target_phys_addr_t addr, int max_sz, /* ELF loader */ +static void elf_default_note(void *opaque, uint8_t *name, uint32_t name_len, + uint8_t *desc, uint32_t desc_len, uint32_t type) +{ +} + static uint64_t elf_default_translate(void *opaque, uint64_t addr) { return addr; @@ -237,6 +242,8 @@ static uint64_t elf_default_translate(void *opaque, uint64_t addr) ElfHandlers elf_default_handlers = { .translate_fn = elf_default_translate, .translate_opaque = NULL, + .note_fn = elf_default_note, + .note_opaque = NULL, }; diff --git a/hw/loader.h b/hw/loader.h index 27a2c36..29d5c71 100644 --- a/hw/loader.h +++ b/hw/loader.h @@ -9,6 +9,9 @@ int load_image_targphys(const char *filename, target_phys_addr_t, int max_sz); typedef struct ElfHandlers { uint64_t (*translate_fn)(void *opaque, uint64_t address); void *translate_opaque; + void (*note_fn)(void *opaque, uint8_t *name, uint32_t name_len, + uint8_t *desc, uint32_t desc_len, uint32_t type); + void *note_opaque; } ElfHandlers; extern ElfHandlers elf_default_handlers;