[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [PATCH v1 07/11] xsplice: Implement payload loading
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?
+{
+ 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.
+
+ 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.
+
+ 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.
+
+ 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
|
