Re: [Xen-devel] [PATCH v6 09/24] xsplice: Implement payload loading

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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
<andrew.cooper3@xxxxxxxxxx>

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