|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v1 07/11] xsplice: Implement payload loading
On Thu, Nov 05, 2015 at 11:15:34AM +0000, Ross Lagerwall wrote:
> On 11/04/2015 10:21 PM, Konrad Rzeszutek Wilk wrote:
> snip
> >>
> >>+
> >>+/*
> >>+ * The following functions prepare an xSplice module to be executed by
> >>+ * allocating space, loading the allocated sections, resolving symbols,
> >>+ * performing relocations, etc.
> >>+ */
> >>+#ifdef CONFIG_X86
> >>+static void *alloc_module(size_t size)
> >
> >s/module/payload/
>
> My intention was that all the code which implements the "module loader"
> functionality (and is sort of independent from xSplice) uses the term
> "module" whereas the payload implies the loaded module + the other
> xSplice-specific bits. Your thoughts?
Aaah. I had module == payload in mind.
>
> >>+{
> >>+ mfn_t *mfn, *mfn_ptr;
> >>+ size_t pages, i;
> >>+ struct page_info *pg;
> >>+ unsigned long hole_start, hole_end, cur;
> >>+ struct payload *data, *data2;
> >>+
> >>+ ASSERT(size);
> >>+
> >>+ pages = PFN_UP(size);
> >>+ mfn = xmalloc_array(mfn_t, pages);
> >>+ if ( mfn == NULL )
> >>+ return NULL;
> >>+
> >>+ for ( i = 0; i < pages; i++ )
> >>+ {
> >>+ pg = alloc_domheap_page(NULL, 0);
> >>+ if ( pg == NULL )
> >>+ goto error;
> >>+ mfn[i] = _mfn(page_to_mfn(pg));
> >>+ }
> >
> >This looks like 'vmalloc'. Why not use that?
> >(That explanation should be part of the commit description probably)
>
> vmalloc allocates pages and then maps them to an arbitrary virtual address
> with PAGE_HYPERVISOR. I needed to use a specific virtual address with
> PAGE_HYPERVISOR_RWX.
/me nods.
>
> >
> >>+
> >>+ hole_start = (unsigned long)module_virt_start;
> >>+ hole_end = hole_start + pages * PAGE_SIZE;
> >>+ spin_lock(&payload_list_lock);
> >>+ list_for_each_entry ( data, &payload_list, list )
> >>+ {
> >>+ list_for_each_entry ( data2, &payload_list, list )
> >>+ {
> >>+ unsigned long start, end;
> >>+
> >>+ start = (unsigned long)data2->module_address;
> >>+ end = start + data2->module_pages * PAGE_SIZE;
> >>+ if ( hole_end > start && hole_start < end )
> >>+ {
> >>+ hole_start = end;
> >>+ hole_end = hole_start + pages * PAGE_SIZE;
> >>+ break;
> >>+ }
> >>+ }
> >>+ if ( &data2->list == &payload_list )
> >>+ break;
> >>+ }
> >>+ spin_unlock(&payload_list_lock);
> >
> >This could be made in a nice function. 'find_hole' perhaps?
> >
> >>+
> >>+ if ( hole_end >= module_virt_end )
> >>+ goto error;
> >>+
> >>+ for ( cur = hole_start, mfn_ptr = mfn; pages--; ++mfn_ptr, cur +=
> >>PAGE_SIZE )
> >>+ {
> >>+ if ( map_pages_to_xen(cur, mfn_x(*mfn_ptr), 1,
> >>PAGE_HYPERVISOR_RWX) )
> >>+ {
> >>+ if ( cur != hole_start )
> >>+ destroy_xen_mappings(hole_start, cur);
> >
> >I think 'destroy_xen_mappings' is OK handling hole_start == cur.
> >
> >>+ goto error;
> >>+ }
> >>+ }
> >>+ xfree(mfn);
> >>+ return (void *)hole_start;
> >>+
> >>+ error:
> >>+ while ( i-- )
> >>+ free_domheap_page(mfn_to_page(mfn_x(mfn[i])));
> >>+ xfree(mfn);
> >>+ return NULL;
> >>+}
> >>+#else
> >>+static void *alloc_module(size_t size)
> >
> >s/module/payload/
> >>+{
> >>+ return NULL;
> >>+}
> >>+#endif
> >>+
> >>+static void free_module(struct payload *payload)
> >>+{
> >>+ int i;
> >
> >unsigned int;
> >
> >>+ struct page_info *pg;
> >>+ PAGE_LIST_HEAD(pg_list);
> >>+ void *va = payload->module_address;
> >>+ unsigned long addr = (unsigned long)va;
> >>+
> >>+ if ( !payload->module_address )
> >>+ return;
> >
> >How about 'if ( !addr )
> > return;
> >?
> >
> >>+
> >>+ payload->module_address = NULL;
> >>+
> >>+ for ( i = 0; i < payload->module_pages; i++ )
> >>+ page_list_add(vmap_to_page(va + i * PAGE_SIZE), &pg_list);
> >>+
> >>+ destroy_xen_mappings(addr, addr + payload->module_pages * PAGE_SIZE);
> >>+
> >>+ while ( (pg = page_list_remove_head(&pg_list)) != NULL )
> >>+ free_domheap_page(pg);
> >>+
> >>+ payload->module_pages = 0;
> >>+}
> >>+
> >>+static void alloc_section(struct xsplice_elf_sec *sec, size_t *core_size)
> >
> >s/alloc/compute/?
> >
> >>+{
> >>+ size_t align_size = ROUNDUP(*core_size, sec->sec->sh_addralign);
> >>+ sec->sec->sh_entsize = align_size;
> >>+ *core_size = sec->sec->sh_size + align_size;
> >>+}
> >>+
> >>+static int move_module(struct payload *payload, struct xsplice_elf *elf)
> >>+{
> >>+ uint8_t *buf;
> >>+ int i;
> >
> >unsigned int i;
> >
> >>+ size_t core_size = 0;
> >>+
> >>+ /* Allocate text regions */
> >
> >s/Allocate/Compute/
> >
> >>+ for ( i = 0; i < elf->hdr->e_shnum; i++ )
> >>+ {
> >>+ if ( (elf->sec[i].sec->sh_flags & (SHF_ALLOC|SHF_EXECINSTR)) ==
> >>+ (SHF_ALLOC|SHF_EXECINSTR) )
> >>+ alloc_section(&elf->sec[i], &core_size);
> >>+ }
> >>+
> >>+ /* Allocate rw data */
> >>+ for ( i = 0; 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) )
> >>+ alloc_section(&elf->sec[i], &core_size);
> >>+ }
> >>+
> >>+ /* Allocate ro data */
> >>+ for ( i = 0; 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) )
> >>+ alloc_section(&elf->sec[i], &core_size);
> >>+ }
> >>+
> >>+ buf = alloc_module(core_size);
> >>+ if ( !buf ) {
> >>+ printk(XENLOG_ERR "Could not allocate memory for module\n");
> >
> >(%s: Could not allocate %u memory for payload!\n", elf->name, core_size);
> >
> >>+ return -ENOMEM;
> >>+ }
> >>+ memset(buf, 0, core_size);
> >
> >Perhaps for fun it ought to be 'ud2' ?
>
> There's no point. It either gets memcpy'd over or needs to be set to zero
> for BSS.
Or 0xcc. Was thinking it may be good have it poison so
that we would trip over bugs more easily. Maybe for debug builds?
>
> >
> >>+
> >>+ for ( i = 0; i < elf->hdr->e_shnum; i++ )
> >>+ {
> >>+ if ( elf->sec[i].sec->sh_flags & SHF_ALLOC )
> >>+ {
> >>+ elf->sec[i].load_addr = buf + elf->sec[i].sec->sh_entsize;
> >>+ memcpy(elf->sec[i].load_addr, elf->sec[i].data,
> >>+ elf->sec[i].sec->sh_size);
> >>+ printk(XENLOG_DEBUG "Loaded %s at 0x%p\n",
> >
> >Add %s: at the start ..
> >>+ elf->sec[i].name, elf->sec[i].load_addr);
> >
> >which would be elf->name.
> >
> >>+ }
> >>+ }
> >>+
> >>+ payload->module_address = buf;
> >>+ payload->module_pages = PFN_UP(core_size);
> >
> >Instead of module could we name it payload?
>
> See comment above.
>
> >
> >>+
> >>+ return 0;
> >>+}
> >>+
> >>+static int resolve_symbols(struct xsplice_elf *elf)
> >
> >s/resolve/check/
>
> No, this is resolving section symbols.
>
> >
> >>+{
> >>+ int i;
> >
> >unsigned int;
> >
> >>+
> >>+ for ( i = 1; i < elf->nsym; i++ )
> >
> >Why 1? Please explain as comment.
>
> The first entry of an ELF symbol table is the "undefined symbol index". This
> code is expected to be read alongside the ELF specification :-)
>
> >
> >
> >>+ {
> >>+ switch ( elf->sym[i].sym->st_shndx )
> >>+ {
> >>+ case SHN_COMMON:
> >>+ printk(XENLOG_ERR "Unexpected common symbol: %s\n",
> >>+ elf->sym[i].name);
> >
> >Please also include elf->name in the error.
> >
> >>+ return -EINVAL;
> >>+ break;
> >>+ case SHN_UNDEF:
> >>+ printk(XENLOG_ERR "Unknown symbol: %s\n",
> >>elf->sym[i].name);
> >
> >Ditto.
> >>+ return -ENOENT;
> >>+ break;
> >>+ case SHN_ABS:
> >>+ printk(XENLOG_DEBUG "Absolute symbol: %s => 0x%p\n",
> >>+ elf->sym[i].name, (void
> >>*)elf->sym[i].sym->st_value);
> >>+ break;
> >>+ default:
> >>+ if ( elf->sec[elf->sym[i].sym->st_shndx].sec->sh_flags &
> >>SHF_ALLOC )
> >>+ {
> >>+ elf->sym[i].sym->st_value +=
> >>+ (unsigned
> >>long)elf->sec[elf->sym[i].sym->st_shndx].load_addr;
> >>+ printk(XENLOG_DEBUG "Symbol resolved: %s => 0x%p\n",
> >
> >Ditto;
> >>+ elf->sym[i].name, (void
> >>*)elf->sym[i].sym->st_value);
> >>+ }
> >>+ }
> >>+ }
> >>+
> >>+ return 0;
> >>+}
> >>+
> >>+static int perform_relocs(struct xsplice_elf *elf)
> >>+{
> >>+ struct xsplice_elf_sec *rela, *base;
> >>+ int i, rc;
> >>+
> >
> >unsigned int i;
> >
> >>+ for ( i = 0; i < elf->hdr->e_shnum; i++ )
> >>+ {
> >>+ rela = &elf->sec[i];
> >>+
> >>+ /* Is it a valid relocation section? */
> >>+ if ( rela->sec->sh_info >= elf->hdr->e_shnum )
> >>+ continue;
> >
> >Um, don't we want to mark it as invalid or such?
> >Or overwrite it so we won't use it?
>
> The code doesn't use it. I don't understand the concern.
The comment. Perhaps change it to
/* Ignore invalid relocation sections. */
>
> >
> >>+
> >>+ 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 = xsplice_perform_rela(elf, base, rela);
> >>+ else if ( elf->sec[i].sec->sh_type == SHT_REL )
> >>+ rc = xsplice_perform_rel(elf, base, rela);
> >>+
> >>+ if ( rc )
> >>+ return rc;
> >>+ }
> >>+
> >>+ return 0;
> >>+}
> >>+
> >>+static int load_module(struct payload *payload, uint8_t *raw, ssize_t len)
> >>+{
> >>+ struct xsplice_elf elf;
> >
> >Wait a minute? We ditch it after this?
> >
> >>+ int rc = 0;
> >>+
> >>+ rc = xsplice_verify_elf(raw, len);
> >>+ if ( rc )
> >>+ return rc;
> >>+
> >>+ rc = xsplice_elf_load(&elf, raw, len);
> >>+ if ( rc )
> >>+ return rc;
> >>+
> >>+ rc = move_module(payload, &elf);
> >>+ if ( rc )
> >>+ goto err_elf;
> >>+
> >>+ rc = resolve_symbols(&elf);
> >>+ if ( rc )
> >>+ goto err_module;
> >>+
> >>+ rc = perform_relocs(&elf);
> >>+ if ( rc )
> >>+ goto err_module;
> >>+
> >
> >Shouldn't you call xsplice_elf_free(&elf) here? Or
> >hook up the elf to the 'struct payload'?
> >
> >
> >If not, who is going to clean up elf->sec and elf->sym when the
> >payload is unloaded?
>
> Yes, I forgot to free it here.
>
> >>+ return 0;
> >>+
> >>+ err_module:
> >>+ free_module(payload);
> >>+ err_elf:
> >>+ xsplice_elf_free(&elf);
> >>+
> >>+ return rc;
> >>+}
> >>+
> >> static int __init xsplice_init(void)
> >> {
> >> register_keyhandler('x', xsplice_printall, "print xsplicing info", 1);
> >>diff --git a/xen/include/asm-x86/x86_64/page.h
> >>b/xen/include/asm-x86/x86_64/page.h
> >>index 19ab4d0..e6f08e9 100644
> >>--- a/xen/include/asm-x86/x86_64/page.h
> >>+++ b/xen/include/asm-x86/x86_64/page.h
> >>@@ -38,6 +38,8 @@
> >> #include <xen/pdx.h>
> >>
> >> extern unsigned long xen_virt_end;
> >>+extern unsigned long module_virt_start;
> >>+extern unsigned long module_virt_end;
> >>
> >> #define spage_to_pdx(spg) (((spg) -
> >> spage_table)<<(SUPERPAGE_SHIFT-PAGE_SHIFT))
> >> #define pdx_to_spage(pdx) (spage_table +
> >> ((pdx)>>(SUPERPAGE_SHIFT-PAGE_SHIFT)))
> >>diff --git a/xen/include/xen/xsplice.h b/xen/include/xen/xsplice.h
> >>index 41e28da..a3946a3 100644
> >>--- a/xen/include/xen/xsplice.h
> >>+++ b/xen/include/xen/xsplice.h
> >>@@ -1,9 +1,31 @@
> >> #ifndef __XEN_XSPLICE_H__
> >> #define __XEN_XSPLICE_H__
> >>
> >>+struct xsplice_elf;
> >>+struct xsplice_elf_sec;
> >>+struct xsplice_elf_sym;
> >>+
> >>+struct xsplice_patch_func {
> >>+ unsigned long new_addr;
> >>+ unsigned long new_size;
> >>+ unsigned long old_addr;
> >>+ unsigned long old_size;
> >>+ char *name;
> >>+ unsigned char undo[8];
> >>+};
> >
> >We don't use them in this patch. They could be moved to another patch.
>
> OK.
>
> --
> Ross Lagerwall
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |