Re: [Xen-devel] [PATCH v3 09/10] xen: modify page table construction

[Daniel Kiper <daniel.kiper@xxxxxxxxxx>]

On Wed, Feb 17, 2016 at 06:19:36PM +0100, Juergen Gross wrote:
> Modify the page table construction to allow multiple virtual regions
> to be mapped. This is done as preparation for removing the p2m list
> from the initial kernel mapping in order to support huge pv domains.
>
> This allows a cleaner approach for mapping the relocator page by
> using this capability.
>
> The interface to the assembler level of the relocator has to be changed
> in order to be able to process multiple page table areas.
>
> Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> ---
> V3: use constants instead of numbers as requested by Daniel Kiper
>     add lots of comments to assembly code as requested by Daniel Kiper
> ---
>  grub-core/lib/i386/xen/relocator.S   |  87 ++++++----
>  grub-core/lib/x86_64/xen/relocator.S | 134 ++++++---------
>  grub-core/lib/xen/relocator.c        |  25 ++-
>  grub-core/loader/i386/xen.c          | 325 
> ++++++++++++++++++++++++-----------
>  include/grub/i386/memory.h           |   7 +
>  include/grub/xen/relocator.h         |   6 +-
>  6 files changed, 354 insertions(+), 230 deletions(-)
>
> diff --git a/grub-core/lib/i386/xen/relocator.S 
> b/grub-core/lib/i386/xen/relocator.S
> index 694a54c..f1c729e 100644
> --- a/grub-core/lib/i386/xen/relocator.S
> +++ b/grub-core/lib/i386/xen/relocator.S
> @@ -16,6 +16,8 @@
>   *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
>   */
>
> +#include <grub/i386/memory.h>
> +#include <grub/i386/types.h>
>  #include <grub/symbol.h>
>  #include <grub/xen.h>
>
> @@ -23,78 +25,86 @@
>
>  VARIABLE(grub_relocator_xen_remap_start)
>  LOCAL(base):
> -     /* mov imm32, %ebx */
> +     /* Remap the remapper to it's new address. */
> +     /* mov imm32, %ebx - %ebx: new virtual address of remapper */
>       .byte   0xbb
>  VARIABLE(grub_relocator_xen_remapper_virt)
>       .long   0
>
> -     /* mov imm32, %ecx */
> +     /* mov imm32, %ecx - %ecx: low part of page table entry */
>       .byte   0xb9
>  VARIABLE(grub_relocator_xen_remapper_map)
>       .long   0
>
> -     /* mov imm32, %edx */
> +     /* mov imm32, %edx  - %edx: high part of page table entry */
>       .byte   0xba
>  VARIABLE(grub_relocator_xen_remapper_map_high)
>       .long   0
>
> -     movl    %ebx, %ebp
> +     movl    %ebx, %ebp      /* %ebx is clobbered by hypercall */
>
> -     movl    $2, %esi
> +     movl    $UVMF_INVLPG, %esi      /* esi: flags (inv. single entry) */
>       movl    $__HYPERVISOR_update_va_mapping, %eax
>       int     $0x82
>
>       movl    %ebp, %ebx
>       addl   $(LOCAL(cont) - LOCAL(base)), %ebx
>
> -     jmp *%ebx
> +     jmp *%ebx               /* Continue with new virtual address */
>
>  LOCAL(cont):
> -     xorl    %eax, %eax
> -     movl    %eax, %ebp
> +     /* Modify mappings of new page tables to be read-only. */
> +     /* mov imm32, %eax */
> +     .byte   0xb8
> +VARIABLE(grub_relocator_xen_paging_areas_addr)
> +     .long   0
> +     movl    %eax, %ebx
>  1:
> +     movl    0(%ebx), %ebp   /* Get start pfn of the current area */
> +     movl    GRUB_TARGET_SIZEOF_LONG(%ebx), %ecx     /* Get # of pg tables */
> +     testl   %ecx, %ecx      /* 0 -> last area reached */
> +     jz      3f
> +     addl    $(2 * GRUB_TARGET_SIZEOF_LONG), %ebx
> +     movl    %ebx, %esp      /* Save current area pointer */
>
> +2:
> +     movl    %ecx, %edi
>       /* mov imm32, %eax */
>       .byte   0xb8
>  VARIABLE(grub_relocator_xen_mfn_list)
>       .long   0
> -     movl    %eax, %edi
> -     movl    %ebp, %eax
> -     movl    0(%edi, %eax, 4), %ecx
> -
> -     /* mov imm32, %ebx */
> -     .byte   0xbb
> -VARIABLE(grub_relocator_xen_paging_start)
> -     .long   0
> -     shll    $12, %eax
> -     addl    %eax, %ebx
> +     movl    0(%eax, %ebp, 4), %ecx  /* mfn */
> +     movl    %ebp, %ebx
> +     shll    $PAGE_SHIFT, %ebx       /* virtual address (1:1 mapping) */
>       movl    %ecx, %edx
> -     shll    $12,  %ecx
> -     shrl    $20,  %edx
> -     orl     $5, %ecx
> -     movl    $2, %esi
> +     shll    $PAGE_SHIFT,  %ecx      /* prepare pte low part */
> +     shrl    $(32 - PAGE_SHIFT),  %edx       /* pte high part */
> +     orl     $(GRUB_PAGE_PRESENT | GRUB_PAGE_USER), %ecx     /* pte low */
> +     movl    $UVMF_INVLPG, %esi
>       movl    $__HYPERVISOR_update_va_mapping, %eax
> -     int     $0x82
> +     int     $0x82           /* parameters: eax, ebx, ecx, edx, esi */
>
> -     incl    %ebp
> -     /* mov imm32, %ecx */
> -     .byte   0xb9
> -VARIABLE(grub_relocator_xen_paging_size)
> -     .long   0
> -     cmpl    %ebp, %ecx
> +     incl    %ebp            /* next pfn */
> +     movl    %edi, %ecx
>
> -     ja      1b
> +     loop    2b
>
> +     mov     %esp, %ebx      /* restore area poniter */
> +     jmp     1b
> +
> +3:
> +     /* Switch page tables: pin new L3 pt, load cr3, unpin old L3. */
>       /* mov imm32, %ebx */
>       .byte   0xbb
>  VARIABLE(grub_relocator_xen_mmu_op_addr)
>       .long  0
> -     movl   $3, %ecx
> -     movl   $0, %edx
> -     movl   $0x7FF0, %esi
> +     movl   $3, %ecx         /* 3 mmu ops */
> +     movl   $0, %edx         /* pdone (not used) */
> +     movl   $DOMID_SELF, %esi
>       movl   $__HYPERVISOR_mmuext_op, %eax
>       int     $0x82
>
> +     /* Continue in virtual kernel mapping. */
>       /* mov imm32, %eax */
>       .byte   0xb8
>  VARIABLE(grub_relocator_xen_remap_continue)
> @@ -102,6 +112,9 @@ VARIABLE(grub_relocator_xen_remap_continue)
>
>       jmp *%eax
>
> +VARIABLE(grub_relocator_xen_paging_areas)
> +     .long   0, 0, 0, 0, 0, 0, 0, 0
> +
>  VARIABLE(grub_relocator_xen_mmu_op)
>       .space 256
>
> @@ -109,6 +122,7 @@ VARIABLE(grub_relocator_xen_remap_end)
>
>
>  VARIABLE(grub_relocator_xen_start)
> +     /* Unmap old remapper area. */
>       /* mov imm32, %eax */
>       .byte   0xb8
>  VARIABLE(grub_relocator_xen_remapper_virt2)
> @@ -116,14 +130,14 @@ VARIABLE(grub_relocator_xen_remapper_virt2)
>
>       movl    %eax, %edi
>
> -     xorl    %ecx, %ecx
> +     xorl    %ecx, %ecx      /* Invalid pte */
>       xorl    %edx, %edx
>
> -     movl    $2, %esi
> +     movl    $UVMF_INVLPG, %esi
>       movl    $__HYPERVISOR_update_va_mapping, %eax
>       int     $0x82
>
> -
> +     /* Prepare registers for starting kernel. */
>       /* mov imm32, %eax */
>       .byte   0xb8
>  VARIABLE(grub_relocator_xen_stack)
> @@ -145,6 +159,7 @@ VARIABLE(grub_relocator_xen_start_info)
>  VARIABLE(grub_relocator_xen_entry_point)
>       .long   0
>
> +     /* Now start the new kernel. */
>       jmp *%eax
>
>  VARIABLE(grub_relocator_xen_end)
> diff --git a/grub-core/lib/x86_64/xen/relocator.S 
> b/grub-core/lib/x86_64/xen/relocator.S
> index 92e9e72..f5364ed 100644
> --- a/grub-core/lib/x86_64/xen/relocator.S
> +++ b/grub-core/lib/x86_64/xen/relocator.S
> @@ -16,95 +16,85 @@
>   *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
>   */
>
> +#include <grub/x86_64/memory.h>
> +#include <grub/x86_64/types.h>
>  #include <grub/symbol.h>
>  #include <grub/xen.h>
>
> +/* Macro to load an imm64 value stored by the C-part into %rax: */
> +#define MOV_IMM64_RAX(var) .byte 0x48, 0xb8; VARIABLE(var); .quad 0
> +
>       .p2align        4       /* force 16-byte alignment */
>
>  VARIABLE(grub_relocator_xen_remap_start)
>  LOCAL(base):
> -     /* mov imm64, %rax */
> -     .byte   0x48
> -     .byte   0xb8
> -VARIABLE(grub_relocator_xen_remapper_virt)
> -     .quad   0
> +     /* Remap the remapper to it's new address. */
> +     MOV_IMM64_RAX(grub_relocator_xen_remapper_virt)
>
> -     movq    %rax, %rdi
> -     movq    %rax, %rbx
> +     movq    %rax, %rdi      /* %rdi: new virtual address of remapper */
> +     movq    %rax, %rbx      /* Remember new virtual address */
>
> -     /* mov imm64, %rax */
> -     .byte   0x48
> -     .byte   0xb8
> -VARIABLE(grub_relocator_xen_remapper_map)
> -     .quad   0
> +     MOV_IMM64_RAX(grub_relocator_xen_remapper_map)
>
> -     movq    %rax, %rsi
> +     movq    %rax, %rsi      /* %rsi: page table entry */
>
> -     movq    $2, %rdx
> +     movq    $UVMF_INVLPG, %rdx      /* %rdx: flags (inv. single entry) */
>       movq    $__HYPERVISOR_update_va_mapping, %rax
> -     syscall
> +     syscall                 /* Do the remap operation */
>
>       addq   $(LOCAL(cont) - LOCAL(base)), %rbx
>
> -     jmp *%rbx
> +     jmp *%rbx               /* Continue with new virtual address */
>
>  LOCAL(cont):
> -
> -     /* mov imm64, %rcx */
> -     .byte   0x48
> -     .byte   0xb9
> -VARIABLE(grub_relocator_xen_paging_size)
> -     .quad   0
> +     /* Modify mappings of new page tables to be read-only. */
> +     MOV_IMM64_RAX(grub_relocator_xen_mfn_list)
>
> -     /* mov imm64, %rax */
> -     .byte   0x48
> -     .byte   0xb8
> -VARIABLE(grub_relocator_xen_paging_start)
> -     .quad   0
> +     movq    %rax, %rbx      /* %rbx is the base of the p2m list */
> +     leaq    EXT_C(grub_relocator_xen_paging_areas) (%rip), %r8
>
> -     movq    %rax, %r12
> -
> -     /* mov imm64, %rax */
> -     .byte   0x48
> -     .byte   0xb8
> -VARIABLE(grub_relocator_xen_mfn_list)
> -     .quad   0
> -
> -     movq    %rax, %rsi
>  1:
> +     movq    0(%r8), %r12    /* Get start pfn of the current area */
> +     movq    GRUB_TARGET_SIZEOF_LONG(%r8), %rcx      /* Get # of pg tables */

Use %r9 here and...

> +     testq   %rcx, %rcx      /* 0 -> last area reached */
> +     jz      3f
> +2:
>       movq    %r12, %rdi
> -     movq    %rsi, %rbx
> -     movq    0(%rsi), %rsi
> -     shlq    $12,  %rsi
> -     orq     $5, %rsi
> -     movq    $2, %rdx
> -     movq    %rcx, %r9
> +     shlq    $PAGE_SHIFT, %rdi       /* virtual address (1:1 mapping) */
> +     movq    (%rbx, %r12, 8), %rsi   /* mfn */
> +     shlq    $PAGE_SHIFT,  %rsi
> +     orq     $(GRUB_PAGE_PRESENT | GRUB_PAGE_USER), %rsi     /* Build pte */
> +     movq    $UVMF_INVLPG, %rdx
> +     movq    %rcx, %r9       /* %rcx clobbered by hypercall */

... you can avoid this...

>       movq    $__HYPERVISOR_update_va_mapping, %rax
>       syscall
>
>       movq    %r9, %rcx

and this...

> -     addq    $8, %rbx
> -     addq    $4096, %r12
> -     movq    %rbx, %rsi
> +     incq    %r12            /* next pfn */
>
> -     loop 1b
> +     loop 2b
>
> -     leaq   LOCAL(mmu_op) (%rip), %rdi
> -     movq   $3, %rsi
> -     movq   $0, %rdx
> -     movq   $0x7FF0, %r10
> +     addq    $(2 * GRUB_TARGET_SIZEOF_LONG), %r8     /* next pg table area */
> +     jmp     1b
> +
> +3:
> +     /* Switch page tables: pin new L4 pt, load cr3, unpin old L4. */
> +     leaq   EXT_C(grub_relocator_xen_mmu_op) (%rip), %rdi
> +     movq   $3, %rsi         /* 3 mmu ops */
> +     movq   $0, %rdx         /* pdone (not used) */
> +     movq   $DOMID_SELF, %r10
>       movq   $__HYPERVISOR_mmuext_op, %rax
>       syscall
>
> -     /* mov imm64, %rax */
> -     .byte   0x48
> -     .byte   0xb8
> -VARIABLE(grub_relocator_xen_remap_continue)
> -     .quad   0
> +     /* Continue in virtual kernel mapping. */
> +     MOV_IMM64_RAX(grub_relocator_xen_remap_continue)
>
>       jmp *%rax
>
> -LOCAL(mmu_op):
> +VARIABLE(grub_relocator_xen_paging_areas)
> +     /* array of start, size pairs, size 0 is end marker */
> +     .quad   0, 0, 0, 0, 0, 0, 0, 0
> +
>  VARIABLE(grub_relocator_xen_mmu_op)
>       .space 256
>
> @@ -112,46 +102,32 @@ VARIABLE(grub_relocator_xen_remap_end)
>
>
>  VARIABLE(grub_relocator_xen_start)
> -     /* mov imm64, %rax */
> -     .byte   0x48
> -     .byte   0xb8
> -VARIABLE(grub_relocator_xen_remapper_virt2)
> -     .quad   0
> +     /* Unmap old remapper area. */
> +     MOV_IMM64_RAX(grub_relocator_xen_remapper_virt2)
>
>       movq    %rax, %rdi
>
> -     xorq    %rax, %rax
> +     xorq    %rax, %rax      /* Invalid pte */
>       movq    %rax, %rsi
>
> -     movq    $2, %rdx
> +     movq    $UVMF_INVLPG, %rdx
>       movq    $__HYPERVISOR_update_va_mapping, %rax
>       syscall
>
> -
> -     /* mov imm64, %rax */
> -     .byte   0x48
> -     .byte   0xb8
> -VARIABLE(grub_relocator_xen_stack)
> -     .quad   0
> +     /* Prepare registers for starting kernel. */
> +     MOV_IMM64_RAX(grub_relocator_xen_stack)
>
>       movq    %rax, %rsp
>
> -     /* mov imm64, %rax */
> -     .byte   0x48
> -     .byte   0xb8
> -VARIABLE(grub_relocator_xen_start_info)
> -     .quad   0
> +     MOV_IMM64_RAX(grub_relocator_xen_start_info)
>
>       movq    %rax, %rsi
>
>       cld
>
> -     /* mov imm64, %rax */
> -     .byte   0x48
> -     .byte   0xb8
> -VARIABLE(grub_relocator_xen_entry_point)
> -     .quad   0
> +     MOV_IMM64_RAX(grub_relocator_xen_entry_point)
>
> +     /* Now start the new kernel. */
>       jmp *%rax
>
>  VARIABLE(grub_relocator_xen_end)
> diff --git a/grub-core/lib/xen/relocator.c b/grub-core/lib/xen/relocator.c
> index 8f427d3..250fbd4 100644
> --- a/grub-core/lib/xen/relocator.c
> +++ b/grub-core/lib/xen/relocator.c
> @@ -36,15 +36,18 @@ extern grub_uint8_t grub_relocator_xen_remap_end;
>  extern grub_xen_reg_t grub_relocator_xen_stack;
>  extern grub_xen_reg_t grub_relocator_xen_start_info;
>  extern grub_xen_reg_t grub_relocator_xen_entry_point;
> -extern grub_xen_reg_t grub_relocator_xen_paging_start;
> -extern grub_xen_reg_t grub_relocator_xen_paging_size;
>  extern grub_xen_reg_t grub_relocator_xen_remapper_virt;
>  extern grub_xen_reg_t grub_relocator_xen_remapper_virt2;
>  extern grub_xen_reg_t grub_relocator_xen_remapper_map;
>  extern grub_xen_reg_t grub_relocator_xen_mfn_list;
> +extern struct {
> +  grub_xen_reg_t start;
> +  grub_xen_reg_t size;
> +} grub_relocator_xen_paging_areas[XEN_MAX_MAPPINGS];

Should not you add GRUB_PACKED here? Could you define type
earlier and use it here?

Daniel

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