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

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

On April 21, 2016 11:36:24 AM EDT, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>> On 21.04.16 at 17:15, <konrad@xxxxxxxxxx> 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.
>

If I did that I will need to uncast constness when memcpy data in (a couple of 
lines below in move_payload). And also when doing relocation.

>>> > >+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:

Sorry.
>
>> 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.
>

OK, so we are in agreement.
>> 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?
>

All. For .text as well.

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

Yes. Maybe the version I have is buggy.

>
>> 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.

Oh. Yes, I can do that!

>
>> 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.

This is a bit of tangent on this above .Let me reply on proper thread.
>
>Jan



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