Re: [PATCH v8.1 11/27] xsplice: Implement payload loading

["Jan Beulich" <JBeulich@suse.com>]

>>> On 21.04.16 at 17:15, <konrad@kernel.org> wrote:
> On Wed, Apr 20, 2016 at 11:59:34AM -0400, Konrad Rzeszutek Wilk wrote:
>> > >@@ -29,6 +30,13 @@ struct payload {
>> >      >uint32_t state;                      /* One of the XSPLICE_STATE_*. */
>> >      >int32_t rc;                          /* 0 or -XEN_EXX. */
>> >      >struct list_head list;               /* Linked to 'payload_list'. */
>> > >+    void *text_addr;                     /* Virtual address of .text. */
>> > >+    size_t text_size;                    /* .. and its size. */
>> > >+    void *rw_addr;                       /* Virtual address of .data. */
>> > >+    size_t rw_size;                      /* .. and its size (if any). */
>> > >+    void *ro_addr;                       /* Virtual address of .rodata. */
>> > >+    size_t ro_size;                      /* .. and its size (if any). */
>> > 
>> > And again the question: Do these pointers really need to be non-const?
>> 
>> I know I tried making them const and the compiler was not happy. I will
>> follow up with the reasoning.
> 
> The big problem I ran in was that I had to do casting in
> 'modify_payload'. To lower the amount of casting I ended making
> 
> load_addr be void * (if it was const void * I would have to cast away
> the constness in xsplice_elf.c):
> 
>  elf->sec[i].load_addr = (void *)buf + offset[i];
> 
> where 'buf' is:
>   buf = payload->text_addr;
> 
> (and buf it also a const void *).

Hence (taking all of the above) without any cast and with load_addr
being const void *, text_addr could be so, too. Or else the picture
above isn't the complete one.

>> > >+int xsplice_elf_resolve_symbols(struct xsplice_elf *elf)
>> .. snip..
>> > >+        default:
> .. snip..
>> > >+            if ( !(elf->sec[idx].sec->sh_flags & SHF_ALLOC) )
>> > >+                break;
>> > 
>> > If you really mean to check this, shouldn't this be done earlier, avoiding 
> needless
>> > errors on unsupported symbol kinds above?

With all of the ..snip..-s above, it's now really hard to answer this:

> So I am coming back to this. The ones that this hits are:
> Symbol's  section .note.GNU-stack is !SHF_ALLOC
> Symbol's  section .comment is !SHF_ALLOC
> Symbol's  section .debug_aranges is !SHF_ALLOC
> Symbol's  section .debug_pubnames is !SHF_ALLOC
> Symbol's  section .debug_info is !SHF_ALLOC
> Symbol's  section .debug_abbrev is !SHF_ALLOC
> Symbol's  section .debug_line is !SHF_ALLOC
> Symbol's  section .debug_frame is !SHF_ALLOC
> Symbol's  section .debug_str is !SHF_ALLOC
> Symbol's  section .debug_loc is !SHF_ALLOC
> Symbol's  section .debug_pubtypes is !SHF_ALLOC
> 
> I am not sure what to make out of it. As in the code ignores
> these sections.

Right, because these sections are of no interest to the loading
process. And hence none of the symbols (if there really are any)
in those sections should be of any interest either.

> I tried to ake the test code strip these out:
> 
> strip -d 
> 
> But that stripped out the relocation entries out to!

Was that all relocations, or just those referencing the sections
above?

> Doing just:
> 
> strip -R .comment
> 
> Did the same exact thing (ripped out .rela*).

That sounds wrong.

> Keep in mind that this code above does not set 'rc' at all - so
> the rc is 0 by default and we just ignore this specific section.
> 
> I would rather keep it this way - otherwise I have to make the
> test code go through an hand-rolled linker script -  I can't use
> Xen's (xen.lds.S) as it has the ASSERTS that get triggered:
> 
> #ifdef CONFIG_KEXEC
> ASSERT(kexec_reloc_size - kexec_reloc <= PAGE_SIZE, "kexec_reloc is too
> large")
> #endif
> 
> As the object file has none of these entries in it.
> 
> What's your preference? Leave it as is (so skip over those
> sections), or be pedantic on them and require no !SHF_ALLOC sections?

I'm not sure I'm with you here: I didn't ask for anything to be removed
or added, all I had asked is whether that check shouldn't be moved up.

> And also bail out if any of the sections don't have an st_type that we
> implement, such as .comment:
> 
>  [28] .comment          PROGBITS 0000000000000000  000002a4
>        000000000000005a  0000000000000001  MS       0     0     1
> 
> Or let them be but not do anything to them (whcih is what we currently
> do).

I don't think I can follow you here either.

Jan

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