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

From: Konrad Rzeszutek Wilk <konrad@kernel.org>
To: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: sstabellini@kernel.org, keir@xen.org, andrew.cooper3@citrix.com,
	mpohlack@amazon.com, ross.lagerwall@citrix.com,
	julien.grall@arm.com, Jan Beulich <jbeulich@suse.com>,
	sasha.levin@oracle.com, xen-devel@lists.xenproject.org
Subject: Re: [PATCH v8.1 11/27] xsplice: Implement payload loading
Date: Thu, 21 Apr 2016 11:15:25 -0400	[thread overview]
Message-ID: <20160421151525.GA29709@localhost.localdomain> (raw)
In-Reply-To: <20160420155913.GA29455@localhost.localdomain>

On Wed, Apr 20, 2016 at 11:59:34AM -0400, Konrad Rzeszutek Wilk wrote:
> > >+void arch_xsplice_free_payload(void *va)
> > >+{
> > >+    vfree_xen(va);
> > >+}
> > 
> > What is the idea behind having this hook (instead of generic code just calling
> > vfree_xen() [or really just vfree()])?
> 
> To have an symmetry with the allocation one. I don't know enough about
> ARM to know whether this logic above can be hoisted in the common code
> and hence called (or compiled) on ARM.
> 
> Let me try.

That worked out nicely. vzalloc_xen and vfree and the
arch_xsplice_alloc_payload and arch_xsplice_free_payload are gone.

> > 
> > >@@ -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 *).
> > 
> > >+static void calc_section(struct xsplice_elf_sec *sec, size_t *size)
> > >+{
> > >+    Elf_Shdr *s = sec->sec;
> > >+    size_t align_size;
> > >+
> > >+    align_size = ROUNDUP(*size, s->sh_addralign);
> > >+    s->sh_entsize = align_size;
> > 
> > So this is one of the places (the only one?) where the section header gets
> > altered. Are you not expecting problems down the road resulting from
> > overwriting this field? After all it's used not just in control sections...
> 
> The 'man elf' tells me :
> "Some  sections  hold a table of fixed-sized entries, such as a symbol
> table.  For such a section, this member gives the size in bytes for each entry.
>  This member contains zero if the section does not hold  a  table  of 
> fixed-size entries."
> 
> We may have an value for an payload with one symbol but we don't depend
> on this value having any value and just re-use.
> 
> We could change the logic to save the aligned size (which would then alter
> the amount of pages to allocate along with the amount of bytes to copy)
> in some 'per-section' temporary variable (so adding an extra field in
> 'struct xsplice_elf_sec').
> 
> Let me prototype that.

And it worked out nicely.

.. snip..
> > >+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?
> 

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. I tried to ake the test code strip these out:

strip -d 

But that stripped out the relocation entries out to!
Doing just:

strip -R .comment

Did the same exact thing (ripped out .rela*).

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?

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

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