Patchwork [02/40] elf: Add notes implementation

login
register
mail settings
Submitter Alexander Graf
Date Nov. 1, 2010, 3:01 p.m.
Message ID <1288623713-28062-3-git-send-email-agraf@suse.de>
Download mbox | patch
Permalink /patch/69779/
State New
Headers show

Comments

Alexander Graf - Nov. 1, 2010, 3:01 p.m.
---
 hw/elf_ops.h |   61 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 hw/loader.c  |    7 ++++++
 hw/loader.h  |    3 ++
 3 files changed, 70 insertions(+), 1 deletions(-)
Blue Swirl - Nov. 1, 2010, 6:29 p.m.
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'.
Paolo Bonzini - Nov. 1, 2010, 6:41 p.m.
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
Stefan Weil - Nov. 1, 2010, 6:42 p.m.
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.
Alexander Graf - Nov. 1, 2010, 6:52 p.m.
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
Paolo Bonzini - Nov. 1, 2010, 7:43 p.m.
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
Alexander Graf - Nov. 1, 2010, 7:48 p.m.
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
Alexander Graf - Nov. 1, 2010, 7:51 p.m.
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
Stefan Weil - Nov. 1, 2010, 8:19 p.m.
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
Alexander Graf - Nov. 1, 2010, 9:17 p.m.
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
Paolo Bonzini - Nov. 1, 2010, 9:23 p.m.
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
Paolo Bonzini - Nov. 1, 2010, 9:28 p.m.
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
Stefan Weil - Nov. 1, 2010, 9:31 p.m.
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
Michael Matz - Nov. 2, 2010, 10:17 a.m.
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.

Patch

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;