Re: [PATCH 05/10] kexec: extend hypercall with improved load/unload ops

["Jan Beulich" <JBeulich@suse.com>]

>>> On 24.06.13 at 19:42, David Vrabel <david.vrabel@citrix.com> wrote:
> +static void init_level2_page(l2_pgentry_t *l2, unsigned long addr)
> +{
> +    unsigned long end_addr;
> +
> +    addr &= PAGE_MASK;
> +    end_addr = addr + L2_PAGETABLE_ENTRIES * (1ul << L2_PAGETABLE_SHIFT);

For one, with the code below, this can't be right: "addr" is getting
only page aligned, but the rest of the code assumes it is suitable
as a super-page.

Further, you're risking mapping non-memory here, as the caller
doesn't pass the true end of the memory block to be mapped.

And then the expression is odd - why not just

    end_addr = addr + (L2_PAGETABLE_ENTRIES << L2_PAGETABLE_SHIFT);

> +static int init_level4_page(struct kexec_image *image, l4_pgentry_t *l4,
> +                            unsigned long addr, unsigned long last_addr)
> +{
> +    unsigned long end_addr;
> +    int result;
> +
> +    addr &= PAGE_MASK;
> +    end_addr = addr + L4_PAGETABLE_ENTRIES * (1ul << L4_PAGETABLE_SHIFT);
> +
> +    while ( (addr < last_addr) && (addr < end_addr) )
> +    {
> +        struct page_info *l3_page;
> +        l3_pgentry_t *l3;
> +
> +        l3_page = kimage_alloc_control_page(image, 0);
> +        if ( !l3_page )
> +            return -ENOMEM;
> +        l3 = __map_domain_page(l3_page);
> +        result = init_level3_page(image, l3, addr, last_addr);
> +        unmap_domain_page(l3);
> +        if (result)

Coding style.

> +static int init_transition_pgtable(struct kexec_image *image, l4_pgentry_t *l4)
> +{
> +    struct page_info *l3_page;
> +    struct page_info *l2_page;
> +    struct page_info *l1_page;
> +    unsigned long vaddr, paddr;
> +    l3_pgentry_t *l3 = NULL;
> +    l2_pgentry_t *l2 = NULL;
> +    l1_pgentry_t *l1 = NULL;
> +    int ret = -ENOMEM;
> +
> +    vaddr = (unsigned long)kexec_reloc;
> +    paddr = page_to_maddr(image->control_code_page);
> +
> +    l4 += l4_table_offset(vaddr);
> +    if ( !(l4e_get_flags(*l4) & _PAGE_PRESENT) )
> +    {
> +        l3_page = kimage_alloc_control_page(image, 0);
> +        if ( !l3_page )
> +            goto out;
> +        l4e_write(l4, l4e_from_page(l3_page, __PAGE_HYPERVISOR));
> +    }
> +    else
> +        l3_page = l4e_get_page(*l4);
> +
> +    l3 = __map_domain_page(l3_page);
> +    l3 += l3_table_offset(vaddr);
> +    if ( !(l3e_get_flags(*l3) & _PAGE_PRESENT) )
> +    {
> +        l2_page = kimage_alloc_control_page(image, 0);
> +        if ( !l2_page )
> +            goto out;
> +        l3e_write(l3, l3e_from_page(l2_page, __PAGE_HYPERVISOR));
> +    }
> +    else
> +        l2_page = l3e_get_page(*l3);

Afaict you're done using "l3" here, so you should unmap it in order
to reduce the pressure on the domain page mapping resources.

> +
> +    l2 = __map_domain_page(l2_page);
> +    l2 += l2_table_offset(vaddr);
> +    if ( !(l2e_get_flags(*l2) & _PAGE_PRESENT) )
> +    {
> +        l1_page = kimage_alloc_control_page(image, 0);
> +        if ( !l1_page )
> +            goto out;
> +        l2e_write(l2, l2e_from_page(l1_page, __PAGE_HYPERVISOR));
> +    }
> +    else
> +        l1_page = l2e_get_page(*l2);

Same for "l2" at this point.

> +static int build_reloc_page_table(struct kexec_image *image)
> +{
> +    struct page_info *l4_page;
> +    l4_pgentry_t *l4;
> +    int result;
> +
> +    l4_page = kimage_alloc_control_page(image, 0);
> +    if ( !l4_page )
> +        return -ENOMEM;
> +    l4 = __map_domain_page(l4_page);
> +
> +    result = init_level4_page(image, l4, 0, max_page << PAGE_SHIFT);

What about holes in the physical address space - not just the
MMIO hole below 4Gb is a problem here, but also discontiguous
physical memory.

> --- /dev/null
> +++ b/xen/arch/x86/x86_64/kexec_reloc.S
> @@ -0,0 +1,211 @@
> +/*
> + * Relocate a kexec_image to its destination and call it.
> + *
> + * Copyright (C) 2013 Citrix Systems R&D Ltd.
> + *
> + * Portions derived from Linux's arch/x86/kernel/relocate_kernel_64.S.
> + *
> + *   Copyright (C) 2002-2005 Eric Biederman  <ebiederm@xmission.com>
> + *
> + * This source code is licensed under the GNU General Public License,
> + * Version 2.  See the file COPYING for more details.
> + */
> +#include <xen/config.h>
> +
> +#include <asm/asm_defns.h>
> +#include <asm/msr.h>
> +#include <asm/page.h>
> +#include <asm/machine_kexec.h>
> +
> +        .text
> +        .align PAGE_SIZE
> +        .code64
> +
> +ENTRY(kexec_reloc)
> +        /* %rdi - code page maddr */
> +        /* %rsi - page table maddr */
> +        /* %rdx - indirection page maddr */
> +        /* %rcx - entry maddr */
> +        /* %r8 - flags */
> +
> +        movq %rdx, %rbx
> +
> +        /* Setup stack. */
> +        leaq (reloc_stack - kexec_reloc)(%rdi), %rsp
> +
> +        /* Load reloc page table. */
> +        movq %rsi, %cr3
> +
> +        /* Jump to identity mapped code. */
> +        leaq (identity_mapped - kexec_reloc)(%rdi), %rax
> +        jmpq *%rax
> +
> +identity_mapped:
> +        pushq %rcx
> +        pushq %rbx
> +        pushq %rsi
> +        pushq %rdi
> +
> +        /*
> +         * Set cr0 to a known state:
> +         *  - Paging enabled
> +         *  - Alignment check disabled
> +         *  - Write protect disabled
> +         *  - No task switch
> +         *  - Don't do FP software emulation.
> +         *  - Proctected mode enabled
> +         */
> +        movq    %cr0, %rax
> +        andq    $~(X86_CR0_AM | X86_CR0_WP | X86_CR0_TS | X86_CR0_EM), %rax
> +        orl     $(X86_CR0_PG | X86_CR0_PE), %eax

Either "andq" and "orq" or "andl" and "orl".

> +        movq    %rax, %cr0
> +
> +        /*
> +         * Set cr4 to a known state:
> +         *  - physical address extension enabled
> +         */
> +        movq    $X86_CR4_PAE, %rax

"movl" suffices here.

> +        movq    %rax, %cr4
> +
> +        movq %rbx, %rdi
> +        call relocate_pages
> +
> +        popq %rdi
> +        popq %rsi
> +        popq %rbx
> +        popq %rcx
> +
> +        /* Need to switch to 32-bit mode? */
> +        testq $KEXEC_RELOC_FLAG_COMPAT, %r8
> +        jnz call_32_bit
> +
> +call_64_bit:
> +        /* Call the image entry point.  This should never return. */
> +        call *%rcx
> +        ud2
> +
> +call_32_bit:
> +        /* Setup IDT. */
> +        lidt compat_mode_idt(%rip)
> +
> +        /* Load compat GDT. */
> +        leaq (compat_mode_gdt - kexec_reloc)(%rdi), %rax
> +        movq %rax, (compat_mode_gdt_desc + 2)(%rip)
> +        lgdt compat_mode_gdt_desc(%rip)
> +
> +        /* Relocate compatibility mode entry point address. */
> +        leal (compatibility_mode - kexec_reloc)(%edi), %eax
> +        movl %eax, compatibility_mode_far(%rip)
> +
> +        /* Enter compatibility mode. */
> +        ljmp *compatibility_mode_far(%rip)

As you're elsewhere using mnemonic suffixes elsewhere even when
not strictly needed, for consistency I'd recommend using one on
this instruction (and perhaps also on the lidt/lgdt above) too.

> +
> +relocate_pages:
> +        /* %rdi - indirection page maddr */
> +        cld
> +        movq    %rdi, %rcx
> +        xorq    %rdi, %rdi
> +        xorq    %rsi, %rsi

I guess performance and code size aren't of highest importance
here, but "xorl" would suffice in both of the above lines.

> +        jmp     1f
> +
> +0:      /* top, read another word for the indirection page */
> +
> +        movq    (%rbx), %rcx
> +        addq    $8,     %rbx
> +1:
> +        testq   $0x1,   %rcx  /* is it a destination page? */

And "testl" (or even "testb") would be sufficient here. There are
more pointless uses of the "q" suffix further down.

In any event the 0x1 here and the other flags tested for below will
want to become manifest constants.

> +        jz      2f
> +        movq    %rcx,   %rdi
> +        andq    $0xfffffffffffff000, %rdi

The number of "f"-s here being correct can't be seen without
actually counting them - either use a manifest constant like
PAGE_MASK, or at least write "$~0xfff".

> +        jmp     0b
> +2:
> +        testq   $0x2,   %rcx  /* is it an indirection page? */
> +        jz      2f
> +        movq    %rcx,   %rbx
> +        andq    $0xfffffffffffff000, %rbx
> +        jmp     0b
> +2:
> +        testq   $0x4,   %rcx  /* is it the done indicator? */
> +        jz      2f
> +        jmp     3f

Just a single (inverse) conditional branch please.

And there are too many "2:" labels here in close succession.

> +2:
> +        testq   $0x8,   %rcx  /* is it the source indicator? */
> +        jz      2f
> +        movq    %rcx,   %rsi  /* For ever source page do a copy */

"every"?

> +        andq    $0xfffffffffffff000, %rsi
> +        movq    $512,   %rcx
> +        rep movsq
> +        jmp     0b
> +2:
> +        testq   $0x10,  %rcx  /* is it the zero indicator? */
> +        jz      0b            /* Ignore it otherwise */
> +        movq    $512,   %rcx  /* Zero the destination page. */
> +        xorq    %rax,   %rax
> +        rep stosq
> +        jmp     0b
> +3:
> +        ret
> +
> +        .code32
> +
> +compatibility_mode:
> +        /* Setup some sane segments. */
> +        movl $0x0008, %eax
> +        movl %eax, %ds
> +        movl %eax, %es
> +        movl %eax, %fs
> +        movl %eax, %gs
> +        movl %eax, %ss
> +
> +        movl %ecx, %ebp
> +
> +        /* Disable paging and therefore leave 64 bit mode. */
> +        movl %cr0, %eax
> +        andl $~X86_CR0_PG, %eax
> +        movl %eax, %cr0
> +
> +        /* Disable long mode */
> +        movl    $MSR_EFER, %ecx
> +        rdmsr
> +        andl    $~EFER_LME, %eax
> +        wrmsr
> +
> +        /* Clear cr4 to disable PAE. */
> +        movl    $0, %eax

        xorl    %eax, %eax

> +        movl    %eax, %cr4
> +
> +        /* Call the image entry point.  This should never return. */
> +        call *%ebp
> +        ud2
> +
> +        .align 16

Why? 4 is all you need.

> +compatibility_mode_far:
> +        .long 0x00000000             /* set in call_32_bit above */
> +        .word 0x0010
> +
> +        .align 16

Even more so here. If you care for alignment, you want 2 mod 8
here.

> +compat_mode_gdt_desc:
> +        .word (3*8)-1
> +        .quad 0x0000000000000000     /* set in call_32_bit above */
> +
> +        .align 16

And just 8 here.

> +compat_mode_gdt:
> +        .quad 0x0000000000000000     /* null                              */
> +        .quad 0x00cf92000000ffff     /* 0x0008 ring 0 data                */
> +        .quad 0x00cf9a000000ffff     /* 0x0010 ring 0 code, compatibility */
> +
> +compat_mode_idt:

compat_mode_idt_desc:

And if you care for alignment, 2 mod 8 again.

> +        .word   0                    /* limit */
> +        .long   0                    /* base */
> +
> +        /*
> +         * 16 words of stack are more than enough.
> +         */
> +        .fill 16,8,0
> +reloc_stack:

And now you don't care for the stack being mis-aligned?

Jan