Re: [Minios-devel] [UNIKRAFT PATCH v2 2/2] plat: Register eh_frame and eh_frame_hdr in memory.h

[Vlad-Andrei BĂDOIU (78692) <vlad_andrei.badoiu@xxxxxxxxxxxxxxx>]

Hey Sharan,

Please see my comments inline.

Thanks,

Vlad

On 3/15/19 7:49 PM, Sharan Santhanam wrote:
> Hello Vlad,
>
> There are some checkpatch warning/error
>
> ERROR: trailing whitespace
> #111: FILE: plat/xen/memory.c:108:
> +^I^Im->len   = (size_t) __EH_FRAME_END $
>
> ERROR: code indent should use tabs where possible
> #112: FILE: plat/xen/memory.c:109:
> +                - (size_t) __EH_FRAME_START;$
>
> WARNING: please, no spaces at the start of a line
> #112: FILE: plat/xen/memory.c:109:
> +                - (size_t) __EH_FRAME_START;$
>
> ERROR: trailing whitespace
> #121: FILE: plat/xen/memory.c:118:
> +^I^Im->len   = (size_t) __EH_FRAME_HDR_END $
>
> ERROR: code indent should use tabs where possible
> #122: FILE: plat/xen/memory.c:119:
> +                - (size_t) __EH_FRAME_HDR_START;$
>
> WARNING: please, no spaces at the start of a line
> #122: FILE: plat/xen/memory.c:119:
> +                - (size_t) __EH_FRAME_HDR_START;$
>
> Please find the comments inline.
Thanks. I forgot to run checkpatch, next time I'll keep it in mind.
>
>
> Thanks & Regards
> Sharan
>
> On 3/11/19 9:28 PM, Vlad-Andrei BĂDOIU (78692) wrote:
>> Signed-off-by: Vlad-Andrei Badoiu <vlad_andrei.badoiu@xxxxxxxxxxxxxxx>
>> ---
>>   plat/common/include/sections.h | 32 +++++++++++++++++++++-----------
>>   plat/kvm/memory.c              | 24 +++++++++++++++++++++++-
>>   plat/xen/memory.c              | 20 ++++++++++++++++++++
>>   3 files changed, 64 insertions(+), 12 deletions(-)
>>
>> diff --git a/plat/common/include/sections.h 
>> b/plat/common/include/sections.h
>> index ecb36072..1052cfc7 100644
>> --- a/plat/common/include/sections.h
>> +++ b/plat/common/include/sections.h
>> @@ -48,6 +48,12 @@ extern char _dtb[];
>>   /* [_text, _etext]: contains .text.* sections */
>>   extern char _text[], _etext[];
>>   +/* [__eh_frame_start, __eh_frame_end]: contains .eh_frame section */
>> +extern char __eh_frame_start, __eh_frame_end;
>> +
>> +/* [__eh_frame_hdr_start, __eh_frame_hdr_end]: contains 
>> .eh_frame_hdr section */
>> +extern char __eh_frame_hdr_start, __eh_frame_hdr_end;
>> +
>>   /* [_rodata, _erodata]: contains .rodata.* sections */
>>   extern char _rodata[], _erodata[];
>>   @@ -65,17 +71,21 @@ extern char _end[];
>>     #define __uk_image_symbol(addr)    ((unsigned long)(addr))
>>   -#define __DTB        __uk_image_symbol(_dtb)
>> -#define __TEXT        __uk_image_symbol(_text)
>> -#define __ETEXT        __uk_image_symbol(_etext)
>> -#define __RODATA    __uk_image_symbol(_rodata)
>> -#define __ERODATA    __uk_image_symbol(_erodata)
>> -#define __DATA        __uk_image_symbol(_data)
>> -#define __EDATA        __uk_image_symbol(_edata)
>> -#define __CTORS        __uk_image_symbol(_ctors)
>> -#define __ECTORS    __uk_image_symbol(_ectors)
>> -#define __BSS_START    __uk_image_symbol(__bss_start)
>> -#define __END        __uk_image_symbol(_end)
>> +#define __DTB                   __uk_image_symbol(_dtb)
>> +#define __TEXT                  __uk_image_symbol(_text)
>> +#define __ETEXT                 __uk_image_symbol(_etext)
>> +#define __EH_FRAME_START __uk_image_symbol(__eh_frame_start)
>> +#define __EH_FRAME_END __uk_image_symbol(__eh_frame_end)
>> +#define __EH_FRAME_HDR_START __uk_image_symbol(__eh_frame_hdr_start)
>> +#define __EH_FRAME_HDR_END __uk_image_symbol(__eh_frame_hdr_end)
>> +#define __RODATA                __uk_image_symbol(_rodata)
>> +#define __ERODATA               __uk_image_symbol(_erodata)
>> +#define __DATA                  __uk_image_symbol(_data)
>> +#define __EDATA                 __uk_image_symbol(_edata)
>> +#define __CTORS                 __uk_image_symbol(_ctors)
>> +#define __ECTORS                __uk_image_symbol(_ectors)
>> +#define __BSS_START             __uk_image_symbol(__bss_start)
>> +#define __END                   __uk_image_symbol(_end)
>>     #endif /*__ASSEMBLY__*/
>>   diff --git a/plat/kvm/memory.c b/plat/kvm/memory.c
>> index a7b4d5ef..6c520941 100644
>> --- a/plat/kvm/memory.c
>> +++ b/plat/kvm/memory.c
>> @@ -33,7 +33,7 @@ extern void *_libkvmplat_mem_end;
>>     int ukplat_memregion_count(void)
>>   {
>> -    return 7;
>> +    return 9;
>>   }
>>     int ukplat_memregion_get(int i, struct ukplat_memregion_desc *m)
>> @@ -115,6 +115,28 @@ int ukplat_memregion_get(int i, struct 
>> ukplat_memregion_desc *m)
>>           ret = 0;
>>   #if CONFIG_UKPLAT_MEMRNAME
>>           m->name  = "bstack";
>> +#endif
>> +        break;
>
> How did you come about the numbering scheme?
>
> Do you think it might be wiser to have the section based on the 
> increasing order of the address?
I agree, they should be numbered based based on the address. I'll change 
this for v2 of this patch.
>> +    case 7: /* eh_frame */
>> +        m->base  = (void *) __EH_FRAME_START;
>> +        m->len   = (size_t) __EH_FRAME_END
>> +               - (size_t) __EH_FRAME_START;
>> +        m->flags = (UKPLAT_MEMRF_RESERVED
>> +                | UKPLAT_MEMRF_READABLE);
>> +        ret = 0;
>> +#if CONFIG_UKPLAT_MEMRNAME
>> +        m->name  = "eh_frame";
>> +#endif
>> +        break;
>> +    case 8: /* eh_frame_hdr */
>> +        m->base  = (void *) __EH_FRAME_HDR_START;
>> +        m->len   = (size_t) __EH_FRAME_HDR_END
>> +               - (size_t) __EH_FRAME_HDR_START;
>> +        m->flags = (UKPLAT_MEMRF_RESERVED
>> +                | UKPLAT_MEMRF_READABLE);
>> +        ret = 0;
>> +#if CONFIG_UKPLAT_MEMRNAME
>> +        m->name  = "eh_frame_hdr";
>>   #endif
>>           break;
>>       default:
>> diff --git a/plat/xen/memory.c b/plat/xen/memory.c
>> index b63f11bb..2ffa6151 100644
>> --- a/plat/xen/memory.c
>> +++ b/plat/xen/memory.c
>> @@ -101,6 +101,26 @@ int ukplat_memregion_get(int i, struct 
>> ukplat_memregion_desc *m)
>>                   | UKPLAT_MEMRF_WRITABLE);
>>   #if CONFIG_UKPLAT_MEMRNAME
>>           m->name  = "bss";
>> +#endif
>> +        break;
>> +    case 5: /* eh_frame */
>> +        m->base  = (void *) __EH_FRAME_START;
>> +        m->len   = (size_t) __EH_FRAME_END
>> +                - (size_t) __EH_FRAME_START;
>> +        m->flags = (UKPLAT_MEMRF_RESERVED
>> +                | UKPLAT_MEMRF_READABLE);
>> +#if CONFIG_UKPLAT_MEMRNAME
>> +        m->name  = "eh_frame";
>> +#endif
>> +        break;
>> +    case 6: /* eh_frame_hdr */
>> +        m->base  = (void *) __EH_FRAME_HDR_START;
>> +        m->len   = (size_t) __EH_FRAME_HDR_END
>> +                - (size_t) __EH_FRAME_HDR_START;
>> +        m->flags = (UKPLAT_MEMRF_RESERVED
>> +                | UKPLAT_MEMRF_READABLE);
>> +#if CONFIG_UKPLAT_MEMRNAME
>> +        m->name  = "eh_frame_hdr";
>>   #endif
>>           break;
>>       default:
>>

_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel