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

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

> >+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.
> 
> >@@ -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.
> 
> >+ size_t pages; /* Total pages for [text,rw,ro]_addr */
> 
> Why size_t and not just unsigned int?

Oh. I was somehow under the impression you liked size_t more than
unsignged int! I will change it over.
> 
> >+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.
> 
> >+static int move_payload(struct payload *payload, struct xsplice_elf *elf)
> >+{
..snip..

> >+    for ( i = 1; i < elf->hdr->e_shnum; i++ )
> >+    {
> >+        if ( !(elf->sec[i].sec->sh_flags & SHF_ALLOC) &&
> >+             (elf->sec[i].sec->sh_flags & SHF_EXECINSTR) &&
> >+             (elf->sec[i].sec->sh_flags & SHF_WRITE) )
> >+        {
> >+            dprintk(XENLOG_ERR, XSPLICE "%s: No WX sections!\n", elf->name);
> >+            return -EINVAL;
> >+        }
> 
> Is there any reason to have four loops here, with quite a bit of redundancy in
> the if()s, instead of just one loop with a if/else sequence?

There was a historical reason - we wre re-using 'size' but that is no
longer the case.
> 
> Also it's not really clear whether you really mean to honor non-progbits, non-
> nobits sections with SHF_ALLOC set. Perhaps such would better be refused
> for the now at least.

OK. 
> 
> >+    for ( i = 1; i < elf->hdr->e_shnum; i++ )
> >+    {
> >+        if ( elf->sec[i].sec->sh_flags & SHF_ALLOC )
> >+        {
> >+            if ( (elf->sec[i].sec->sh_flags & SHF_EXECINSTR) )
> >+                 buf = payload->text_addr;
> >+            else if ( (elf->sec[i].sec->sh_flags & SHF_WRITE) )
> >+                buf = payload->rw_addr;
> >+             else
> 
> Something's wrong with indentation here (not visible above anymore due to
> the limitations of this web frontend of our mail system).
> 
> >+            /* Don't copy NOBITS - such as BSS. */
> >+            if ( elf->sec[i].sec->sh_type != SHT_NOBITS )
> >+            {
> >+                memcpy(elf->sec[i].load_addr, elf->sec[i].data,
> >+                       elf->sec[i].sec->sh_size);
> >+                dprintk(XENLOG_DEBUG, XSPLICE "%s: Loaded %s at 0x%p\n",
> >+                        elf->name, elf->sec[i].name, elf->sec[i].load_addr);
> >+            }
> 
> "else memset();" is what I would have expected here. Now I see that the
> allocation function clears the pages (in a bogusly open coded way, instead
> of using vzalloc()), but why is that so?

B/c we end up having vzalloc_xen (oh wait, we made that go away). Yes
we do need an memset or introduce vzalloc_xen (and keep vmalloc_xen?).

Your call - memset or introduce vzalloc_xen ?
> 
> >+int xsplice_elf_resolve_symbols(struct xsplice_elf *elf)
.. snip..
> >+        default:
> >+            /* SHN_COMMON and SHN_ABS are above. */
> >+            if ( idx > SHN_LORESERVE )
> 
> >=
> 
> >+                rc = -EOPNOTSUPP;
> >+            /* SHN_UNDEF (0) above. */
> >+            else if ( idx > elf->hdr->e_shnum && idx < SHN_LORESERVE )
> 
> >= and the right side of the && seems pointless due to the preceding if().
> 
> >+            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?

Right! Will move those  checks right above the switch statement.

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