[v6,09/24] xsplice: Implement payload loading

Commit Message

Konrad Rzeszutek Wilk April 7, 2016, 3:49 a.m. UTC

From: Ross Lagerwall <[e-mail]>

Add support for loading xsplice payloads. This is somewhat similar to
the Linux kernel module loader, implementing the following steps:
- Verify the elf file.
- Parse the elf file.
- Allocate a region of memory mapped within a free area of
  [xen_virt_end, XEN_VIRT_END].
- Copy allocated sections into the new region. Split them in three
  regions - .text, .data, and .rodata. MUST have at least .text.
- Resolve section symbols. All other symbols must be absolute addresses.
  (Note that patch titled "xsplice,symbols: Implement symbol name resolution
   on address" implements that)
- Perform relocations.
- Secure the the regions (.text,.data,.rodata) with proper permissions.

We capitalize on the vmalloc callback API (see patch titled:
"vmap: Add vmalloc_cb and vfree_cb") to allocate a region
of memory within the [xen_virt_end, XEN_VIRT_END] for the code.

Signed-off-by: Ross Lagerwall <[e-mail]>
Signed-off-by: Konrad Rzeszutek Wilk <[e-mail]>

---
Cc: Stefano Stabellini <[e-mail]>
Cc: Julien Grall <[e-mail]>
Cc: Keir Fraser <[e-mail]>
Cc: Jan Beulich <[e-mail]>
Cc: Andrew Cooper <[e-mail]>

v2: - Change the 'xsplice_patch_func' structure layout/size.
    - Add more error checking. Fix memory leak.
    - Move elf_resolve and elf_perform relocs in elf file.
    - Print the payload address and pages in keyhandler.
v3:
    - Make it build under ARM
    - Build it without using the return_ macro.
    - Add fixes from Ross.
    - Add the _return macro back - but only use it during debug builds.
    - Remove the macro, prefix arch_ on arch specific calls.
v4:
    - Move alloc_payload to arch specific file.
    - Use void* instead of uint8_t, use const
    - Add copyrights
    - Unroll the vmap code to add ASSERT. Change while to not incur
      potential long error loop
   - Use vmalloc/vfree cb APIs
   - Secure .text pages to be RX instead of RWX.
v5:
  - Fix allocation of virtual addresses only allowing one page to be allocated.
  - Create .text, .data, and .rodata regions with different permissions.
  - Make the find_space_t not a typedef to pointer to a function.
  - Allocate memory in here.
v6: Drop parentheses on typedefs.
  - s/an xSplice/a xSplice/
  - Rebase on "vmap: Add vmalloc_cb"
  - Rebase on "vmap: Add vmalloc_type and vm_init_type"
  - s/uint8_t/void/ on load_addr
  - Set xsplice_elf on stack without using memset.
---
---
 xen/arch/arm/Makefile         |   1 +
 xen/arch/arm/xsplice.c        |  55 ++++++++++
 xen/arch/x86/Makefile         |   1 +
 xen/arch/x86/xsplice.c        | 199 +++++++++++++++++++++++++++++++++++
 xen/common/xsplice.c          | 238 +++++++++++++++++++++++++++++++++++++++++-
 xen/common/xsplice_elf.c      | 118 +++++++++++++++++++++
 xen/include/xen/xsplice.h     |  30 ++++++
 xen/include/xen/xsplice_elf.h |   5 +
 8 files changed, 644 insertions(+), 3 deletions(-)
 create mode 100644 xen/arch/arm/xsplice.c
 create mode 100644 xen/arch/x86/xsplice.c

Comments

Andrew Cooper April 8, 2016, 3:31 p.m. UTC | #1

On 07/04/16 04:49, Konrad Rzeszutek Wilk wrote:
> diff --git a/xen/arch/x86/xsplice.c b/xen/arch/x86/xsplice.c
> new file mode 100644
> index 0000000..cadf1f1
> --- /dev/null
> +++ b/xen/arch/x86/xsplice.c
> @@ -0,0 +1,199 @@
> +/*
> + * Copyright (C) 2016 Citrix Systems R&D Ltd.
> + */
> +
> +#include <xen/errno.h>
> +#include <xen/lib.h>
> +#include <xen/mm.h>
> +#include <xen/pfn.h>
> +#include <xen/vmap.h>
> +#include <xen/xsplice_elf.h>
> +#include <xen/xsplice.h>
> +
> +int arch_xsplice_verify_elf(const struct xsplice_elf *elf)
> +{
> +
> +    const Elf_Ehdr *hdr = elf->hdr;
> +
> +    if ( hdr->e_machine != EM_X86_64 )
> +    {
> +        printk(XENLOG_ERR XSPLICE "%s: Invalid ELF payload!\n", elf->name);

Would be nicer as "Unsupported ELF Machine type %u".

> +        return -EOPNOTSUPP;
> +    }
> +
> +    return 0;
> +}
> +
> +int arch_xsplice_perform_rel(struct xsplice_elf *elf,
> +                             const struct xsplice_elf_sec *base,
> +                             const struct xsplice_elf_sec *rela)
> +{
> +    dprintk(XENLOG_ERR, XSPLICE "%s: SHT_REL relocation unsupported\n",
> +            elf->name);
> +    return -ENOSYS;
> +}
> +
> +int arch_xsplice_perform_rela(struct xsplice_elf *elf,
> +                              const struct xsplice_elf_sec *base,
> +                              const struct xsplice_elf_sec *rela)
> +{
> +    const Elf_RelA *r;
> +    unsigned int symndx, i;
> +    uint64_t val;
> +    uint8_t *dest;
> +
> +    if ( !rela->sec->sh_entsize || !rela->sec->sh_size ||
> +         rela->sec->sh_entsize != sizeof(Elf_RelA) )
> +    {
> +        dprintk(XENLOG_DEBUG, XSPLICE "%s: Section relative header is corrupted!\n",
> +                elf->name);

XENLOG_ERR surely? (and the other examples).

> +/*
> + * Once the resolving symbols, performing relocations, etc is complete
> + * we secure the memory by putting in the proper page table attributes
> + * for the desired type.
> + */
> +int arch_xsplice_secure(void *va, unsigned int pages, enum va_type type,
> +                        const mfn_t *mfn)
> +{
> +    unsigned long cur;
> +    unsigned long start = (unsigned long)va;
> +    int flag;
> +
> +    ASSERT(va);
> +    ASSERT(pages);
> +
> +    if ( type == XSPLICE_VA_RX )
> +        flag = PAGE_HYPERVISOR_RX;
> +    else if ( type == XSPLICE_VA_RW )
> +        flag = PAGE_HYPERVISOR_RW;
> +    else
> +        flag = PAGE_HYPERVISOR_RO;
> +
> +    /*
> +     * We could walk the pagetable and do the pagetable manipulations
> +     * (strip the _PAGE_RW), which would mean also not needing the mfn
> +     * array, but there are no generic code for this yet (TODO).
> +     *
> +     * For right now tear down the pagetables and recreate them.
> +     */
> +    destroy_xen_mappings(start, start + pages * PAGE_SIZE);
> +
> +    for ( cur = start; pages--; ++mfn, cur += PAGE_SIZE )
> +    {
> +        if ( map_pages_to_xen(cur, mfn_x(*mfn), 1, flag) )
> +        {
> +            if ( cur != start )
> +                destroy_xen_mappings(start, cur);
> +            return -EINVAL;
> +        }
> +    }

:) Much nicer than before.

> +
> +    return 0;
> +}
> +
> +void arch_xsplice_free_payload(void *va)
> +{
> +    vfree_type(va, VMAP_XEN);
> +}
> +
> +void arch_xsplice_init(void)
> +{
> +    void *start, *end;
> +
> +    start = (void *)xen_virt_end;
> +    end = (void *)(XEN_VIRT_END - NR_CPUS * PAGE_SIZE);

Another TODO for the future.  Make a constant to cover the VA space
occupied by the per-cpu stubs.

> @@ -276,6 +374,26 @@ static int xsplice_header_check(const struct xsplice_elf *elf)
>          return -EOPNOTSUPP;
>      }
>  
> +    if ( !IS_ELF(*hdr) )
> +    {
> +        printk(XENLOG_ERR XSPLICE "%s: Not an ELF payload!\n", elf->name);
> +        return -EINVAL;
> +    }
> +
> +    if ( hdr->e_ident[EI_CLASS] != ELFCLASS64 ||
> +         hdr->e_ident[EI_DATA] != ELFDATA2LSB ||
> +         hdr->e_ident[EI_OSABI] != ELFOSABI_SYSV ||
> +         hdr->e_type != ET_REL ||
> +         hdr->e_phnum != 0 )
> +    {
> +        printk(XENLOG_ERR XSPLICE "%s: Invalid ELF payload!\n", elf->name);
> +        return -EOPNOTSUPP;
> +    }

This hunk up to this point is a rebasing error over the previous patch. 
These two checks are currently duplicated.  (Clearly making doubly sure
it is a valid ELF payload ;p).

With these minor bits fixed, Reviewed-by: Andrew Cooper
<[e-mail]>

Julien Grall April 8, 2016, 3:35 p.m. UTC | #2

Hi Konrad,

On 07/04/16 04:49, Konrad Rzeszutek Wilk wrote:
> From: Ross Lagerwall <[e-mail]>
>
> Add support for loading xsplice payloads. This is somewhat similar to
> the Linux kernel module loader, implementing the following steps:
> - Verify the elf file.
> - Parse the elf file.
> - Allocate a region of memory mapped within a free area of
>    [xen_virt_end, XEN_VIRT_END].
> - Copy allocated sections into the new region. Split them in three
>    regions - .text, .data, and .rodata. MUST have at least .text.
> - Resolve section symbols. All other symbols must be absolute addresses.
>    (Note that patch titled "xsplice,symbols: Implement symbol name resolution
>     on address" implements that)
> - Perform relocations.
> - Secure the the regions (.text,.data,.rodata) with proper permissions.
>
> We capitalize on the vmalloc callback API (see patch titled:
> "vmap: Add vmalloc_cb and vfree_cb") to allocate a region
> of memory within the [xen_virt_end, XEN_VIRT_END] for the code.
>
> Signed-off-by: Ross Lagerwall <[e-mail]>
> Signed-off-by: Konrad Rzeszutek Wilk <[e-mail]>

For the ARM bits:

Acked-by: Julien Grall <[e-mail]>

Regards,

Konrad Rzeszutek Wilk April 8, 2016, 9:10 p.m. UTC | #3

> > +int arch_xsplice_perform_rela(struct xsplice_elf *elf,
> > +                              const struct xsplice_elf_sec *base,
> > +                              const struct xsplice_elf_sec *rela)
> > +{
> > +    const Elf_RelA *r;
> > +    unsigned int symndx, i;
> > +    uint64_t val;
> > +    uint8_t *dest;
> > +
> > +    if ( !rela->sec->sh_entsize || !rela->sec->sh_size ||
> > +         rela->sec->sh_entsize != sizeof(Elf_RelA) )
> > +    {
> > +        dprintk(XENLOG_DEBUG, XSPLICE "%s: Section relative header is corrupted!\n",
> > +                elf->name);
> 
> XENLOG_ERR surely? (and the other examples).

Yes! I modified all of those that return an error. One of them I made
an printk (the one about more than 64 sections).
> 
> > +/*
> > + * Once the resolving symbols, performing relocations, etc is complete
> > + * we secure the memory by putting in the proper page table attributes
> > + * for the desired type.
> > + */
> > +int arch_xsplice_secure(void *va, unsigned int pages, enum va_type type,
> > +                        const mfn_t *mfn)
> > +{
> > +    unsigned long cur;
> > +    unsigned long start = (unsigned long)va;
> > +    int flag;
> > +
> > +    ASSERT(va);
> > +    ASSERT(pages);
> > +
> > +    if ( type == XSPLICE_VA_RX )
> > +        flag = PAGE_HYPERVISOR_RX;
> > +    else if ( type == XSPLICE_VA_RW )
> > +        flag = PAGE_HYPERVISOR_RW;
> > +    else
> > +        flag = PAGE_HYPERVISOR_RO;
> > +
> > +    /*
> > +     * We could walk the pagetable and do the pagetable manipulations
> > +     * (strip the _PAGE_RW), which would mean also not needing the mfn
> > +     * array, but there are no generic code for this yet (TODO).
> > +     *
> > +     * For right now tear down the pagetables and recreate them.
> > +     */
> > +    destroy_xen_mappings(start, start + pages * PAGE_SIZE);
> > +
> > +    for ( cur = start; pages--; ++mfn, cur += PAGE_SIZE )
> > +    {
> > +        if ( map_pages_to_xen(cur, mfn_x(*mfn), 1, flag) )
> > +        {
> > +            if ( cur != start )
> > +                destroy_xen_mappings(start, cur);
> > +            return -EINVAL;
> > +        }
> > +    }
> 
> :) Much nicer than before.
> 
> > +
> > +    return 0;
> > +}
> > +
> > +void arch_xsplice_free_payload(void *va)
> > +{
> > +    vfree_type(va, VMAP_XEN);
> > +}
> > +
> > +void arch_xsplice_init(void)
> > +{
> > +    void *start, *end;
> > +
> > +    start = (void *)xen_virt_end;
> > +    end = (void *)(XEN_VIRT_END - NR_CPUS * PAGE_SIZE);
> 
> Another TODO for the future.  Make a constant to cover the VA space
> occupied by the per-cpu stubs.

Wrote it down in my TODO. Thanks.
> 
> > @@ -276,6 +374,26 @@ static int xsplice_header_check(const struct xsplice_elf *elf)
> >          return -EOPNOTSUPP;
> >      }
> >  
> > +    if ( !IS_ELF(*hdr) )
> > +    {
> > +        printk(XENLOG_ERR XSPLICE "%s: Not an ELF payload!\n", elf->name);
> > +        return -EINVAL;
> > +    }
> > +
> > +    if ( hdr->e_ident[EI_CLASS] != ELFCLASS64 ||
> > +         hdr->e_ident[EI_DATA] != ELFDATA2LSB ||
> > +         hdr->e_ident[EI_OSABI] != ELFOSABI_SYSV ||
> > +         hdr->e_type != ET_REL ||
> > +         hdr->e_phnum != 0 )
> > +    {
> > +        printk(XENLOG_ERR XSPLICE "%s: Invalid ELF payload!\n", elf->name);
> > +        return -EOPNOTSUPP;
> > +    }
> 
> This hunk up to this point is a rebasing error over the previous patch. 
> These two checks are currently duplicated.  (Clearly making doubly sure
> it is a valid ELF payload ;p).

Eww. Thanks for noticing!
> 
> With these minor bits fixed, Reviewed-by: Andrew Cooper
> <[e-mail]>

Jan Beulich April 8, 2016, 9:18 p.m. UTC | #4

>>> On 08.04.16 at 23:10, <[e-mail]> wrote:
>> > +int arch_xsplice_perform_rela(struct xsplice_elf *elf,
>> > +                              const struct xsplice_elf_sec *base,
>> > +                              const struct xsplice_elf_sec *rela)
>> > +{
>> > +    const Elf_RelA *r;
>> > +    unsigned int symndx, i;
>> > +    uint64_t val;
>> > +    uint8_t *dest;
>> > +
>> > +    if ( !rela->sec->sh_entsize || !rela->sec->sh_size ||
>> > +         rela->sec->sh_entsize != sizeof(Elf_RelA) )
>> > +    {
>> > +        dprintk(XENLOG_DEBUG, XSPLICE "%s: Section relative header is corrupted!\n",
>> > +                elf->name);
>> 
>> XENLOG_ERR surely? (and the other examples).
> 
> Yes! I modified all of those that return an error. One of them I made
> an printk (the one about more than 64 sections).

Why would that be any worse than other check failures? I think
these log messages should all be issued consistently.

Jan

Konrad Rzeszutek Wilk April 8, 2016, 10:45 p.m. UTC | #5

On Fri, Apr 08, 2016 at 03:18:09PM -0600, Jan Beulich wrote:
> >>> On 08.04.16 at 23:10, <[e-mail]> wrote:
> >> > +int arch_xsplice_perform_rela(struct xsplice_elf *elf,
> >> > +                              const struct xsplice_elf_sec *base,
> >> > +                              const struct xsplice_elf_sec *rela)
> >> > +{
> >> > +    const Elf_RelA *r;
> >> > +    unsigned int symndx, i;
> >> > +    uint64_t val;
> >> > +    uint8_t *dest;
> >> > +
> >> > +    if ( !rela->sec->sh_entsize || !rela->sec->sh_size ||
> >> > +         rela->sec->sh_entsize != sizeof(Elf_RelA) )
> >> > +    {
> >> > +        dprintk(XENLOG_DEBUG, XSPLICE "%s: Section relative header is corrupted!\n",
> >> > +                elf->name);
> >> 
> >> XENLOG_ERR surely? (and the other examples).
> > 
> > Yes! I modified all of those that return an error. One of them I made
> > an printk (the one about more than 64 sections).
> 
> Why would that be any worse than other check failures? I think
> these log messages should all be issued consistently.

OK, so all be printk instead of dprintk?

> 
> Jan
>

Jan Beulich April 8, 2016, 10:50 p.m. UTC | #6

>>> On 09.04.16 at 00:45, <[e-mail]> wrote:
> On Fri, Apr 08, 2016 at 03:18:09PM -0600, Jan Beulich wrote:
>> >>> On 08.04.16 at 23:10, <[e-mail]> wrote:
>> >> > +int arch_xsplice_perform_rela(struct xsplice_elf *elf,
>> >> > +                              const struct xsplice_elf_sec *base,
>> >> > +                              const struct xsplice_elf_sec *rela)
>> >> > +{
>> >> > +    const Elf_RelA *r;
>> >> > +    unsigned int symndx, i;
>> >> > +    uint64_t val;
>> >> > +    uint8_t *dest;
>> >> > +
>> >> > +    if ( !rela->sec->sh_entsize || !rela->sec->sh_size ||
>> >> > +         rela->sec->sh_entsize != sizeof(Elf_RelA) )
>> >> > +    {
>> >> > +        dprintk(XENLOG_DEBUG, XSPLICE "%s: Section relative header is corrupted!\n",
>> >> > +                elf->name);
>> >> 
>> >> XENLOG_ERR surely? (and the other examples).
>> > 
>> > Yes! I modified all of those that return an error. One of them I made
>> > an printk (the one about more than 64 sections).
>> 
>> Why would that be any worse than other check failures? I think
>> these log messages should all be issued consistently.
> 
> OK, so all be printk instead of dprintk?

Rather the other way around I would say.

Jan

Konrad Rzeszutek Wilk April 9, 2016, 12:37 a.m. UTC | #7

On Fri, Apr 08, 2016 at 04:50:10PM -0600, Jan Beulich wrote:
> >>> On 09.04.16 at 00:45, <[e-mail]> wrote:
> > On Fri, Apr 08, 2016 at 03:18:09PM -0600, Jan Beulich wrote:
> >> >>> On 08.04.16 at 23:10, <[e-mail]> wrote:
> >> >> > +int arch_xsplice_perform_rela(struct xsplice_elf *elf,
> >> >> > +                              const struct xsplice_elf_sec *base,
> >> >> > +                              const struct xsplice_elf_sec *rela)
> >> >> > +{
> >> >> > +    const Elf_RelA *r;
> >> >> > +    unsigned int symndx, i;
> >> >> > +    uint64_t val;
> >> >> > +    uint8_t *dest;
> >> >> > +
> >> >> > +    if ( !rela->sec->sh_entsize || !rela->sec->sh_size ||
> >> >> > +         rela->sec->sh_entsize != sizeof(Elf_RelA) )
> >> >> > +    {
> >> >> > +        dprintk(XENLOG_DEBUG, XSPLICE "%s: Section relative header is corrupted!\n",
> >> >> > +                elf->name);
> >> >> 
> >> >> XENLOG_ERR surely? (and the other examples).
> >> > 
> >> > Yes! I modified all of those that return an error. One of them I made
> >> > an printk (the one about more than 64 sections).
> >> 
> >> Why would that be any worse than other check failures? I think
> >> these log messages should all be issued consistently.
> > 
> > OK, so all be printk instead of dprintk?
> 
> Rather the other way around I would say.

Back to dprintk(XENLOG_DEBUG for all of them then.

> 
> Jan
>

Konrad Rzeszutek Wilk April 9, 2016, 11:48 a.m. UTC | #8

On Fri, Apr 08, 2016 at 08:37:45PM -0400, Konrad Rzeszutek Wilk wrote:
> On Fri, Apr 08, 2016 at 04:50:10PM -0600, Jan Beulich wrote:
> > >>> On 09.04.16 at 00:45, <[e-mail]> wrote:
> > > On Fri, Apr 08, 2016 at 03:18:09PM -0600, Jan Beulich wrote:
> > >> >>> On 08.04.16 at 23:10, <[e-mail]> wrote:
> > >> >> > +int arch_xsplice_perform_rela(struct xsplice_elf *elf,
> > >> >> > +                              const struct xsplice_elf_sec *base,
> > >> >> > +                              const struct xsplice_elf_sec *rela)
> > >> >> > +{
> > >> >> > +    const Elf_RelA *r;
> > >> >> > +    unsigned int symndx, i;
> > >> >> > +    uint64_t val;
> > >> >> > +    uint8_t *dest;
> > >> >> > +
> > >> >> > +    if ( !rela->sec->sh_entsize || !rela->sec->sh_size ||
> > >> >> > +         rela->sec->sh_entsize != sizeof(Elf_RelA) )
> > >> >> > +    {
> > >> >> > +        dprintk(XENLOG_DEBUG, XSPLICE "%s: Section relative header is corrupted!\n",
> > >> >> > +                elf->name);
> > >> >> 
> > >> >> XENLOG_ERR surely? (and the other examples).
> > >> > 
> > >> > Yes! I modified all of those that return an error. One of them I made
> > >> > an printk (the one about more than 64 sections).
> > >> 
> > >> Why would that be any worse than other check failures? I think
> > >> these log messages should all be issued consistently.
> > > 
> > > OK, so all be printk instead of dprintk?
> > 
> > Rather the other way around I would say.
> 
> Back to dprintk(XENLOG_DEBUG for all of them then.

The one question I have is - what shall we do in the field?
Where the hypervisor is not built with debug=y and all the
dprintk are gone. Some of these would be beneficial to
the consumer (like the corruptions)?

Jan Beulich April 11, 2016, 3:53 p.m. UTC | #9

>>> On 09.04.16 at 02:37, <[e-mail]> wrote:
> On Fri, Apr 08, 2016 at 04:50:10PM -0600, Jan Beulich wrote:
>> >>> On 09.04.16 at 00:45, <[e-mail]> wrote:
>> > On Fri, Apr 08, 2016 at 03:18:09PM -0600, Jan Beulich wrote:
>> >> >>> On 08.04.16 at 23:10, <[e-mail]> wrote:
>> >> >> > +int arch_xsplice_perform_rela(struct xsplice_elf *elf,
>> >> >> > +                              const struct xsplice_elf_sec *base,
>> >> >> > +                              const struct xsplice_elf_sec *rela)
>> >> >> > +{
>> >> >> > +    const Elf_RelA *r;
>> >> >> > +    unsigned int symndx, i;
>> >> >> > +    uint64_t val;
>> >> >> > +    uint8_t *dest;
>> >> >> > +
>> >> >> > +    if ( !rela->sec->sh_entsize || !rela->sec->sh_size ||
>> >> >> > +         rela->sec->sh_entsize != sizeof(Elf_RelA) )
>> >> >> > +    {
>> >> >> > +        dprintk(XENLOG_DEBUG, XSPLICE "%s: Section relative header is corrupted!\n",
>> >> >> > +                elf->name);
>> >> >> 
>> >> >> XENLOG_ERR surely? (and the other examples).
>> >> > 
>> >> > Yes! I modified all of those that return an error. One of them I made
>> >> > an printk (the one about more than 64 sections).
>> >> 
>> >> Why would that be any worse than other check failures? I think
>> >> these log messages should all be issued consistently.
>> > 
>> > OK, so all be printk instead of dprintk?
>> 
>> Rather the other way around I would say.
> 
> Back to dprintk(XENLOG_DEBUG for all of them then.

I don't see why dprintk() can't be used with log levels other
than debug, as suggested by (I think) Andrew above.

Jan

Konrad Rzeszutek Wilk April 11, 2016, 4:03 p.m. UTC | #10

On Mon, Apr 11, 2016 at 09:53:06AM -0600, Jan Beulich wrote:
> >>> On 09.04.16 at 02:37, <[e-mail]> wrote:
> > On Fri, Apr 08, 2016 at 04:50:10PM -0600, Jan Beulich wrote:
> >> >>> On 09.04.16 at 00:45, <[e-mail]> wrote:
> >> > On Fri, Apr 08, 2016 at 03:18:09PM -0600, Jan Beulich wrote:
> >> >> >>> On 08.04.16 at 23:10, <[e-mail]> wrote:
> >> >> >> > +int arch_xsplice_perform_rela(struct xsplice_elf *elf,
> >> >> >> > +                              const struct xsplice_elf_sec *base,
> >> >> >> > +                              const struct xsplice_elf_sec *rela)
> >> >> >> > +{
> >> >> >> > +    const Elf_RelA *r;
> >> >> >> > +    unsigned int symndx, i;
> >> >> >> > +    uint64_t val;
> >> >> >> > +    uint8_t *dest;
> >> >> >> > +
> >> >> >> > +    if ( !rela->sec->sh_entsize || !rela->sec->sh_size ||
> >> >> >> > +         rela->sec->sh_entsize != sizeof(Elf_RelA) )
> >> >> >> > +    {
> >> >> >> > +        dprintk(XENLOG_DEBUG, XSPLICE "%s: Section relative header is corrupted!\n",
> >> >> >> > +                elf->name);
> >> >> >> 
> >> >> >> XENLOG_ERR surely? (and the other examples).
> >> >> > 
> >> >> > Yes! I modified all of those that return an error. One of them I made
> >> >> > an printk (the one about more than 64 sections).
> >> >> 
> >> >> Why would that be any worse than other check failures? I think
> >> >> these log messages should all be issued consistently.
> >> > 
> >> > OK, so all be printk instead of dprintk?
> >> 
> >> Rather the other way around I would say.
> > 
> > Back to dprintk(XENLOG_DEBUG for all of them then.
> 
> I don't see why dprintk() can't be used with log levels other
> than debug, as suggested by (I think) Andrew above.

OK, let me make them dprintk(XENLOG_ERROR

> 
> Jan
>

Konrad Rzeszutek Wilk April 11, 2016, 4:34 p.m. UTC | #11

On Mon, Apr 11, 2016 at 12:03:49PM -0400, Konrad Rzeszutek Wilk wrote:
> On Mon, Apr 11, 2016 at 09:53:06AM -0600, Jan Beulich wrote:
> > >>> On 09.04.16 at 02:37, <[e-mail]> wrote:
> > > On Fri, Apr 08, 2016 at 04:50:10PM -0600, Jan Beulich wrote:
> > >> >>> On 09.04.16 at 00:45, <[e-mail]> wrote:
> > >> > On Fri, Apr 08, 2016 at 03:18:09PM -0600, Jan Beulich wrote:
> > >> >> >>> On 08.04.16 at 23:10, <[e-mail]> wrote:
> > >> >> >> > +int arch_xsplice_perform_rela(struct xsplice_elf *elf,
> > >> >> >> > +                              const struct xsplice_elf_sec *base,
> > >> >> >> > +                              const struct xsplice_elf_sec *rela)
> > >> >> >> > +{
> > >> >> >> > +    const Elf_RelA *r;
> > >> >> >> > +    unsigned int symndx, i;
> > >> >> >> > +    uint64_t val;
> > >> >> >> > +    uint8_t *dest;
> > >> >> >> > +
> > >> >> >> > +    if ( !rela->sec->sh_entsize || !rela->sec->sh_size ||
> > >> >> >> > +         rela->sec->sh_entsize != sizeof(Elf_RelA) )
> > >> >> >> > +    {
> > >> >> >> > +        dprintk(XENLOG_DEBUG, XSPLICE "%s: Section relative header is corrupted!\n",
> > >> >> >> > +                elf->name);
> > >> >> >> 
> > >> >> >> XENLOG_ERR surely? (and the other examples).
> > >> >> > 
> > >> >> > Yes! I modified all of those that return an error. One of them I made
> > >> >> > an printk (the one about more than 64 sections).
> > >> >> 
> > >> >> Why would that be any worse than other check failures? I think
> > >> >> these log messages should all be issued consistently.
> > >> > 
> > >> > OK, so all be printk instead of dprintk?
> > >> 
> > >> Rather the other way around I would say.
> > > 
> > > Back to dprintk(XENLOG_DEBUG for all of them then.
> > 
> > I don't see why dprintk() can't be used with log levels other
> > than debug, as suggested by (I think) Andrew above.
> 
> OK, let me make them dprintk(XENLOG_ERROR

I've pretty much modified most of them to that, the exceptions are these:

+                dprintk(XENLOG_DEBUG, XSPLICE "%s: Loaded %s at 0x%p\n",
+            dprintk(XENLOG_DEBUG, XSPLICE "%s: Resolved old address %s => %p\n",
+                dprintk(XENLOG_DEBUG, XSPLICE "%s: Already loaded as %s!\n",
+            dprintk(XENLOG_DEBUG, XSPLICE "%s: new symbol %s\n",
+            dprintk(XENLOG_DEBUG, XSPLICE "%s: overriding symbol %s\n",
+    dprintk(XENLOG_DEBUG, XSPLICE "%s: Applying %u functions.\n",
+    dprintk(XENLOG_DEBUG, XSPLICE "%s: Reverting.\n", data->name);
+    dprintk(XENLOG_DEBUG, XSPLICE "%s: timeout is %"PRI_stime"ms\n",
+            dprintk(XENLOG_DEBUG, XSPLICE "%s: CPU%u - IPIing the other %u CPUs\n",
+            dprintk(XENLOG_DEBUG, XSPLICE "%s: Undefined symbol resolved: %s => %#"PRIxElfAddr"\n",
+            dprintk(XENLOG_DEBUG, XSPLICE "%s: Absolute symbol: %s => %#"PRIxElfAddr"\n",
+                dprintk(XENLOG_DEBUG, XSPLICE "%s: Symbol resolved: %s => %#"PRIxElfAddr"(%s)\n",

And then these are printk variants:


+                printk(XENLOG_ERR XSPLICE "%s: Overflow in relocation %u in %s for %s!\n",
+            printk(XENLOG_ERR XSPLICE "%s: Unhandled relocation %lu\n",
+        printk(XENLOG_ERR XSPLICE "%s: Could not allocate memory for payload!\n",
+            printk(XENLOG_ERR XSPLICE "%s: %s is missing!\n",
+            printk(XENLOG_ERR XSPLICE "%s: %s is empty!\n",
+            printk(XENLOG_ERR XSPLICE "%s: %s was seen more than once!\n",
+                    printk(XENLOG_ERR XSPLICE "%s: Could not resolve old address of %s\n",
+                printk(XENLOG_ERR XSPLICE "%s: duplicate new symbol: %s\n",
+        printk(XENLOG_ERR XSPLICE "%s: unable to get cpu_maps lock!\n",
+        printk(XENLOG_ERR XSPLICE "%s: Timed out on %s semaphore %u/%u\n",
+            printk(XENLOG_ERR XSPLICE "%s: CPU%u - unable to get cpu_maps lock!\n",
+        printk(XENLOG_INFO XSPLICE "%s finished %s with rc=%d\n",
+        printk(XENLOG_INFO XSPLICE ": build-id: %*phN\n", len, binary_id);
+        printk(XENLOG_ERR XSPLICE"%s: Could not allocate memory for section table!\n",
+        printk(XENLOG_ERR XSPLICE "%s: Could not allocate memory for symbols\n",
+            printk(XENLOG_ERR XSPLICE "%s: Unexpected common symbol: %s\n",
+                    printk(XENLOG_ERR XSPLICE "%s: Unknown symbol: %s\n",

We can change some of those to dprintk if folks want that.

Jan Beulich April 11, 2016, 4:55 p.m. UTC | #12

>>> On 11.04.16 at 18:34, <[e-mail]> wrote:
> On Mon, Apr 11, 2016 at 12:03:49PM -0400, Konrad Rzeszutek Wilk wrote:
>> On Mon, Apr 11, 2016 at 09:53:06AM -0600, Jan Beulich wrote:
>> > >>> On 09.04.16 at 02:37, <[e-mail]> wrote:
>> > > On Fri, Apr 08, 2016 at 04:50:10PM -0600, Jan Beulich wrote:
>> > >> >>> On 09.04.16 at 00:45, <[e-mail]> wrote:
>> > >> > On Fri, Apr 08, 2016 at 03:18:09PM -0600, Jan Beulich wrote:
>> > >> >> >>> On 08.04.16 at 23:10, <[e-mail]> wrote:
>> > >> >> >> > +int arch_xsplice_perform_rela(struct xsplice_elf *elf,
>> > >> >> >> > +                              const struct xsplice_elf_sec *base,
>> > >> >> >> > +                              const struct xsplice_elf_sec *rela)
>> > >> >> >> > +{
>> > >> >> >> > +    const Elf_RelA *r;
>> > >> >> >> > +    unsigned int symndx, i;
>> > >> >> >> > +    uint64_t val;
>> > >> >> >> > +    uint8_t *dest;
>> > >> >> >> > +
>> > >> >> >> > +    if ( !rela->sec->sh_entsize || !rela->sec->sh_size ||
>> > >> >> >> > +         rela->sec->sh_entsize != sizeof(Elf_RelA) )
>> > >> >> >> > +    {
>> > >> >> >> > +        dprintk(XENLOG_DEBUG, XSPLICE "%s: Section relative header is 
> corrupted!\n",
>> > >> >> >> > +                elf->name);
>> > >> >> >> 
>> > >> >> >> XENLOG_ERR surely? (and the other examples).
>> > >> >> > 
>> > >> >> > Yes! I modified all of those that return an error. One of them I made
>> > >> >> > an printk (the one about more than 64 sections).
>> > >> >> 
>> > >> >> Why would that be any worse than other check failures? I think
>> > >> >> these log messages should all be issued consistently.
>> > >> > 
>> > >> > OK, so all be printk instead of dprintk?
>> > >> 
>> > >> Rather the other way around I would say.
>> > > 
>> > > Back to dprintk(XENLOG_DEBUG for all of them then.
>> > 
>> > I don't see why dprintk() can't be used with log levels other
>> > than debug, as suggested by (I think) Andrew above.
>> 
>> OK, let me make them dprintk(XENLOG_ERROR
> 
> I've pretty much modified most of them to that, the exceptions are these:
> 
> +                dprintk(XENLOG_DEBUG, XSPLICE "%s: Loaded %s at 0x%p\n",
> +            dprintk(XENLOG_DEBUG, XSPLICE "%s: Resolved old address %s => %p\n",
> +                dprintk(XENLOG_DEBUG, XSPLICE "%s: Already loaded as %s!\n",
> +            dprintk(XENLOG_DEBUG, XSPLICE "%s: new symbol %s\n",
> +            dprintk(XENLOG_DEBUG, XSPLICE "%s: overriding symbol %s\n",
> +    dprintk(XENLOG_DEBUG, XSPLICE "%s: Applying %u functions.\n",
> +    dprintk(XENLOG_DEBUG, XSPLICE "%s: Reverting.\n", data->name);
> +    dprintk(XENLOG_DEBUG, XSPLICE "%s: timeout is %"PRI_stime"ms\n",
> +            dprintk(XENLOG_DEBUG, XSPLICE "%s: CPU%u - IPIing the other %u CPUs\n",
> +            dprintk(XENLOG_DEBUG, XSPLICE "%s: Undefined symbol resolved: %s => %#"PRIxElfAddr"\n",
> +            dprintk(XENLOG_DEBUG, XSPLICE "%s: Absolute symbol: %s => %#"PRIxElfAddr"\n",
> +                dprintk(XENLOG_DEBUG, XSPLICE "%s: Symbol resolved: %s => %#"PRIxElfAddr"(%s)\n",
> 
> And then these are printk variants:
> 
> 
> +                printk(XENLOG_ERR XSPLICE "%s: Overflow in relocation %u in %s for %s!\n",
> +            printk(XENLOG_ERR XSPLICE "%s: Unhandled relocation %lu\n",
> +        printk(XENLOG_ERR XSPLICE "%s: Could not allocate memory for payload!\n",
> +            printk(XENLOG_ERR XSPLICE "%s: %s is missing!\n",
> +            printk(XENLOG_ERR XSPLICE "%s: %s is empty!\n",
> +            printk(XENLOG_ERR XSPLICE "%s: %s was seen more than once!\n",
> +                    printk(XENLOG_ERR XSPLICE "%s: Could not resolve old address of %s\n",
> +                printk(XENLOG_ERR XSPLICE "%s: duplicate new symbol: %s\n",
> +        printk(XENLOG_ERR XSPLICE "%s: unable to get cpu_maps lock!\n",
> +        printk(XENLOG_ERR XSPLICE "%s: Timed out on %s semaphore %u/%u\n",
> +            printk(XENLOG_ERR XSPLICE "%s: CPU%u - unable to get cpu_maps lock!\n",
> +        printk(XENLOG_INFO XSPLICE "%s finished %s with rc=%d\n",
> +        printk(XENLOG_INFO XSPLICE ": build-id: %*phN\n", len, binary_id);
> +        printk(XENLOG_ERR XSPLICE"%s: Could not allocate memory for section table!\n",
> +        printk(XENLOG_ERR XSPLICE "%s: Could not allocate memory for symbols\n",
> +            printk(XENLOG_ERR XSPLICE "%s: Unexpected common symbol: %s\n",
> +                    printk(XENLOG_ERR XSPLICE "%s: Unknown symbol: %s\n",
> 
> We can change some of those to dprintk if folks want that.

So as mentioned before I'd again like to ask for consistency: I
cannot really see the criteria by which some of these use dprintk()
vs printk(). The main aspect here is: If things go severely wrong,
will the console be spammed? And the second one: Which of these
are really useful in the field?

Jan

Konrad Rzeszutek Wilk April 11, 2016, 5:08 p.m. UTC | #13

On Mon, Apr 11, 2016 at 10:55:38AM -0600, Jan Beulich wrote:
> >>> On 11.04.16 at 18:34, <[e-mail]> wrote:
> > On Mon, Apr 11, 2016 at 12:03:49PM -0400, Konrad Rzeszutek Wilk wrote:
> >> On Mon, Apr 11, 2016 at 09:53:06AM -0600, Jan Beulich wrote:
> >> > >>> On 09.04.16 at 02:37, <[e-mail]> wrote:
> >> > > On Fri, Apr 08, 2016 at 04:50:10PM -0600, Jan Beulich wrote:
> >> > >> >>> On 09.04.16 at 00:45, <[e-mail]> wrote:
> >> > >> > On Fri, Apr 08, 2016 at 03:18:09PM -0600, Jan Beulich wrote:
> >> > >> >> >>> On 08.04.16 at 23:10, <[e-mail]> wrote:
> >> > >> >> >> > +int arch_xsplice_perform_rela(struct xsplice_elf *elf,
> >> > >> >> >> > +                              const struct xsplice_elf_sec *base,
> >> > >> >> >> > +                              const struct xsplice_elf_sec *rela)
> >> > >> >> >> > +{
> >> > >> >> >> > +    const Elf_RelA *r;
> >> > >> >> >> > +    unsigned int symndx, i;
> >> > >> >> >> > +    uint64_t val;
> >> > >> >> >> > +    uint8_t *dest;
> >> > >> >> >> > +
> >> > >> >> >> > +    if ( !rela->sec->sh_entsize || !rela->sec->sh_size ||
> >> > >> >> >> > +         rela->sec->sh_entsize != sizeof(Elf_RelA) )
> >> > >> >> >> > +    {
> >> > >> >> >> > +        dprintk(XENLOG_DEBUG, XSPLICE "%s: Section relative header is 
> > corrupted!\n",
> >> > >> >> >> > +                elf->name);
> >> > >> >> >> 
> >> > >> >> >> XENLOG_ERR surely? (and the other examples).
> >> > >> >> > 
> >> > >> >> > Yes! I modified all of those that return an error. One of them I made
> >> > >> >> > an printk (the one about more than 64 sections).
> >> > >> >> 
> >> > >> >> Why would that be any worse than other check failures? I think
> >> > >> >> these log messages should all be issued consistently.
> >> > >> > 
> >> > >> > OK, so all be printk instead of dprintk?
> >> > >> 
> >> > >> Rather the other way around I would say.
> >> > > 
> >> > > Back to dprintk(XENLOG_DEBUG for all of them then.
> >> > 
> >> > I don't see why dprintk() can't be used with log levels other
> >> > than debug, as suggested by (I think) Andrew above.
> >> 
> >> OK, let me make them dprintk(XENLOG_ERROR
> > 
> > I've pretty much modified most of them to that, the exceptions are these:
> > 
> > +                dprintk(XENLOG_DEBUG, XSPLICE "%s: Loaded %s at 0x%p\n",
> > +            dprintk(XENLOG_DEBUG, XSPLICE "%s: Resolved old address %s => %p\n",
> > +                dprintk(XENLOG_DEBUG, XSPLICE "%s: Already loaded as %s!\n",
> > +            dprintk(XENLOG_DEBUG, XSPLICE "%s: new symbol %s\n",
> > +            dprintk(XENLOG_DEBUG, XSPLICE "%s: overriding symbol %s\n",
> > +    dprintk(XENLOG_DEBUG, XSPLICE "%s: Applying %u functions.\n",
> > +    dprintk(XENLOG_DEBUG, XSPLICE "%s: Reverting.\n", data->name);
> > +    dprintk(XENLOG_DEBUG, XSPLICE "%s: timeout is %"PRI_stime"ms\n",
> > +            dprintk(XENLOG_DEBUG, XSPLICE "%s: CPU%u - IPIing the other %u CPUs\n",
> > +            dprintk(XENLOG_DEBUG, XSPLICE "%s: Undefined symbol resolved: %s => %#"PRIxElfAddr"\n",
> > +            dprintk(XENLOG_DEBUG, XSPLICE "%s: Absolute symbol: %s => %#"PRIxElfAddr"\n",
> > +                dprintk(XENLOG_DEBUG, XSPLICE "%s: Symbol resolved: %s => %#"PRIxElfAddr"(%s)\n",
> > 
> > And then these are printk variants:
> > 
> > 
> > +                printk(XENLOG_ERR XSPLICE "%s: Overflow in relocation %u in %s for %s!\n",
> > +            printk(XENLOG_ERR XSPLICE "%s: Unhandled relocation %lu\n",

> > +        printk(XENLOG_ERR XSPLICE "%s: Could not allocate memory for payload!\n",
> > +            printk(XENLOG_ERR XSPLICE "%s: %s is missing!\n",
> > +            printk(XENLOG_ERR XSPLICE "%s: %s is empty!\n",
> > +            printk(XENLOG_ERR XSPLICE "%s: %s was seen more than once!\n",
> > +                    printk(XENLOG_ERR XSPLICE "%s: Could not resolve old address of %s\n",
> > +                printk(XENLOG_ERR XSPLICE "%s: duplicate new symbol: %s\n",
> > +        printk(XENLOG_ERR XSPLICE "%s: unable to get cpu_maps lock!\n",
> > +        printk(XENLOG_ERR XSPLICE "%s: Timed out on %s semaphore %u/%u\n",
> > +            printk(XENLOG_ERR XSPLICE "%s: CPU%u - unable to get cpu_maps lock!\n",
> > +        printk(XENLOG_INFO XSPLICE "%s finished %s with rc=%d\n",
> > +        printk(XENLOG_INFO XSPLICE ": build-id: %*phN\n", len, binary_id);
> > +        printk(XENLOG_ERR XSPLICE"%s: Could not allocate memory for section table!\n",
> > +        printk(XENLOG_ERR XSPLICE "%s: Could not allocate memory for symbols\n",
> > +            printk(XENLOG_ERR XSPLICE "%s: Unexpected common symbol: %s\n",
> > +                    printk(XENLOG_ERR XSPLICE "%s: Unknown symbol: %s\n",
> > 
> > We can change some of those to dprintk if folks want that.
> 
> So as mentioned before I'd again like to ask for consistency: I
> cannot really see the criteria by which some of these use dprintk()
> vs printk(). The main aspect here is: If things go severely wrong,
> will the console be spammed? And the second one: Which of these

If the system admin continously tried to unload and load the patchset
then we certainly would spam.

But the 'loading' is (or ought to) be a single event. The applying
or reverting may be done more often.

As such I would say that the operations that are tied to apply/reverting
should go through printk - to at least leave breadcrumbs if things
fall apart. I would say:

        printk(XENLOG_ERR XSPLICE "%s: unable to get cpu_maps lock!\n",
        printk(XENLOG_ERR XSPLICE "%s: Timed out on %s semaphore %u/%u\n",
            printk(XENLOG_ERR XSPLICE "%s: CPU%u - unable to get cpu_maps lock!\n",
        printk(XENLOG_INFO XSPLICE "%s finished %s with rc=%d\n",
        printk(XENLOG_INFO XSPLICE ": build-id: %*phN\n", len, binary_id);
    dprintk(XENLOG_DEBUG, XSPLICE "%s: Applying %u functions.\n",
    dprintk(XENLOG_DEBUG, XSPLICE "%s: Reverting.\n", data->name);
    dprintk(XENLOG_DEBUG, XSPLICE "%s: timeout is %"PRI_stime"ms\n",
            dprintk(XENLOG_DEBUG, XSPLICE "%s: CPU%u - IPIing the other %u CPUs\n",

Should be come printk. And make them INFO (except on errors - they should be ERR).


Then comes the question of payloads loading. In the fields all
the dprintk are gone - and that is exactly where the payloads would
be used. And that is the only _way_ to actually test the payload. But
if you don't have dprintk and something goes wrong you only get -EINVAL.

As such I would think that all of the dprintk that deal with the payload
should be made printk. So these:

+        dprintk(XENLOG_ERR, XSPLICE "%s: Unsupported ELF Machine type!\n",
+    dprintk(XENLOG_ERR, XSPLICE "%s: SHT_REL relocation unsupported\n",
+        dprintk(XENLOG_ERR, XSPLICE "%s: Section relative header is corrupted!\n",
+            dprintk(XENLOG_ERR, XSPLICE "%s: Relative entry %u in %s is past end!\n",
+            dprintk(XENLOG_ERR, XSPLICE "%s: Relative symbol wants symbol@%u which is past end!\n",
+            dprintk(XENLOG_ERR, XSPLICE "%s: No WX sections!\n", elf->name);
+                dprintk(XENLOG_DEBUG, XSPLICE "%s: Loaded %s at 0x%p\n",
+        dprintk(XENLOG_ERR, XSPLICE "%s: Wrong size of .xsplice.funcs!\n",
+            dprintk(XENLOG_ERR, XSPLICE "%s: Wrong version (%u). Expected %d!\n",
+            dprintk(XENLOG_ERR, XSPLICE "%s: Address or size fields are zero!\n",
+            dprintk(XENLOG_DEBUG, XSPLICE "%s: Resolved old address %s => %p\n",
+                dprintk(XENLOG_DEBUG, XSPLICE "%s: Already loaded as %s!\n",
+            dprintk(XENLOG_ERR, XSPLICE "%s: Wrong size of .bug_frames.%u!\n",
+            dprintk(XENLOG_ERR, XSPLICE "%s: Wrong size of .alt_instr (exp:%lu vs %lu)!\n",
+                dprintk(XENLOG_ERR, XSPLICE "%s Alt patching outside payload: 0x%lx!\n",
+            dprintk(XENLOG_ERR, XSPLICE "%s: Wrong size of .ex_table (exp:%lu vs %lu)!\n",
+            dprintk(XENLOG_DEBUG, XSPLICE "%s: new symbol %s\n",
+            dprintk(XENLOG_DEBUG, XSPLICE "%s: overriding symbol %s\n",
+        dprintk(XENLOG_ERR, "%s%s: check against %s build-id failed!\n",
+                dprintk(XENLOG_ERR, "%s%s: can't unload. Top is %s!\n",
+            dprintk(XENLOG_ERR, XSPLICE "%s: Section table is past end of payload!\n",
+            dprintk(XENLOG_ERR, XSPLICE "%s: Section [%u] data is past end of payload!\n",
+                dprintk(XENLOG_ERR, XSPLICE "%s: Unsupported multiple symbol tables!\n",
+        dprintk(XENLOG_ERR, XSPLICE "%s: No symbol table found!\n",
+        dprintk(XENLOG_ERR, XSPLICE "%s: Symbol table header is corrupted!\n",
+        dprintk(XENLOG_ERR, XSPLICE "%s: String table section is corrupted\n",
+        dprintk(XENLOG_ERR, XSPLICE "%s: Section string table is corrupted\n",
+            dprintk(XENLOG_ERR, XSPLICE "%s: shstrtab [%u] data is past end of payload!\n",
+            dprintk(XENLOG_ERR, XSPLICE "%s: Symbol [%u] data is past end of payload!\n",
+                dprintk(XENLOG_ERR, XSPLICE "%s: Unknown type=%#"PRIx16"\n",
+            dprintk(XENLOG_ERR, XSPLICE "%s: Relative link of %s is incorrect (%d, expected=%d)\n",
+        dprintk(XENLOG_ERR, XSPLICE "%s: Section header is bigger than payload!\n",
+        dprintk(XENLOG_ERR, XSPLICE "%s: Not an ELF payload!\n", elf->name);
+        dprintk(XENLOG_ERR, XSPLICE "%s: Invalid ELF payload!\n", elf->name);
+        dprintk(XENLOG_ERR, XSPLICE "%s: Section name idx is undefined!?\n",
+        dprintk(XENLOG_ERR, XSPLICE "%s: Section name idx (%u) is past end of sections (%u)!\n",
+        dprintk(XENLOG_ERR, XSPLICE "%s: Too many (%u) sections!\n",
+        dprintk(XENLOG_ERR, XSPLICE "%s: Bogus e_shoff!\n", elf->name);
+        dprintk(XENLOG_ERR, XSPLICE "%s: Section header size is %u! Expected %zu!?\n",

Should be printk.

Which would leave almost no dprintks in the code.

but some of them are chatty.

 +                dprintk(XENLOG_DEBUG, XSPLICE "%s: Loaded %s at 0x%p\n",
 +            dprintk(XENLOG_DEBUG, XSPLICE "%s: Resolved old address %s => %p\n",
 +            dprintk(XENLOG_DEBUG, XSPLICE "%s: new symbol %s\n",
 +            dprintk(XENLOG_DEBUG, XSPLICE "%s: overriding symbol %s\n",
 +                dprintk(XENLOG_DEBUG, XSPLICE "%s: Symbol resolved: %s => %#"PRIxElfAddr"(%s)\n",
+            dprintk(XENLOG_DEBUG, XSPLICE "%s: Undefined symbol resolved: %s => %#"PRIxElfAddr"\n",
+            dprintk(XENLOG_DEBUG, XSPLICE "%s: Absolute symbol: %s => %#"PRIxElfAddr"\n",

and those could certainly be printk(XENLOG_DEBUG' perhaps? Or leave them as dprintk?

Jan Beulich April 11, 2016, 5:26 p.m. UTC | #14

>>> On 11.04.16 at 19:08, <[e-mail]> wrote:
> If the system admin continously tried to unload and load the patchset
> then we certainly would spam.
> 
> But the 'loading' is (or ought to) be a single event. The applying
> or reverting may be done more often.
> 
> As such I would say that the operations that are tied to apply/reverting
> should go through printk - to at least leave breadcrumbs if things
> fall apart. I would say:
> 
>         printk(XENLOG_ERR XSPLICE "%s: unable to get cpu_maps lock!\n",
>         printk(XENLOG_ERR XSPLICE "%s: Timed out on %s semaphore %u/%u\n",
>             printk(XENLOG_ERR XSPLICE "%s: CPU%u - unable to get cpu_maps lock!\n",
>         printk(XENLOG_INFO XSPLICE "%s finished %s with rc=%d\n",
>         printk(XENLOG_INFO XSPLICE ": build-id: %*phN\n", len, binary_id);
>     dprintk(XENLOG_DEBUG, XSPLICE "%s: Applying %u functions.\n",
>     dprintk(XENLOG_DEBUG, XSPLICE "%s: Reverting.\n", data->name);
>     dprintk(XENLOG_DEBUG, XSPLICE "%s: timeout is %"PRI_stime"ms\n",
>             dprintk(XENLOG_DEBUG, XSPLICE "%s: CPU%u - IPIing the other %u CPUs\n",
> 
> Should be come printk. And make them INFO (except on errors - they should be 
> ERR).

Especially for the last one I don't see what use this has outside of
debugging activities. For the others a primary question is: Can any
of these occur more than once for a single operation (hypercall)?

> Then comes the question of payloads loading. In the fields all
> the dprintk are gone - and that is exactly where the payloads would
> be used. And that is the only _way_ to actually test the payload. But
> if you don't have dprintk and something goes wrong you only get -EINVAL.
> 
> As such I would think that all of the dprintk that deal with the payload
> should be made printk. So these:
> 
> +        dprintk(XENLOG_ERR, XSPLICE "%s: Unsupported ELF Machine type!\n",
> +    dprintk(XENLOG_ERR, XSPLICE "%s: SHT_REL relocation unsupported\n",
> +        dprintk(XENLOG_ERR, XSPLICE "%s: Section relative header is corrupted!\n",
> +            dprintk(XENLOG_ERR, XSPLICE "%s: Relative entry %u in %s is past end!\n",
> +            dprintk(XENLOG_ERR, XSPLICE "%s: Relative symbol wants symbol@%u which is past end!\n",
> +            dprintk(XENLOG_ERR, XSPLICE "%s: No WX sections!\n", elf->name);
> +                dprintk(XENLOG_DEBUG, XSPLICE "%s: Loaded %s at 0x%p\n",
> +        dprintk(XENLOG_ERR, XSPLICE "%s: Wrong size of .xsplice.funcs!\n",
> +            dprintk(XENLOG_ERR, XSPLICE "%s: Wrong version (%u). Expected  %d!\n",
> +            dprintk(XENLOG_ERR, XSPLICE "%s: Address or size fields are  zero!\n",
> +            dprintk(XENLOG_DEBUG, XSPLICE "%s: Resolved old address %s => %p\n",
> +                dprintk(XENLOG_DEBUG, XSPLICE "%s: Already loaded as %s!\n",
> +            dprintk(XENLOG_ERR, XSPLICE "%s: Wrong size of .bug_frames.%u!\n",
> +            dprintk(XENLOG_ERR, XSPLICE "%s: Wrong size of .alt_instr (exp:%lu vs %lu)!\n",
> +                dprintk(XENLOG_ERR, XSPLICE "%s Alt patching outside payload: 0x%lx!\n",
> +            dprintk(XENLOG_ERR, XSPLICE "%s: Wrong size of .ex_table (exp:%lu vs %lu)!\n",
> +            dprintk(XENLOG_DEBUG, XSPLICE "%s: new symbol %s\n",
> +            dprintk(XENLOG_DEBUG, XSPLICE "%s: overriding symbol %s\n",
> +        dprintk(XENLOG_ERR, "%s%s: check against %s build-id failed!\n",
> +                dprintk(XENLOG_ERR, "%s%s: can't unload. Top is %s!\n",
> +            dprintk(XENLOG_ERR, XSPLICE "%s: Section table is past end of payload!\n",
> +            dprintk(XENLOG_ERR, XSPLICE "%s: Section [%u] data is past end of payload!\n",
> +                dprintk(XENLOG_ERR, XSPLICE "%s: Unsupported multiple symbol tables!\n",
> +        dprintk(XENLOG_ERR, XSPLICE "%s: No symbol table found!\n",
> +        dprintk(XENLOG_ERR, XSPLICE "%s: Symbol table header is corrupted!\n",
> +        dprintk(XENLOG_ERR, XSPLICE "%s: String table section is corrupted\n",
> +        dprintk(XENLOG_ERR, XSPLICE "%s: Section string table is corrupted\n",
> +            dprintk(XENLOG_ERR, XSPLICE "%s: shstrtab [%u] data is past end of payload!\n",
> +            dprintk(XENLOG_ERR, XSPLICE "%s: Symbol [%u] data is past end of payload!\n",
> +                dprintk(XENLOG_ERR, XSPLICE "%s: Unknown type=%#"PRIx16"\n",
> +            dprintk(XENLOG_ERR, XSPLICE "%s: Relative link of %s is incorrect (%d, expected=%d)\n",
> +        dprintk(XENLOG_ERR, XSPLICE "%s: Section header is bigger than payload!\n",
> +        dprintk(XENLOG_ERR, XSPLICE "%s: Not an ELF payload!\n", elf->name);
> +        dprintk(XENLOG_ERR, XSPLICE "%s: Invalid ELF payload!\n", elf->name);
> +        dprintk(XENLOG_ERR, XSPLICE "%s: Section name idx is undefined!?\n",
> +        dprintk(XENLOG_ERR, XSPLICE "%s: Section name idx (%u) is past end of sections (%u)!\n",
> +        dprintk(XENLOG_ERR, XSPLICE "%s: Too many (%u) sections!\n",
> +        dprintk(XENLOG_ERR, XSPLICE "%s: Bogus e_shoff!\n", elf->name);
> +        dprintk(XENLOG_ERR, XSPLICE "%s: Section header size is %u! Expected %zu!?\n",
> 
> Should be printk.

I disagree - issues with the payload image should be diagnosed using
user space tools. I.e. I'd rather question whether many of the above
shouldn't go away altogether.

> Which would leave almost no dprintks in the code.
> 
> but some of them are chatty.
> 
>  +                dprintk(XENLOG_DEBUG, XSPLICE "%s: Loaded %s at 0x%p\n",
>  +            dprintk(XENLOG_DEBUG, XSPLICE "%s: Resolved old address %s => %p\n",
>  +            dprintk(XENLOG_DEBUG, XSPLICE "%s: new symbol %s\n",
>  +            dprintk(XENLOG_DEBUG, XSPLICE "%s: overriding symbol %s\n",
>  +                dprintk(XENLOG_DEBUG, XSPLICE "%s: Symbol resolved: %s => %#"PRIxElfAddr"(%s)\n",
> +            dprintk(XENLOG_DEBUG, XSPLICE "%s: Undefined symbol resolved: %s => %#"PRIxElfAddr"\n",
> +            dprintk(XENLOG_DEBUG, XSPLICE "%s: Absolute symbol: %s => %#"PRIxElfAddr"\n",
> 
> and those could certainly be printk(XENLOG_DEBUG' perhaps? Or leave them as 
> dprintk?

Afaic - leave as many dprintk() as possible.

Jan

Konrad Rzeszutek Wilk April 11, 2016, 6:21 p.m. UTC | #15

On Mon, Apr 11, 2016 at 11:26:05AM -0600, Jan Beulich wrote:
> >>> On 11.04.16 at 19:08, <[e-mail]> wrote:
> > If the system admin continously tried to unload and load the patchset
> > then we certainly would spam.
> > 
> > But the 'loading' is (or ought to) be a single event. The applying
> > or reverting may be done more often.
> > 
> > As such I would say that the operations that are tied to apply/reverting
> > should go through printk - to at least leave breadcrumbs if things
> > fall apart. I would say:
> > 
> >         printk(XENLOG_ERR XSPLICE "%s: unable to get cpu_maps lock!\n",
> >         printk(XENLOG_ERR XSPLICE "%s: Timed out on %s semaphore %u/%u\n",
> >             printk(XENLOG_ERR XSPLICE "%s: CPU%u - unable to get cpu_maps lock!\n",
> >         printk(XENLOG_INFO XSPLICE "%s finished %s with rc=%d\n",
> >         printk(XENLOG_INFO XSPLICE ": build-id: %*phN\n", len, binary_id);
> >     dprintk(XENLOG_DEBUG, XSPLICE "%s: Applying %u functions.\n",
> >     dprintk(XENLOG_DEBUG, XSPLICE "%s: Reverting.\n", data->name);
> >     dprintk(XENLOG_DEBUG, XSPLICE "%s: timeout is %"PRI_stime"ms\n",
> >             dprintk(XENLOG_DEBUG, XSPLICE "%s: CPU%u - IPIing the other %u CPUs\n",
> > 
> > Should be come printk. And make them INFO (except on errors - they should be 
> > ERR).
> 
> Especially for the last one I don't see what use this has outside of
> debugging activities. For the others a primary question is: Can any

True, last one is very much debug.

> of these occur more than once for a single operation (hypercall)?

The apply/replace hypercall can 

     dprintk(XENLOG_DEBUG, XSPLICE "%s: timeout is %"PRI_stime"ms\n",
     dprintk(XENLOG_DEBUG, XSPLICE "%s: Applying %u functions.\n",
         printk(XENLOG_INFO XSPLICE "%s finished %s with rc=%d\n",

The replace can trigger a lot of "Reverting".. And one "Applying"

The uploading can trigger tons of them if payload is buggy.

The 'get' and 'list' are silent.
> 
> > Then comes the question of payloads loading. In the fields all
> > the dprintk are gone - and that is exactly where the payloads would
> > be used. And that is the only _way_ to actually test the payload. But
> > if you don't have dprintk and something goes wrong you only get -EINVAL.
> > 
> > As such I would think that all of the dprintk that deal with the payload
> > should be made printk. So these:
> > 
> > +        dprintk(XENLOG_ERR, XSPLICE "%s: Unsupported ELF Machine type!\n",
> > +    dprintk(XENLOG_ERR, XSPLICE "%s: SHT_REL relocation unsupported\n",
> > +        dprintk(XENLOG_ERR, XSPLICE "%s: Section relative header is corrupted!\n",
> > +            dprintk(XENLOG_ERR, XSPLICE "%s: Relative entry %u in %s is past end!\n",
> > +            dprintk(XENLOG_ERR, XSPLICE "%s: Relative symbol wants symbol@%u which is past end!\n",
> > +            dprintk(XENLOG_ERR, XSPLICE "%s: No WX sections!\n", elf->name);
> > +                dprintk(XENLOG_DEBUG, XSPLICE "%s: Loaded %s at 0x%p\n",
> > +        dprintk(XENLOG_ERR, XSPLICE "%s: Wrong size of .xsplice.funcs!\n",
> > +            dprintk(XENLOG_ERR, XSPLICE "%s: Wrong version (%u). Expected  %d!\n",
> > +            dprintk(XENLOG_ERR, XSPLICE "%s: Address or size fields are  zero!\n",
> > +            dprintk(XENLOG_DEBUG, XSPLICE "%s: Resolved old address %s => %p\n",
> > +                dprintk(XENLOG_DEBUG, XSPLICE "%s: Already loaded as %s!\n",
> > +            dprintk(XENLOG_ERR, XSPLICE "%s: Wrong size of .bug_frames.%u!\n",
> > +            dprintk(XENLOG_ERR, XSPLICE "%s: Wrong size of .alt_instr (exp:%lu vs %lu)!\n",
> > +                dprintk(XENLOG_ERR, XSPLICE "%s Alt patching outside payload: 0x%lx!\n",
> > +            dprintk(XENLOG_ERR, XSPLICE "%s: Wrong size of .ex_table (exp:%lu vs %lu)!\n",
> > +            dprintk(XENLOG_DEBUG, XSPLICE "%s: new symbol %s\n",
> > +            dprintk(XENLOG_DEBUG, XSPLICE "%s: overriding symbol %s\n",
> > +        dprintk(XENLOG_ERR, "%s%s: check against %s build-id failed!\n",
> > +                dprintk(XENLOG_ERR, "%s%s: can't unload. Top is %s!\n",
> > +            dprintk(XENLOG_ERR, XSPLICE "%s: Section table is past end of payload!\n",
> > +            dprintk(XENLOG_ERR, XSPLICE "%s: Section [%u] data is past end of payload!\n",
> > +                dprintk(XENLOG_ERR, XSPLICE "%s: Unsupported multiple symbol tables!\n",
> > +        dprintk(XENLOG_ERR, XSPLICE "%s: No symbol table found!\n",
> > +        dprintk(XENLOG_ERR, XSPLICE "%s: Symbol table header is corrupted!\n",
> > +        dprintk(XENLOG_ERR, XSPLICE "%s: String table section is corrupted\n",
> > +        dprintk(XENLOG_ERR, XSPLICE "%s: Section string table is corrupted\n",
> > +            dprintk(XENLOG_ERR, XSPLICE "%s: shstrtab [%u] data is past end of payload!\n",
> > +            dprintk(XENLOG_ERR, XSPLICE "%s: Symbol [%u] data is past end of payload!\n",
> > +                dprintk(XENLOG_ERR, XSPLICE "%s: Unknown type=%#"PRIx16"\n",
> > +            dprintk(XENLOG_ERR, XSPLICE "%s: Relative link of %s is incorrect (%d, expected=%d)\n",
> > +        dprintk(XENLOG_ERR, XSPLICE "%s: Section header is bigger than payload!\n",
> > +        dprintk(XENLOG_ERR, XSPLICE "%s: Not an ELF payload!\n", elf->name);
> > +        dprintk(XENLOG_ERR, XSPLICE "%s: Invalid ELF payload!\n", elf->name);
> > +        dprintk(XENLOG_ERR, XSPLICE "%s: Section name idx is undefined!?\n",
> > +        dprintk(XENLOG_ERR, XSPLICE "%s: Section name idx (%u) is past end of sections (%u)!\n",
> > +        dprintk(XENLOG_ERR, XSPLICE "%s: Too many (%u) sections!\n",
> > +        dprintk(XENLOG_ERR, XSPLICE "%s: Bogus e_shoff!\n", elf->name);
> > +        dprintk(XENLOG_ERR, XSPLICE "%s: Section header size is %u! Expected %zu!?\n",
> > 
> > Should be printk.
> 
> I disagree - issues with the payload image should be diagnosed using
> user space tools. I.e. I'd rather question whether many of the above
> shouldn't go away altogether.

Lets wait with the deletion part. They will be useful when doing
the ARM part.

So all related to 'upload' should be dprintk.
> 
> > Which would leave almost no dprintks in the code.
> > 
> > but some of them are chatty.
> > 
> >  +                dprintk(XENLOG_DEBUG, XSPLICE "%s: Loaded %s at 0x%p\n",
> >  +            dprintk(XENLOG_DEBUG, XSPLICE "%s: Resolved old address %s => %p\n",
> >  +            dprintk(XENLOG_DEBUG, XSPLICE "%s: new symbol %s\n",
> >  +            dprintk(XENLOG_DEBUG, XSPLICE "%s: overriding symbol %s\n",
> >  +                dprintk(XENLOG_DEBUG, XSPLICE "%s: Symbol resolved: %s => %#"PRIxElfAddr"(%s)\n",
> > +            dprintk(XENLOG_DEBUG, XSPLICE "%s: Undefined symbol resolved: %s => %#"PRIxElfAddr"\n",
> > +            dprintk(XENLOG_DEBUG, XSPLICE "%s: Absolute symbol: %s => %#"PRIxElfAddr"\n",
> > 
> > and those could certainly be printk(XENLOG_DEBUG' perhaps? Or leave them as 
> > dprintk?
> 
> Afaic - leave as many dprintk() as possible.

OK, dprintk it is.

> 
> Jan

Konrad Rzeszutek Wilk April 11, 2016, 6:57 p.m. UTC | #16

On Mon, Apr 11, 2016 at 02:21:55PM -0400, Konrad Rzeszutek Wilk wrote:
> On Mon, Apr 11, 2016 at 11:26:05AM -0600, Jan Beulich wrote:
> > >>> On 11.04.16 at 19:08, <[e-mail]> wrote:
> > > If the system admin continously tried to unload and load the patchset
> > > then we certainly would spam.
> > > 
> > > But the 'loading' is (or ought to) be a single event. The applying
> > > or reverting may be done more often.
> > > 
> > > As such I would say that the operations that are tied to apply/reverting
> > > should go through printk - to at least leave breadcrumbs if things
> > > fall apart. I would say:
> > > 
> > >         printk(XENLOG_ERR XSPLICE "%s: unable to get cpu_maps lock!\n",
> > >         printk(XENLOG_ERR XSPLICE "%s: Timed out on %s semaphore %u/%u\n",
> > >             printk(XENLOG_ERR XSPLICE "%s: CPU%u - unable to get cpu_maps lock!\n",
> > >         printk(XENLOG_INFO XSPLICE "%s finished %s with rc=%d\n",
> > >         printk(XENLOG_INFO XSPLICE ": build-id: %*phN\n", len, binary_id);
> > >     dprintk(XENLOG_DEBUG, XSPLICE "%s: Applying %u functions.\n",
> > >     dprintk(XENLOG_DEBUG, XSPLICE "%s: Reverting.\n", data->name);
> > >     dprintk(XENLOG_DEBUG, XSPLICE "%s: timeout is %"PRI_stime"ms\n",
> > >             dprintk(XENLOG_DEBUG, XSPLICE "%s: CPU%u - IPIing the other %u CPUs\n",
> > > 
> > > Should be come printk. And make them INFO (except on errors - they should be 
> > > ERR).
> > 
> > Especially for the last one I don't see what use this has outside of
> > debugging activities. For the others a primary question is: Can any
> 
> True, last one is very much debug.
> 
> > of these occur more than once for a single operation (hypercall)?
> 
> The apply/replace hypercall can 
> 
>      dprintk(XENLOG_DEBUG, XSPLICE "%s: timeout is %"PRI_stime"ms\n",
>      dprintk(XENLOG_DEBUG, XSPLICE "%s: Applying %u functions.\n",
>          printk(XENLOG_INFO XSPLICE "%s finished %s with rc=%d\n",
> 
> The replace can trigger a lot of "Reverting".. And one "Applying"
> 
> The uploading can trigger tons of them if payload is buggy.
> 
> The 'get' and 'list' are silent.

With that in mind the series only has these be printk:

[konrad@char xen]$ git diff origin/staging.. | grep printk | grep -v dprintk | grep XEN > /tmp/z

+    printk(XENLOG_INFO XSPLICE "%s: Applying %u functions.\n",
+    printk(XENLOG_INFO XSPLICE "%s: Reverting.\n", data->name);
+        printk(XENLOG_ERR XSPLICE "%s: unable to get cpu_maps lock!\n",
+        printk(XENLOG_ERR XSPLICE "%s: Timed out on %s semaphore %u/%u\n",
+            printk(XENLOG_ERR XSPLICE "%s: CPU%u - unable to get cpu_maps lock!\n",
+        printk(XENLOG_INFO XSPLICE "%s finished %s with rc=%d\n",
+        printk(XENLOG_INFO XSPLICE ": build-id: %*phN\n", len, binary_id);

while the rest are in dprintk with XENLOG_DEBUG (10), XENLOG_ERR (47) levels.

Patch

diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
index 0328b50..eae5cb3 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -40,6 +40,7 @@  obj-y += device.o
 obj-y += decode.o
 obj-y += processor.o
 obj-y += smc.o
+obj-$(CONFIG_XSPLICE) += xsplice.o
 
 #obj-bin-y += ....o
 
diff --git a/xen/arch/arm/xsplice.c b/xen/arch/arm/xsplice.c
new file mode 100644
index 0000000..2d07415
--- /dev/null
+++ b/xen/arch/arm/xsplice.c
@@ -0,0 +1,55 @@ 
+/*
+ *  Copyright (C) 2016 Citrix Systems R&D Ltd.
+ */
+#include <xen/lib.h>
+#include <xen/errno.h>
+#include <xen/xsplice_elf.h>
+#include <xen/xsplice.h>
+
+int arch_xsplice_verify_elf(const struct xsplice_elf *elf)
+{
+    return -ENOSYS;
+}
+
+int arch_xsplice_perform_rel(struct xsplice_elf *elf,
+                             const struct xsplice_elf_sec *base,
+                             const struct xsplice_elf_sec *rela)
+{
+    return -ENOSYS;
+}
+
+int arch_xsplice_perform_rela(struct xsplice_elf *elf,
+                              const struct xsplice_elf_sec *base,
+                              const struct xsplice_elf_sec *rela)
+{
+    return -ENOSYS;
+}
+
+void *arch_xsplice_alloc_payload(unsigned int pages, mfn_t **mfn)
+{
+    return NULL;
+}
+
+int arch_xsplice_secure(void *va, unsigned int pages, enum va_type type,
+                        const mfn_t *mfn)
+{
+    return -ENOSYS;
+}
+
+void arch_xsplice_free_payload(void *va)
+{
+}
+
+void arch_xsplice_init(void)
+{
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
index 729065b..8a6a7d5 100644
--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -64,6 +64,7 @@  obj-y += vm_event.o
 obj-y += xstate.o
 
 obj-$(crash_debug) += gdbstub.o
+obj-$(CONFIG_XSPLICE) += xsplice.o
 
 x86_emulate.o: x86_emulate/x86_emulate.c x86_emulate/x86_emulate.h
 
diff --git a/xen/arch/x86/xsplice.c b/xen/arch/x86/xsplice.c
new file mode 100644
index 0000000..cadf1f1
--- /dev/null
+++ b/xen/arch/x86/xsplice.c
@@ -0,0 +1,199 @@ 
+/*
+ * Copyright (C) 2016 Citrix Systems R&D Ltd.
+ */
+
+#include <xen/errno.h>
+#include <xen/lib.h>
+#include <xen/mm.h>
+#include <xen/pfn.h>
+#include <xen/vmap.h>
+#include <xen/xsplice_elf.h>
+#include <xen/xsplice.h>
+
+int arch_xsplice_verify_elf(const struct xsplice_elf *elf)
+{
+
+    const Elf_Ehdr *hdr = elf->hdr;
+
+    if ( hdr->e_machine != EM_X86_64 )
+    {
+        printk(XENLOG_ERR XSPLICE "%s: Invalid ELF payload!\n", elf->name);
+        return -EOPNOTSUPP;
+    }
+
+    return 0;
+}
+
+int arch_xsplice_perform_rel(struct xsplice_elf *elf,
+                             const struct xsplice_elf_sec *base,
+                             const struct xsplice_elf_sec *rela)
+{
+    dprintk(XENLOG_ERR, XSPLICE "%s: SHT_REL relocation unsupported\n",
+            elf->name);
+    return -ENOSYS;
+}
+
+int arch_xsplice_perform_rela(struct xsplice_elf *elf,
+                              const struct xsplice_elf_sec *base,
+                              const struct xsplice_elf_sec *rela)
+{
+    const Elf_RelA *r;
+    unsigned int symndx, i;
+    uint64_t val;
+    uint8_t *dest;
+
+    if ( !rela->sec->sh_entsize || !rela->sec->sh_size ||
+         rela->sec->sh_entsize != sizeof(Elf_RelA) )
+    {
+        dprintk(XENLOG_DEBUG, XSPLICE "%s: Section relative header is corrupted!\n",
+                elf->name);
+        return -EINVAL;
+    }
+
+    for ( i = 0; i < (rela->sec->sh_size / rela->sec->sh_entsize); i++ )
+    {
+        r = rela->data + i * rela->sec->sh_entsize;
+        if ( (unsigned long)r > (unsigned long)(elf->hdr + elf->len) )
+        {
+            dprintk(XENLOG_DEBUG, XSPLICE "%s: Relative entry %u in %s is past end!\n",
+                    elf->name, i, rela->name);
+            return -EINVAL;
+        }
+
+        symndx = ELF64_R_SYM(r->r_info);
+        if ( symndx > elf->nsym )
+        {
+            dprintk(XENLOG_DEBUG, XSPLICE "%s: Relative symbol wants symbol@%u which is past end!\n",
+                    elf->name, symndx);
+            return -EINVAL;
+        }
+
+        dest = base->load_addr + r->r_offset;
+        val = r->r_addend + elf->sym[symndx].sym->st_value;
+
+        switch ( ELF64_R_TYPE(r->r_info) )
+        {
+        case R_X86_64_NONE:
+            break;
+
+        case R_X86_64_64:
+            *(uint64_t *)dest = val;
+            break;
+
+        case R_X86_64_PLT32:
+            /*
+             * Xen uses -fpic which normally uses PLT relocations
+             * except that it sets visibility to hidden which means
+             * that they are not used.  However, when gcc cannot
+             * inline memcpy it emits memcpy with default visibility
+             * which then creates a PLT relocation.  It can just be
+             * treated the same as R_X86_64_PC32.
+             */
+            /* Fall through */
+
+        case R_X86_64_PC32:
+            *(uint32_t *)dest = val - (uint64_t)dest;
+            break;
+
+        default:
+            printk(XENLOG_ERR XSPLICE "%s: Unhandled relocation %lu\n",
+                   elf->name, ELF64_R_TYPE(r->r_info));
+            return -EINVAL;
+        }
+    }
+
+    return 0;
+}
+
+/*
+ * The function prepares a xSplice payload by allocating space which
+ * then can be used for loading the allocated sections, resolving symbols,
+ * performing relocations, etc.
+ */
+void *arch_xsplice_alloc_payload(unsigned int pages, mfn_t **mfn)
+{
+    unsigned int i;
+    void *p;
+
+    ASSERT(pages);
+    ASSERT(mfn && !*mfn);
+
+    p = vmalloc_type(pages * PAGE_SIZE, VMAP_XEN, mfn);
+    WARN_ON(!p);
+    if ( p )
+    {
+        for ( i = 0; i < pages; i++ )
+            clear_page(p + (i * PAGE_SIZE) );
+    }
+    return p;
+}
+
+/*
+ * Once the resolving symbols, performing relocations, etc is complete
+ * we secure the memory by putting in the proper page table attributes
+ * for the desired type.
+ */
+int arch_xsplice_secure(void *va, unsigned int pages, enum va_type type,
+                        const mfn_t *mfn)
+{
+    unsigned long cur;
+    unsigned long start = (unsigned long)va;
+    int flag;
+
+    ASSERT(va);
+    ASSERT(pages);
+
+    if ( type == XSPLICE_VA_RX )
+        flag = PAGE_HYPERVISOR_RX;
+    else if ( type == XSPLICE_VA_RW )
+        flag = PAGE_HYPERVISOR_RW;
+    else
+        flag = PAGE_HYPERVISOR_RO;
+
+    /*
+     * We could walk the pagetable and do the pagetable manipulations
+     * (strip the _PAGE_RW), which would mean also not needing the mfn
+     * array, but there are no generic code for this yet (TODO).
+     *
+     * For right now tear down the pagetables and recreate them.
+     */
+    destroy_xen_mappings(start, start + pages * PAGE_SIZE);
+
+    for ( cur = start; pages--; ++mfn, cur += PAGE_SIZE )
+    {
+        if ( map_pages_to_xen(cur, mfn_x(*mfn), 1, flag) )
+        {
+            if ( cur != start )
+                destroy_xen_mappings(start, cur);
+            return -EINVAL;
+        }
+    }
+
+    return 0;
+}
+
+void arch_xsplice_free_payload(void *va)
+{
+    vfree_type(va, VMAP_XEN);
+}
+
+void arch_xsplice_init(void)
+{
+    void *start, *end;
+
+    start = (void *)xen_virt_end;
+    end = (void *)(XEN_VIRT_END - NR_CPUS * PAGE_SIZE);
+
+    BUG_ON(end <= start);
+
+    vm_init_type(VMAP_XEN, start, end);
+}
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/common/xsplice.c b/xen/common/xsplice.c
index 06f4a7b..10c8166 100644
--- a/xen/common/xsplice.c
+++ b/xen/common/xsplice.c
@@ -13,6 +13,7 @@ 
 #include <xen/smp.h>
 #include <xen/spinlock.h>
 #include <xen/vmap.h>
+#include <xen/xsplice_elf.h>
 #include <xen/xsplice.h>
 
 #include <asm/event.h>
@@ -28,6 +29,14 @@  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). */
+    size_t pages;                        /* Total pages for [text,rw,ro]_addr */
+    mfn_t *mfn;                          /* The MFNs backing these pages. */
     char name[XEN_XSPLICE_NAME_SIZE];    /* Name of it. */
 };
 
@@ -85,6 +94,178 @@  static struct payload *find_payload(const char *name)
     return found;
 }
 
+/*
+ * Functions related to XEN_SYSCTL_XSPLICE_UPLOAD (see xsplice_upload), and
+ * freeing payload (XEN_SYSCTL_XSPLICE_ACTION:XSPLICE_ACTION_UNLOAD).
+ */
+
+static void free_payload_data(struct payload *payload)
+{
+    /* Set to zero until "move_payload". */
+    if ( !payload->text_addr )
+        return;
+
+    xfree(payload->mfn);
+    payload->mfn = NULL;
+
+    arch_xsplice_free_payload(payload->text_addr);
+
+    payload->text_addr = NULL;
+    payload->ro_addr = NULL;
+    payload->rw_addr = NULL;
+    payload->pages = 0;
+}
+
+/*
+* calc_section computes the size (taking into account section alignment).
+*
+* It also modifies sh_entsize with the offset of from the start of
+* virtual address space. This is used in move_payload to figure out the
+* destination location.
+*/
+static void calc_section(struct xsplice_elf_sec *sec, size_t *size)
+{
+    Elf_Shdr *s;
+    size_t align_size;
+
+    /* Casting away constness since we modify it. */
+    s = (Elf_Shdr *)sec->sec;
+    align_size = ROUNDUP(*size, s->sh_addralign);
+    s->sh_entsize = align_size;
+
+    *size = s->sh_size + align_size;
+}
+
+static int move_payload(struct payload *payload, struct xsplice_elf *elf)
+{
+    uint8_t *buf;
+    unsigned int i;
+    size_t size = 0;
+
+    /* Compute text regions. */
+    for ( i = 1; i < elf->hdr->e_shnum; i++ )
+    {
+        if ( (elf->sec[i].sec->sh_flags & (SHF_ALLOC|SHF_EXECINSTR)) ==
+             (SHF_ALLOC|SHF_EXECINSTR) )
+            calc_section(&elf->sec[i], &payload->text_size);
+    }
+
+    /* Compute rw data. */
+    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) )
+            calc_section(&elf->sec[i], &payload->rw_size);
+    }
+
+    /* Compute ro data. */
+    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) )
+            calc_section(&elf->sec[i], &payload->ro_size);
+    }
+
+    /* Do not accept wx. */
+    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_DEBUG, XSPLICE "%s: No WX sections!\n", elf->name);
+            return -EINVAL;
+        }
+    }
+
+    /*
+     * Total of all three regions - RX, RW, and RO. We have to have
+     * keep them in seperate pages so we PAGE_ALIGN the RX and RW to have
+     * them on seperate pages. The last one will by default fall on its
+     * own page.
+     */
+    size = PAGE_ALIGN(payload->text_size) + PAGE_ALIGN(payload->rw_size) +
+                      payload->ro_size;
+
+    size = PFN_UP(size);
+    buf = arch_xsplice_alloc_payload(size, &payload->mfn);
+    if ( !buf )
+    {
+        printk(XENLOG_ERR XSPLICE "%s: Could not allocate memory for payload!\n",
+               elf->name);
+        return -ENOMEM;
+    }
+    payload->pages = size;
+    payload->text_addr = buf;
+    payload->rw_addr = payload->text_addr + PAGE_ALIGN(payload->text_size);
+    payload->ro_addr = payload->rw_addr + PAGE_ALIGN(payload->rw_size);
+
+    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
+                buf = payload->ro_addr;
+
+            elf->sec[i].load_addr = buf + elf->sec[i].sec->sh_entsize;
+
+            /* 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);
+            }
+        }
+    }
+
+    return 0;
+}
+
+static int secure_payload(struct payload *payload, struct xsplice_elf *elf)
+{
+    int rc;
+    unsigned int text_pages, rw_pages, ro_pages;
+    mfn_t *mfn = payload->mfn;
+
+    ASSERT(mfn);
+
+    text_pages = PFN_UP(payload->text_size);
+    ASSERT(text_pages);
+
+    rc = arch_xsplice_secure(payload->text_addr, text_pages, XSPLICE_VA_RX,
+                             mfn);
+    if ( rc )
+        return rc;
+
+    rw_pages = PFN_UP(payload->rw_size);
+    if ( rw_pages )
+    {
+        rc = arch_xsplice_secure(payload->rw_addr, rw_pages, XSPLICE_VA_RW,
+                                 mfn + text_pages);
+        if ( rc )
+            return rc;
+    }
+
+    ro_pages = PFN_UP(payload->ro_size);
+    if ( ro_pages )
+    {
+        rc = arch_xsplice_secure(payload->ro_addr, ro_pages, XSPLICE_VA_RO,
+                                 mfn + text_pages + rw_pages);
+    }
+
+    ASSERT(ro_pages + rw_pages + text_pages == payload->pages);
+
+    return rc;
+}
+
 /* We MUST be holding the payload_lock spinlock. */
 static void free_payload(struct payload *data)
 {
@@ -92,13 +273,48 @@  static void free_payload(struct payload *data)
     list_del(&data->list);
     payload_cnt--;
     payload_version++;
+    free_payload_data(data);
     xfree(data);
 }
 
+static int load_payload_data(struct payload *payload, void *raw, size_t len)
+{
+    struct xsplice_elf elf = { .name = payload->name, .len = len };
+    int rc = 0;
+
+    rc = xsplice_elf_load(&elf, raw);
+    if ( rc )
+        goto out;
+
+    rc = move_payload(payload, &elf);
+    if ( rc )
+        goto out;
+
+    rc = xsplice_elf_resolve_symbols(&elf);
+    if ( rc )
+        goto out;
+
+    rc = xsplice_elf_perform_relocs(&elf);
+    if ( rc )
+        goto out;
+
+    rc = secure_payload(payload, &elf);
+
+ out:
+    if ( rc )
+        free_payload_data(payload);
+
+    /* Free our temporary data structure. */
+    xsplice_elf_free(&elf);
+
+    return rc;
+}
+
 static int xsplice_upload(xen_sysctl_xsplice_upload_t *upload)
 {
     struct payload *data = NULL, *found;
     char n[XEN_XSPLICE_NAME_SIZE];
+    void *raw_data = NULL;
     int rc;
 
     rc = verify_payload(upload, n);
@@ -127,9 +343,20 @@  static int xsplice_upload(xen_sysctl_xsplice_upload_t *upload)
         goto out;
     }
 
-    rc = 0;
+    rc = -ENOMEM;
+    raw_data = vmalloc(upload->size);
+    if ( !raw_data )
+        goto out;
+
+    rc = -EFAULT;
+    if ( __copy_from_guest(raw_data, upload->payload, upload->size) )
+        goto out;
 
     memcpy(data->name, n, strlen(n));
+    rc = load_payload_data(data, raw_data, upload->size);
+    if ( rc )
+        goto out;
+
     data->state = XSPLICE_STATE_CHECKED;
     INIT_LIST_HEAD(&data->list);
 
@@ -140,6 +367,8 @@  static int xsplice_upload(xen_sysctl_xsplice_upload_t *upload)
  out:
     spin_unlock(&payload_lock);
 
+    vfree(raw_data);
+
     if ( rc )
         xfree(data);
 
@@ -379,8 +608,9 @@  static void xsplice_printall(unsigned char key)
     }
 
     list_for_each_entry ( data, &payload_list, list )
-        printk(" name=%s state=%s(%d)\n", data->name,
-               state2str(data->state), data->state);
+        printk(" name=%s state=%s(%d) %p (.data=%p, .rodata=%p) using %zu pages.\n",
+               data->name, state2str(data->state), data->state, data->text_addr,
+               data->rw_addr, data->ro_addr, data->pages);
 
     spin_unlock(&payload_lock);
 }
@@ -388,6 +618,8 @@  static void xsplice_printall(unsigned char key)
 static int __init xsplice_init(void)
 {
     register_keyhandler('x', xsplice_printall, "print xsplicing info", 1);
+
+    arch_xsplice_init();
     return 0;
 }
 __initcall(xsplice_init);
diff --git a/xen/common/xsplice_elf.c b/xen/common/xsplice_elf.c
index b244d42..59323b8c 100644
--- a/xen/common/xsplice_elf.c
+++ b/xen/common/xsplice_elf.c
@@ -249,9 +249,107 @@  static int elf_get_sym(struct xsplice_elf *elf, const void *data)
     return 0;
 }
 
+int xsplice_elf_resolve_symbols(struct xsplice_elf *elf)
+{
+    unsigned int i;
+    int rc = 0;
+
+    /*
+     * The first entry of an ELF symbol table is the "undefined symbol index".
+     * aka reserved so we skip it.
+     */
+    ASSERT(elf->sym);
+
+    for ( i = 1; i < elf->nsym; i++ )
+    {
+        uint16_t idx = elf->sym[i].sym->st_shndx;
+
+        rc = 0;
+        switch ( idx )
+        {
+        case SHN_COMMON:
+            printk(XENLOG_ERR XSPLICE "%s: Unexpected common symbol: %s\n",
+                   elf->name, elf->sym[i].name);
+            rc = -EINVAL;
+            break;
+
+        case SHN_UNDEF:
+            printk(XENLOG_ERR XSPLICE "%s: Unknown symbol: %s\n",
+                   elf->name, elf->sym[i].name);
+            rc = -ENOENT;
+            break;
+
+        case SHN_ABS:
+            dprintk(XENLOG_DEBUG, XSPLICE "%s: Absolute symbol: %s => %#"PRIx64"\n",
+                    elf->name, elf->sym[i].name,
+                    (uint64_t)elf->sym[i].sym->st_value);
+            break;
+
+        default:
+            if ( elf->sec[idx].sec->sh_flags & SHF_ALLOC )
+            {
+                elf->sym[i].sym->st_value +=
+                    (unsigned long)elf->sec[idx].load_addr;
+                if ( elf->sym[i].name )
+                    printk(XENLOG_DEBUG XSPLICE "%s: Symbol resolved: %s => #%"PRIx64"(%s)\n",
+                           elf->name, elf->sym[i].name,
+                           (uint64_t)elf->sym[i].sym->st_value,
+                           elf->sec[idx].name);
+            }
+        }
+        if ( rc )
+            break;
+    }
+
+    return rc;
+}
+
+int xsplice_elf_perform_relocs(struct xsplice_elf *elf)
+{
+    struct xsplice_elf_sec *rela, *base;
+    unsigned int i;
+    int rc = 0;
+
+    /*
+     * The first entry of an ELF symbol table is the "undefined symbol index".
+     * aka reserved so we skip it.
+     */
+    ASSERT(elf->sym);
+
+    for ( i = 1; i < elf->hdr->e_shnum; i++ )
+    {
+        rela = &elf->sec[i];
+
+        if ( (rela->sec->sh_type != SHT_RELA ) &&
+             (rela->sec->sh_type != SHT_REL ) )
+            continue;
+
+         /* Is it a valid relocation section? */
+         if ( rela->sec->sh_info >= elf->hdr->e_shnum )
+            continue;
+
+         base = &elf->sec[rela->sec->sh_info];
+
+         /* Don't relocate non-allocated sections. */
+         if ( !(base->sec->sh_flags & SHF_ALLOC) )
+            continue;
+
+        if ( elf->sec[i].sec->sh_type == SHT_RELA )
+            rc = arch_xsplice_perform_rela(elf, base, rela);
+        else /* SHT_REL */
+            rc = arch_xsplice_perform_rel(elf, base, rela);
+
+        if ( rc )
+            break;
+    }
+
+    return rc;
+}
+
 static int xsplice_header_check(const struct xsplice_elf *elf)
 {
     const Elf_Ehdr *hdr = elf->hdr;
+    int rc;
 
     if ( sizeof(*elf->hdr) > elf->len )
     {
@@ -276,6 +374,26 @@  static int xsplice_header_check(const struct xsplice_elf *elf)
         return -EOPNOTSUPP;
     }
 
+    if ( !IS_ELF(*hdr) )
+    {
+        printk(XENLOG_ERR XSPLICE "%s: Not an ELF payload!\n", elf->name);
+        return -EINVAL;
+    }
+
+    if ( hdr->e_ident[EI_CLASS] != ELFCLASS64 ||
+         hdr->e_ident[EI_DATA] != ELFDATA2LSB ||
+         hdr->e_ident[EI_OSABI] != ELFOSABI_SYSV ||
+         hdr->e_type != ET_REL ||
+         hdr->e_phnum != 0 )
+    {
+        printk(XENLOG_ERR XSPLICE "%s: Invalid ELF payload!\n", elf->name);
+        return -EOPNOTSUPP;
+    }
+
+    rc = arch_xsplice_verify_elf(elf);
+    if ( rc )
+        return rc;
+
     if ( elf->hdr->e_shstrndx == SHN_UNDEF )
     {
         dprintk(XENLOG_DEBUG, XSPLICE "%s: Section name idx is undefined!?\n",
diff --git a/xen/include/xen/xsplice.h b/xen/include/xen/xsplice.h
index 00482d0..b843b5f 100644
--- a/xen/include/xen/xsplice.h
+++ b/xen/include/xen/xsplice.h
@@ -6,6 +6,9 @@ 
 #ifndef __XEN_XSPLICE_H__
 #define __XEN_XSPLICE_H__
 
+struct xsplice_elf;
+struct xsplice_elf_sec;
+struct xsplice_elf_sym;
 struct xen_sysctl_xsplice_op;
 
 #ifdef CONFIG_XSPLICE
@@ -15,6 +18,33 @@  struct xen_sysctl_xsplice_op;
 
 int xsplice_op(struct xen_sysctl_xsplice_op *);
 
+/* Arch hooks. */
+int arch_xsplice_verify_elf(const struct xsplice_elf *elf);
+int arch_xsplice_perform_rel(struct xsplice_elf *elf,
+                             const struct xsplice_elf_sec *base,
+                             const struct xsplice_elf_sec *rela);
+int arch_xsplice_perform_rela(struct xsplice_elf *elf,
+                              const struct xsplice_elf_sec *base,
+                              const struct xsplice_elf_sec *rela);
+enum va_type {
+    XSPLICE_VA_RX, /* .text */
+    XSPLICE_VA_RW, /* .data */
+    XSPLICE_VA_RO, /* .rodata */
+};
+
+#include <xen/mm.h>
+void *arch_xsplice_alloc_payload(unsigned int pages, mfn_t **mfn);
+
+/*
+ * Function to secure the allocate pages (from arch_xsplice_alloc_payload)
+ * with the right page permissions.
+ */
+int arch_xsplice_secure(void *va, unsigned int pages, enum va_type types,
+                        const mfn_t *mfn);
+
+void arch_xsplice_free_payload(void *va);
+
+void arch_xsplice_init(void);
 #else
 
 #include <xen/errno.h> /* For -EOPNOTSUPP */
diff --git a/xen/include/xen/xsplice_elf.h b/xen/include/xen/xsplice_elf.h
index 3231639..1d436ad 100644
--- a/xen/include/xen/xsplice_elf.h
+++ b/xen/include/xen/xsplice_elf.h
@@ -15,6 +15,8 @@  struct xsplice_elf_sec {
                                             elf_resolve_section_names. */
     const void *data;                    /* Pointer to the section (done by
                                             elf_resolve_sections). */
+    void *load_addr;            /* A pointer to the allocated destination.
+                                      Done by load_payload_data. */
 };
 
 struct xsplice_elf_sym {
@@ -38,6 +40,9 @@  const struct xsplice_elf_sec *xsplice_elf_sec_by_name(const struct xsplice_elf *
 int xsplice_elf_load(struct xsplice_elf *elf, const void *data);
 void xsplice_elf_free(struct xsplice_elf *elf);
 
+int xsplice_elf_resolve_symbols(struct xsplice_elf *elf);
+int xsplice_elf_perform_relocs(struct xsplice_elf *elf);
+
 #endif /* __XEN_XSPLICE_ELF_H__ */
 
 /*