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

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

> > +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
> <andrew.cooper3@citrix.com>

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