Re: [Xen-devel] [PATCH v10 08/24] xsplice: Implement payload loading

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 27.04.16 at 21:27, <konrad.wilk@xxxxxxxxxx> wrote:
> From: Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>
> 
> 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:
> "rm/x86/vmap: Add v[z|m]alloc_xen, and vm_init_type") to allocate
> a region of memory within the [xen_virt_end, XEN_VIRT_END] for the code.
> 
> We also use the "x86/mm: Introduce modify_xen_mappings()"
> to change the virtual address page-table permissions.
> 
> Signed-off-by: Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> Acked-by: Julien Grall <julien.grall@xxxxxxx>

Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
with one remark (not strictly calling for a change):

> +static int move_payload(struct payload *payload, struct xsplice_elf *elf)
> +{
> +    void *text_buf, *ro_buf, *rw_buf;
> +    unsigned int i;
> +    size_t size = 0;
> +    unsigned int *offset;
> +    int rc = 0;
> +
> +    offset = xmalloc_array(unsigned int, elf->hdr->e_shnum);
> +    if ( !offset )
> +        return -ENOMEM;
> +
> +    /* Compute size of different regions. */
> +    for ( i = 1; i < elf->hdr->e_shnum; i++ )
> +    {
> +        /*
> +         * Do nothing. These are .rel.text, rel.*, .symtab, .strtab,
> +         * and .shstrtab. For the non-relocate we allocate and copy these
> +         * via other means - and the .rel we can ignore as we only use it
> +         * once during loading.
> +         */
> +        if ( !(elf->sec[i].sec->sh_flags & SHF_ALLOC) )
> +        {
> +            offset[i] = UINT_MAX;
> +            continue;
> +        }

You could even have avoided the need for braces and "continue"
by making ...

> +        if ( (elf->sec[i].sec->sh_flags & SHF_EXECINSTR) &&

... this "else if".

Jan


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