| Message ID | 1455177206-8974-6-git-send-email-jgross@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Feb 11, 2016 at 08:53:25AM +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. I hope that older kernels could be loaded as is and everything works as expected. > Signed-off-by: Juergen Gross <jgross@suse.com> > --- > grub-core/lib/i386/xen/relocator.S | 47 +++--- > grub-core/lib/x86_64/xen/relocator.S | 44 +++-- > grub-core/lib/xen/relocator.c | 22 ++- > grub-core/loader/i386/xen.c | 313 +++++++++++++++++++++++------------ > include/grub/xen/relocator.h | 6 +- > 5 files changed, 277 insertions(+), 155 deletions(-) > > diff --git a/grub-core/lib/i386/xen/relocator.S b/grub-core/lib/i386/xen/relocator.S > index 694a54c..c23b405 100644 > --- a/grub-core/lib/i386/xen/relocator.S > +++ b/grub-core/lib/i386/xen/relocator.S > @@ -50,41 +50,45 @@ VARIABLE(grub_relocator_xen_remapper_map_high) > jmp *%ebx > > LOCAL(cont): > - xorl %eax, %eax > - movl %eax, %ebp > + /* mov imm32, %eax */ > + .byte 0xb8 > +VARIABLE(grub_relocator_xen_paging_areas_addr) > + .long 0 > + movl %eax, %ebx > 1: > - > + movl 0(%ebx), %ebp > + movl 4(%ebx), %ecx Oh... No, please use constants not plain numbers and explain in comments what is going on. Otherwise it is unreadable. > + testl %ecx, %ecx > + jz 3f > + addl $8, %ebx > + movl %ebx, %esp > + > +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 > + movl %ebp, %ebx > + shll $12, %ebx Ditto. > movl %ecx, %edx > shll $12, %ecx > shrl $20, %edx > orl $5, %ecx > movl $2, %esi > movl $__HYPERVISOR_update_va_mapping, %eax > - int $0x82 > + int $0x82 /* parameters: eax, ebx, ecx, edx, esi */ Please use more verbose comments. > > incl %ebp > - /* mov imm32, %ecx */ > - .byte 0xb9 > -VARIABLE(grub_relocator_xen_paging_size) > - .long 0 > - cmpl %ebp, %ecx > + movl %edi, %ecx > + > + loop 2b > > - ja 1b > + mov %esp, %ebx > + jmp 1b > > +3: > /* mov imm32, %ebx */ > .byte 0xbb > VARIABLE(grub_relocator_xen_mmu_op_addr) > @@ -102,6 +106,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 > > diff --git a/grub-core/lib/x86_64/xen/relocator.S b/grub-core/lib/x86_64/xen/relocator.S > index 92e9e72..dbb90c7 100644 > --- a/grub-core/lib/x86_64/xen/relocator.S > +++ b/grub-core/lib/x86_64/xen/relocator.S Is to possible to split this patch to i386 and x86-64 stuff patches? > @@ -50,31 +50,24 @@ VARIABLE(grub_relocator_xen_remapper_map) > > LOCAL(cont): > > - /* mov imm64, %rcx */ > - .byte 0x48 > - .byte 0xb9 > -VARIABLE(grub_relocator_xen_paging_size) > - .quad 0 > - > - /* mov imm64, %rax */ > - .byte 0x48 > - .byte 0xb8 > -VARIABLE(grub_relocator_xen_paging_start) > - .quad 0 > - > - movq %rax, %r12 > - > /* mov imm64, %rax */ > .byte 0x48 > .byte 0xb8 > VARIABLE(grub_relocator_xen_mfn_list) > .quad 0 > > - movq %rax, %rsi > + movq %rax, %rbx > + leaq EXT_C(grub_relocator_xen_paging_areas) (%rip), %r8 > + > 1: > + movq 0(%r8), %r12 > + movq 8(%r8), %rcx Ditto. > + testq %rcx, %rcx > + jz 3f > +2: > movq %r12, %rdi > - movq %rsi, %rbx > - movq 0(%rsi), %rsi > + shlq $12, %rdi > + movq (%rbx, %r12, 8), %rsi Ditto. > shlq $12, %rsi > orq $5, %rsi > movq $2, %rdx > @@ -83,13 +76,15 @@ VARIABLE(grub_relocator_xen_mfn_list) > syscall > > movq %r9, %rcx > - addq $8, %rbx > - addq $4096, %r12 > - movq %rbx, %rsi > + incq %r12 > + > + loop 2b > > - loop 1b > + addq $16, %r8 Ditto. > + jmp 1b > > - leaq LOCAL(mmu_op) (%rip), %rdi > +3: > + leaq EXT_C(grub_relocator_xen_mmu_op) (%rip), %rdi > movq $3, %rsi > movq $0, %rdx > movq $0x7FF0, %r10 Ugh... Please fix this too. Probably in separate patch. > @@ -104,7 +99,10 @@ VARIABLE(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 > > diff --git a/grub-core/lib/xen/relocator.c b/grub-core/lib/xen/relocator.c > index 8f427d3..bc29055 100644 > --- a/grub-core/lib/xen/relocator.c > +++ b/grub-core/lib/xen/relocator.c > @@ -36,15 +36,15 @@ 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 grub_xen_reg_t grub_relocator_xen_paging_areas[2 * XEN_MAX_MAPPINGS]; > extern grub_xen_reg_t grub_relocator_xen_remap_continue; > #ifdef __i386__ > extern grub_xen_reg_t grub_relocator_xen_mmu_op_addr; > +extern grub_xen_reg_t grub_relocator_xen_paging_areas_addr; > extern grub_xen_reg_t grub_relocator_xen_remapper_map_high; > #endif > extern mmuext_op_t grub_relocator_xen_mmu_op[3]; > @@ -61,6 +61,7 @@ grub_relocator_xen_boot (struct grub_relocator *rel, > { > grub_err_t err; > void *relst; > + int i; > grub_relocator_chunk_t ch, ch_tramp; > grub_xen_mfn_t *mfn_list = > (grub_xen_mfn_t *) grub_xen_start_page_addr->mfn_list; > @@ -77,8 +78,11 @@ grub_relocator_xen_boot (struct grub_relocator *rel, > grub_relocator_xen_stack = state.stack; > grub_relocator_xen_start_info = state.start_info; > grub_relocator_xen_entry_point = state.entry_point; > - grub_relocator_xen_paging_start = state.paging_start << 12; > - grub_relocator_xen_paging_size = state.paging_size; > + for (i = 0; i < XEN_MAX_MAPPINGS; i++) > + { > + grub_relocator_xen_paging_areas[2 * i] = state.paging_start[i]; > + grub_relocator_xen_paging_areas[2 * i + 1] = state.paging_size[i]; Why 2 and 1? Constants? > + } > grub_relocator_xen_remapper_virt = remapper_virt; > grub_relocator_xen_remapper_virt2 = remapper_virt; > grub_relocator_xen_remap_continue = trampoline_virt; > @@ -88,10 +92,12 @@ grub_relocator_xen_boot (struct grub_relocator *rel, > grub_relocator_xen_remapper_map_high = (mfn_list[remapper_pfn] >> 20); > grub_relocator_xen_mmu_op_addr = (char *) &grub_relocator_xen_mmu_op > - (char *) &grub_relocator_xen_remap_start + remapper_virt; > + grub_relocator_xen_paging_areas_addr = > + (char *) &grub_relocator_xen_paging_areas > + - (char *) &grub_relocator_xen_remap_start + remapper_virt; > #endif > > - grub_relocator_xen_mfn_list = state.mfn_list > - + state.paging_start * sizeof (grub_addr_t); > + grub_relocator_xen_mfn_list = state.mfn_list; > > grub_memset (grub_relocator_xen_mmu_op, 0, > sizeof (grub_relocator_xen_mmu_op)); > @@ -100,9 +106,9 @@ grub_relocator_xen_boot (struct grub_relocator *rel, > #else > grub_relocator_xen_mmu_op[0].cmd = MMUEXT_PIN_L4_TABLE; > #endif > - grub_relocator_xen_mmu_op[0].arg1.mfn = mfn_list[state.paging_start]; > + grub_relocator_xen_mmu_op[0].arg1.mfn = mfn_list[state.paging_start[0]]; > grub_relocator_xen_mmu_op[1].cmd = MMUEXT_NEW_BASEPTR; > - grub_relocator_xen_mmu_op[1].arg1.mfn = mfn_list[state.paging_start]; > + grub_relocator_xen_mmu_op[1].arg1.mfn = mfn_list[state.paging_start[0]]; > grub_relocator_xen_mmu_op[2].cmd = MMUEXT_UNPIN_TABLE; > grub_relocator_xen_mmu_op[2].arg1.mfn = > mfn_list[grub_xen_start_page_addr->pt_base >> 12]; > diff --git a/grub-core/loader/i386/xen.c b/grub-core/loader/i386/xen.c > index 0f41048..5e10420 100644 > --- a/grub-core/loader/i386/xen.c > +++ b/grub-core/loader/i386/xen.c > @@ -42,6 +42,29 @@ > > GRUB_MOD_LICENSE ("GPLv3+"); > > +#ifdef __x86_64__ > +#define NUMBER_OF_LEVELS 4 > +#define INTERMEDIATE_OR 7 > +#define VIRT_MASK 0x0000ffffffffffffULL > +#else > +#define NUMBER_OF_LEVELS 3 > +#define INTERMEDIATE_OR 3 > +#define VIRT_MASK 0x00000000ffffffffULL Long expected constants... Nice! > +#endif > + > +struct grub_xen_mapping_lvl { > + grub_uint64_t virt_start; > + grub_uint64_t virt_end; > + grub_uint64_t pfn_start; > + grub_uint64_t n_pt_pages; > +}; > + > +struct grub_xen_mapping { > + grub_uint64_t *where; > + struct grub_xen_mapping_lvl area; > + struct grub_xen_mapping_lvl lvls[NUMBER_OF_LEVELS]; > +}; > + > static struct grub_relocator *relocator = NULL; > static grub_uint64_t max_addr; > static grub_dl_t my_mod; > @@ -56,9 +79,10 @@ static struct grub_relocator_xen_state state; > static grub_xen_mfn_t *virt_mfn_list; > static struct start_info *virt_start_info; > static grub_xen_mfn_t console_pfn; > -static grub_uint64_t *virt_pgtable; > -static grub_uint64_t pgtbl_start; > static grub_uint64_t pgtbl_end; > +static struct grub_xen_mapping mappings[XEN_MAX_MAPPINGS]; > +static int n_mappings; > +static struct grub_xen_mapping *map_reloc; > > #define PAGE_SIZE 4096 > #define MAX_MODULES (PAGE_SIZE / sizeof (struct xen_multiboot_mod_list)) > @@ -75,100 +99,162 @@ page2offset (grub_uint64_t page) > return page << PAGE_SHIFT; > } > > -#ifdef __x86_64__ > -#define NUMBER_OF_LEVELS 4 > -#define INTERMEDIATE_OR 7 > -#else > -#define NUMBER_OF_LEVELS 3 > -#define INTERMEDIATE_OR 3 > -#endif > - > -static grub_uint64_t > -get_pgtable_size (grub_uint64_t total_pages, grub_uint64_t virt_base) > +static grub_err_t > +get_pgtable_size (grub_uint64_t from, grub_uint64_t to, grub_uint64_t pfn) > { > - if (!virt_base) > - total_pages++; > - grub_uint64_t ret = 0; > - grub_uint64_t ll = total_pages; > - int i; > - for (i = 0; i < NUMBER_OF_LEVELS; i++) > - { > - ll = (ll + POINTERS_PER_PAGE - 1) >> LOG_POINTERS_PER_PAGE; > - /* PAE wants all 4 root directories present. */ > -#ifdef __i386__ > - if (i == 1) > - ll = 4; > -#endif > - ret += ll; > - } > - for (i = 1; i < NUMBER_OF_LEVELS; i++) > - if (virt_base >> (PAGE_SHIFT + i * LOG_POINTERS_PER_PAGE)) > - ret++; > - return ret; > -} > + struct grub_xen_mapping *map, *map_cmp; > + grub_uint64_t mask, bits; > + int i, m; > > -static void > -generate_page_table (grub_uint64_t *where, grub_uint64_t paging_start, > - grub_uint64_t paging_end, grub_uint64_t total_pages, > - grub_uint64_t virt_base, grub_xen_mfn_t *mfn_list) > -{ > - if (!virt_base) > - paging_end++; > + if (n_mappings == XEN_MAX_MAPPINGS) > + return grub_error (GRUB_ERR_BUG, "too many mapped areas"); > + > + grub_dprintf ("xen", "get_pgtable_size %d from=%llx, to=%llx, pfn=%llx\n", > + n_mappings, (unsigned long long) from, (unsigned long long) to, > + (unsigned long long) pfn); > > - grub_uint64_t lx[NUMBER_OF_LEVELS], lxs[NUMBER_OF_LEVELS]; > - grub_uint64_t nlx, nls, sz = 0; > - int l; > + map = mappings + n_mappings; > + grub_memset (map, 0, sizeof (*map)); > > - nlx = paging_end; > - nls = virt_base >> PAGE_SHIFT; > - for (l = 0; l < NUMBER_OF_LEVELS; l++) > + map->area.virt_start = from & VIRT_MASK; > + map->area.virt_end = (to - 1) & VIRT_MASK; > + map->area.n_pt_pages = 0; > + > + for (i = NUMBER_OF_LEVELS - 1; i >= 0; i--) > { > - nlx = (nlx + POINTERS_PER_PAGE - 1) >> LOG_POINTERS_PER_PAGE; > - /* PAE wants all 4 root directories present. */ > + map->lvls[i].pfn_start = pfn + map->area.n_pt_pages; > + if (i == NUMBER_OF_LEVELS - 1) > + { > + if (n_mappings == 0) > + { > + map->lvls[i].virt_start = 0; > + map->lvls[i].virt_end = VIRT_MASK; > + map->lvls[i].n_pt_pages = 1; > + map->area.n_pt_pages++; > + } > + continue; > + } > + > + bits = PAGE_SHIFT + (i + 1) * LOG_POINTERS_PER_PAGE; > + mask = (1ULL << bits) - 1; > + map->lvls[i].virt_start = map->area.virt_start & ~mask; > + map->lvls[i].virt_end = map->area.virt_end | mask; > #ifdef __i386__ > - if (l == 1) > - nlx = 4; > + /* PAE wants last root directory present. */ > + if (i == 1 && to <= 0xc0000000 && n_mappings == 0) Ditto. > + map->lvls[i].virt_end = VIRT_MASK; > #endif > - lx[l] = nlx; > - sz += lx[l]; > - lxs[l] = nls & (POINTERS_PER_PAGE - 1); > - if (nls && l != 0) > - sz++; > - nls >>= LOG_POINTERS_PER_PAGE; > + for (m = 0; m < n_mappings; m++) > + { > + map_cmp = mappings + m; > + if (map_cmp->lvls[i].virt_start == map_cmp->lvls[i].virt_end) > + continue; > + if (map->lvls[i].virt_start >= map_cmp->lvls[i].virt_start && > + map->lvls[i].virt_end <= map_cmp->lvls[i].virt_end) > + { > + map->lvls[i].virt_start = 0; > + map->lvls[i].virt_end = 0; > + break; > + } > + if (map->lvls[i].virt_start >= map_cmp->lvls[i].virt_start && > + map->lvls[i].virt_start <= map_cmp->lvls[i].virt_end) > + map->lvls[i].virt_start = map_cmp->lvls[i].virt_end + 1; > + if (map->lvls[i].virt_end >= map_cmp->lvls[i].virt_start && > + map->lvls[i].virt_end <= map_cmp->lvls[i].virt_end) > + map->lvls[i].virt_end = map_cmp->lvls[i].virt_start - 1; > + } > + if (map->lvls[i].virt_start < map->lvls[i].virt_end) > + map->lvls[i].n_pt_pages = > + ((map->lvls[i].virt_end - map->lvls[i].virt_start) >> bits) + 1; > + map->area.n_pt_pages += map->lvls[i].n_pt_pages; > + grub_dprintf ("xen", "get_pgtable_size level %d: virt %llx-%llx %d pts\n", > + i, (unsigned long long) map->lvls[i].virt_start, > + (unsigned long long) map->lvls[i].virt_end, > + (int) map->lvls[i].n_pt_pages); > } > > - grub_uint64_t lp; > - grub_uint64_t j; > - grub_uint64_t *pg = (grub_uint64_t *) where; > - int pr = 0; > + grub_dprintf ("xen", "get_pgtable_size return: %d page tables\n", > + (int) map->area.n_pt_pages); > + > + state.paging_start[n_mappings] = pfn; > + state.paging_size[n_mappings] = map->area.n_pt_pages; > + > + return GRUB_ERR_NONE; > +} > + > +static grub_uint64_t * > +get_pg_table_virt (int mapping, int level) > +{ > + grub_uint64_t pfn; > + struct grub_xen_mapping *map; > + > + map = mappings + mapping; > + pfn = map->lvls[level].pfn_start - map->lvls[NUMBER_OF_LEVELS - 1].pfn_start; > + return map->where + pfn * POINTERS_PER_PAGE; > +} > > - grub_memset (pg, 0, sz * PAGE_SIZE); > +static grub_uint64_t > +get_pg_table_prot (int level, grub_uint64_t pfn) > +{ > + int m; > + grub_uint64_t pfn_s, pfn_e; > > - lp = paging_start + lx[NUMBER_OF_LEVELS - 1]; > - for (l = NUMBER_OF_LEVELS - 1; l >= 1; l--) > + if (level > 0) > + return INTERMEDIATE_OR; > + for (m = 0; m < n_mappings; m++) > { > - if (lxs[l] || pr) > - pg[0] = page2offset (mfn_list[lp++]) | INTERMEDIATE_OR; > - if (pr) > - pg += POINTERS_PER_PAGE; > - for (j = 0; j < lx[l - 1]; j++) > - pg[j + lxs[l]] = page2offset (mfn_list[lp++]) | INTERMEDIATE_OR; > - pg += lx[l] * POINTERS_PER_PAGE; > - if (lxs[l]) > - pr = 1; > + pfn_s = mappings[m].lvls[NUMBER_OF_LEVELS - 1].pfn_start; > + pfn_e = mappings[m].area.n_pt_pages + pfn_s; > + if (pfn >= pfn_s && pfn < pfn_e) > + return 5; > } > + return 7; What 7 and 5 means? I am exhausted. Please fix above stuff and I will review this patch again after fixes. Daniel
On 11/02/16 13:47, Daniel Kiper wrote: > On Thu, Feb 11, 2016 at 08:53:25AM +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. > > I hope that older kernels could be loaded as is and everything works as expected. See Patch 0/6. I have tested old kernels, too. > >> Signed-off-by: Juergen Gross <jgross@suse.com> >> --- >> grub-core/lib/i386/xen/relocator.S | 47 +++--- >> grub-core/lib/x86_64/xen/relocator.S | 44 +++-- >> grub-core/lib/xen/relocator.c | 22 ++- >> grub-core/loader/i386/xen.c | 313 +++++++++++++++++++++++------------ >> include/grub/xen/relocator.h | 6 +- >> 5 files changed, 277 insertions(+), 155 deletions(-) >> >> diff --git a/grub-core/lib/i386/xen/relocator.S b/grub-core/lib/i386/xen/relocator.S >> index 694a54c..c23b405 100644 >> --- a/grub-core/lib/i386/xen/relocator.S >> +++ b/grub-core/lib/i386/xen/relocator.S >> @@ -50,41 +50,45 @@ VARIABLE(grub_relocator_xen_remapper_map_high) >> jmp *%ebx >> >> LOCAL(cont): >> - xorl %eax, %eax >> - movl %eax, %ebp >> + /* mov imm32, %eax */ >> + .byte 0xb8 >> +VARIABLE(grub_relocator_xen_paging_areas_addr) >> + .long 0 >> + movl %eax, %ebx >> 1: >> - >> + movl 0(%ebx), %ebp >> + movl 4(%ebx), %ecx > > Oh... No, please use constants not plain numbers and > explain in comments what is going on. Otherwise it > is unreadable. Hmm, yes, some comments wouldn't hurt. :-) Regarding constants: do you really think I should introduce their first usage in this file with my patch? > >> + testl %ecx, %ecx >> + jz 3f >> + addl $8, %ebx >> + movl %ebx, %esp >> + >> +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 >> + movl %ebp, %ebx >> + shll $12, %ebx > > Ditto. Look at the removed line above: I just switched register usage. > >> movl %ecx, %edx >> shll $12, %ecx >> shrl $20, %edx >> orl $5, %ecx >> movl $2, %esi >> movl $__HYPERVISOR_update_va_mapping, %eax >> - int $0x82 >> + int $0x82 /* parameters: eax, ebx, ecx, edx, esi */ > > Please use more verbose comments. :-) > >> >> incl %ebp >> - /* mov imm32, %ecx */ >> - .byte 0xb9 >> -VARIABLE(grub_relocator_xen_paging_size) >> - .long 0 >> - cmpl %ebp, %ecx >> + movl %edi, %ecx >> + >> + loop 2b >> >> - ja 1b >> + mov %esp, %ebx >> + jmp 1b >> >> +3: >> /* mov imm32, %ebx */ >> .byte 0xbb >> VARIABLE(grub_relocator_xen_mmu_op_addr) >> @@ -102,6 +106,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 >> >> diff --git a/grub-core/lib/x86_64/xen/relocator.S b/grub-core/lib/x86_64/xen/relocator.S >> index 92e9e72..dbb90c7 100644 >> --- a/grub-core/lib/x86_64/xen/relocator.S >> +++ b/grub-core/lib/x86_64/xen/relocator.S > > Is to possible to split this patch to i386 and x86-64 stuff patches? I don't think so. The C-part is common and assembler sources must both match. > >> @@ -50,31 +50,24 @@ VARIABLE(grub_relocator_xen_remapper_map) >> >> LOCAL(cont): >> >> - /* mov imm64, %rcx */ >> - .byte 0x48 >> - .byte 0xb9 >> -VARIABLE(grub_relocator_xen_paging_size) >> - .quad 0 >> - >> - /* mov imm64, %rax */ >> - .byte 0x48 >> - .byte 0xb8 >> -VARIABLE(grub_relocator_xen_paging_start) >> - .quad 0 >> - >> - movq %rax, %r12 >> - >> /* mov imm64, %rax */ >> .byte 0x48 >> .byte 0xb8 >> VARIABLE(grub_relocator_xen_mfn_list) >> .quad 0 >> >> - movq %rax, %rsi >> + movq %rax, %rbx >> + leaq EXT_C(grub_relocator_xen_paging_areas) (%rip), %r8 >> + >> 1: >> + movq 0(%r8), %r12 >> + movq 8(%r8), %rcx > > Ditto. > >> + testq %rcx, %rcx >> + jz 3f >> +2: >> movq %r12, %rdi >> - movq %rsi, %rbx >> - movq 0(%rsi), %rsi >> + shlq $12, %rdi >> + movq (%rbx, %r12, 8), %rsi > > Ditto. > >> shlq $12, %rsi >> orq $5, %rsi >> movq $2, %rdx >> @@ -83,13 +76,15 @@ VARIABLE(grub_relocator_xen_mfn_list) >> syscall >> >> movq %r9, %rcx >> - addq $8, %rbx >> - addq $4096, %r12 >> - movq %rbx, %rsi >> + incq %r12 >> + >> + loop 2b >> >> - loop 1b >> + addq $16, %r8 > > Ditto. > >> + jmp 1b >> >> - leaq LOCAL(mmu_op) (%rip), %rdi >> +3: >> + leaq EXT_C(grub_relocator_xen_mmu_op) (%rip), %rdi >> movq $3, %rsi >> movq $0, %rdx >> movq $0x7FF0, %r10 > > Ugh... Please fix this too. Probably in separate patch. I don't mind cleaning the assembler sources by using more comments and constants. I just don't think this should be part of this series. > >> @@ -104,7 +99,10 @@ VARIABLE(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 >> >> diff --git a/grub-core/lib/xen/relocator.c b/grub-core/lib/xen/relocator.c >> index 8f427d3..bc29055 100644 >> --- a/grub-core/lib/xen/relocator.c >> +++ b/grub-core/lib/xen/relocator.c >> @@ -36,15 +36,15 @@ 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 grub_xen_reg_t grub_relocator_xen_paging_areas[2 * XEN_MAX_MAPPINGS]; >> extern grub_xen_reg_t grub_relocator_xen_remap_continue; >> #ifdef __i386__ >> extern grub_xen_reg_t grub_relocator_xen_mmu_op_addr; >> +extern grub_xen_reg_t grub_relocator_xen_paging_areas_addr; >> extern grub_xen_reg_t grub_relocator_xen_remapper_map_high; >> #endif >> extern mmuext_op_t grub_relocator_xen_mmu_op[3]; >> @@ -61,6 +61,7 @@ grub_relocator_xen_boot (struct grub_relocator *rel, >> { >> grub_err_t err; >> void *relst; >> + int i; >> grub_relocator_chunk_t ch, ch_tramp; >> grub_xen_mfn_t *mfn_list = >> (grub_xen_mfn_t *) grub_xen_start_page_addr->mfn_list; >> @@ -77,8 +78,11 @@ grub_relocator_xen_boot (struct grub_relocator *rel, >> grub_relocator_xen_stack = state.stack; >> grub_relocator_xen_start_info = state.start_info; >> grub_relocator_xen_entry_point = state.entry_point; >> - grub_relocator_xen_paging_start = state.paging_start << 12; >> - grub_relocator_xen_paging_size = state.paging_size; >> + for (i = 0; i < XEN_MAX_MAPPINGS; i++) >> + { >> + grub_relocator_xen_paging_areas[2 * i] = state.paging_start[i]; >> + grub_relocator_xen_paging_areas[2 * i + 1] = state.paging_size[i]; > > Why 2 and 1? Constants? I guess I should probably define grub_relocator_xen_paging_areas as an array of struct { grub_xen_reg_t start; grub_xen_reg_t size; }; > >> + } >> grub_relocator_xen_remapper_virt = remapper_virt; >> grub_relocator_xen_remapper_virt2 = remapper_virt; >> grub_relocator_xen_remap_continue = trampoline_virt; >> @@ -88,10 +92,12 @@ grub_relocator_xen_boot (struct grub_relocator *rel, >> grub_relocator_xen_remapper_map_high = (mfn_list[remapper_pfn] >> 20); >> grub_relocator_xen_mmu_op_addr = (char *) &grub_relocator_xen_mmu_op >> - (char *) &grub_relocator_xen_remap_start + remapper_virt; >> + grub_relocator_xen_paging_areas_addr = >> + (char *) &grub_relocator_xen_paging_areas >> + - (char *) &grub_relocator_xen_remap_start + remapper_virt; >> #endif >> >> - grub_relocator_xen_mfn_list = state.mfn_list >> - + state.paging_start * sizeof (grub_addr_t); >> + grub_relocator_xen_mfn_list = state.mfn_list; >> >> grub_memset (grub_relocator_xen_mmu_op, 0, >> sizeof (grub_relocator_xen_mmu_op)); >> @@ -100,9 +106,9 @@ grub_relocator_xen_boot (struct grub_relocator *rel, >> #else >> grub_relocator_xen_mmu_op[0].cmd = MMUEXT_PIN_L4_TABLE; >> #endif >> - grub_relocator_xen_mmu_op[0].arg1.mfn = mfn_list[state.paging_start]; >> + grub_relocator_xen_mmu_op[0].arg1.mfn = mfn_list[state.paging_start[0]]; >> grub_relocator_xen_mmu_op[1].cmd = MMUEXT_NEW_BASEPTR; >> - grub_relocator_xen_mmu_op[1].arg1.mfn = mfn_list[state.paging_start]; >> + grub_relocator_xen_mmu_op[1].arg1.mfn = mfn_list[state.paging_start[0]]; >> grub_relocator_xen_mmu_op[2].cmd = MMUEXT_UNPIN_TABLE; >> grub_relocator_xen_mmu_op[2].arg1.mfn = >> mfn_list[grub_xen_start_page_addr->pt_base >> 12]; >> diff --git a/grub-core/loader/i386/xen.c b/grub-core/loader/i386/xen.c >> index 0f41048..5e10420 100644 >> --- a/grub-core/loader/i386/xen.c >> +++ b/grub-core/loader/i386/xen.c >> @@ -42,6 +42,29 @@ >> >> GRUB_MOD_LICENSE ("GPLv3+"); >> >> +#ifdef __x86_64__ >> +#define NUMBER_OF_LEVELS 4 >> +#define INTERMEDIATE_OR 7 >> +#define VIRT_MASK 0x0000ffffffffffffULL >> +#else >> +#define NUMBER_OF_LEVELS 3 >> +#define INTERMEDIATE_OR 3 >> +#define VIRT_MASK 0x00000000ffffffffULL > > Long expected constants... Nice! Just to please you. :-) They were just moved up some lines as they are used in the new structure definitions. Using constants was unavoidable here. ;-) > >> +#endif >> + >> +struct grub_xen_mapping_lvl { >> + grub_uint64_t virt_start; >> + grub_uint64_t virt_end; >> + grub_uint64_t pfn_start; >> + grub_uint64_t n_pt_pages; >> +}; >> + >> +struct grub_xen_mapping { >> + grub_uint64_t *where; >> + struct grub_xen_mapping_lvl area; >> + struct grub_xen_mapping_lvl lvls[NUMBER_OF_LEVELS]; >> +}; >> + >> static struct grub_relocator *relocator = NULL; >> static grub_uint64_t max_addr; >> static grub_dl_t my_mod; >> @@ -56,9 +79,10 @@ static struct grub_relocator_xen_state state; >> static grub_xen_mfn_t *virt_mfn_list; >> static struct start_info *virt_start_info; >> static grub_xen_mfn_t console_pfn; >> -static grub_uint64_t *virt_pgtable; >> -static grub_uint64_t pgtbl_start; >> static grub_uint64_t pgtbl_end; >> +static struct grub_xen_mapping mappings[XEN_MAX_MAPPINGS]; >> +static int n_mappings; >> +static struct grub_xen_mapping *map_reloc; >> >> #define PAGE_SIZE 4096 >> #define MAX_MODULES (PAGE_SIZE / sizeof (struct xen_multiboot_mod_list)) >> @@ -75,100 +99,162 @@ page2offset (grub_uint64_t page) >> return page << PAGE_SHIFT; >> } >> >> -#ifdef __x86_64__ >> -#define NUMBER_OF_LEVELS 4 >> -#define INTERMEDIATE_OR 7 >> -#else >> -#define NUMBER_OF_LEVELS 3 >> -#define INTERMEDIATE_OR 3 >> -#endif >> - >> -static grub_uint64_t >> -get_pgtable_size (grub_uint64_t total_pages, grub_uint64_t virt_base) >> +static grub_err_t >> +get_pgtable_size (grub_uint64_t from, grub_uint64_t to, grub_uint64_t pfn) >> { >> - if (!virt_base) >> - total_pages++; >> - grub_uint64_t ret = 0; >> - grub_uint64_t ll = total_pages; >> - int i; >> - for (i = 0; i < NUMBER_OF_LEVELS; i++) >> - { >> - ll = (ll + POINTERS_PER_PAGE - 1) >> LOG_POINTERS_PER_PAGE; >> - /* PAE wants all 4 root directories present. */ >> -#ifdef __i386__ >> - if (i == 1) >> - ll = 4; >> -#endif >> - ret += ll; >> - } >> - for (i = 1; i < NUMBER_OF_LEVELS; i++) >> - if (virt_base >> (PAGE_SHIFT + i * LOG_POINTERS_PER_PAGE)) >> - ret++; >> - return ret; >> -} >> + struct grub_xen_mapping *map, *map_cmp; >> + grub_uint64_t mask, bits; >> + int i, m; >> >> -static void >> -generate_page_table (grub_uint64_t *where, grub_uint64_t paging_start, >> - grub_uint64_t paging_end, grub_uint64_t total_pages, >> - grub_uint64_t virt_base, grub_xen_mfn_t *mfn_list) >> -{ >> - if (!virt_base) >> - paging_end++; >> + if (n_mappings == XEN_MAX_MAPPINGS) >> + return grub_error (GRUB_ERR_BUG, "too many mapped areas"); >> + >> + grub_dprintf ("xen", "get_pgtable_size %d from=%llx, to=%llx, pfn=%llx\n", >> + n_mappings, (unsigned long long) from, (unsigned long long) to, >> + (unsigned long long) pfn); >> >> - grub_uint64_t lx[NUMBER_OF_LEVELS], lxs[NUMBER_OF_LEVELS]; >> - grub_uint64_t nlx, nls, sz = 0; >> - int l; >> + map = mappings + n_mappings; >> + grub_memset (map, 0, sizeof (*map)); >> >> - nlx = paging_end; >> - nls = virt_base >> PAGE_SHIFT; >> - for (l = 0; l < NUMBER_OF_LEVELS; l++) >> + map->area.virt_start = from & VIRT_MASK; >> + map->area.virt_end = (to - 1) & VIRT_MASK; >> + map->area.n_pt_pages = 0; >> + >> + for (i = NUMBER_OF_LEVELS - 1; i >= 0; i--) >> { >> - nlx = (nlx + POINTERS_PER_PAGE - 1) >> LOG_POINTERS_PER_PAGE; >> - /* PAE wants all 4 root directories present. */ >> + map->lvls[i].pfn_start = pfn + map->area.n_pt_pages; >> + if (i == NUMBER_OF_LEVELS - 1) >> + { >> + if (n_mappings == 0) >> + { >> + map->lvls[i].virt_start = 0; >> + map->lvls[i].virt_end = VIRT_MASK; >> + map->lvls[i].n_pt_pages = 1; >> + map->area.n_pt_pages++; >> + } >> + continue; >> + } >> + >> + bits = PAGE_SHIFT + (i + 1) * LOG_POINTERS_PER_PAGE; >> + mask = (1ULL << bits) - 1; >> + map->lvls[i].virt_start = map->area.virt_start & ~mask; >> + map->lvls[i].virt_end = map->area.virt_end | mask; >> #ifdef __i386__ >> - if (l == 1) >> - nlx = 4; >> + /* PAE wants last root directory present. */ >> + if (i == 1 && to <= 0xc0000000 && n_mappings == 0) > > Ditto. > >> + map->lvls[i].virt_end = VIRT_MASK; >> #endif >> - lx[l] = nlx; >> - sz += lx[l]; >> - lxs[l] = nls & (POINTERS_PER_PAGE - 1); >> - if (nls && l != 0) >> - sz++; >> - nls >>= LOG_POINTERS_PER_PAGE; >> + for (m = 0; m < n_mappings; m++) >> + { >> + map_cmp = mappings + m; >> + if (map_cmp->lvls[i].virt_start == map_cmp->lvls[i].virt_end) >> + continue; >> + if (map->lvls[i].virt_start >= map_cmp->lvls[i].virt_start && >> + map->lvls[i].virt_end <= map_cmp->lvls[i].virt_end) >> + { >> + map->lvls[i].virt_start = 0; >> + map->lvls[i].virt_end = 0; >> + break; >> + } >> + if (map->lvls[i].virt_start >= map_cmp->lvls[i].virt_start && >> + map->lvls[i].virt_start <= map_cmp->lvls[i].virt_end) >> + map->lvls[i].virt_start = map_cmp->lvls[i].virt_end + 1; >> + if (map->lvls[i].virt_end >= map_cmp->lvls[i].virt_start && >> + map->lvls[i].virt_end <= map_cmp->lvls[i].virt_end) >> + map->lvls[i].virt_end = map_cmp->lvls[i].virt_start - 1; >> + } >> + if (map->lvls[i].virt_start < map->lvls[i].virt_end) >> + map->lvls[i].n_pt_pages = >> + ((map->lvls[i].virt_end - map->lvls[i].virt_start) >> bits) + 1; >> + map->area.n_pt_pages += map->lvls[i].n_pt_pages; >> + grub_dprintf ("xen", "get_pgtable_size level %d: virt %llx-%llx %d pts\n", >> + i, (unsigned long long) map->lvls[i].virt_start, >> + (unsigned long long) map->lvls[i].virt_end, >> + (int) map->lvls[i].n_pt_pages); >> } >> >> - grub_uint64_t lp; >> - grub_uint64_t j; >> - grub_uint64_t *pg = (grub_uint64_t *) where; >> - int pr = 0; >> + grub_dprintf ("xen", "get_pgtable_size return: %d page tables\n", >> + (int) map->area.n_pt_pages); >> + >> + state.paging_start[n_mappings] = pfn; >> + state.paging_size[n_mappings] = map->area.n_pt_pages; >> + >> + return GRUB_ERR_NONE; >> +} >> + >> +static grub_uint64_t * >> +get_pg_table_virt (int mapping, int level) >> +{ >> + grub_uint64_t pfn; >> + struct grub_xen_mapping *map; >> + >> + map = mappings + mapping; >> + pfn = map->lvls[level].pfn_start - map->lvls[NUMBER_OF_LEVELS - 1].pfn_start; >> + return map->where + pfn * POINTERS_PER_PAGE; >> +} >> >> - grub_memset (pg, 0, sz * PAGE_SIZE); >> +static grub_uint64_t >> +get_pg_table_prot (int level, grub_uint64_t pfn) >> +{ >> + int m; >> + grub_uint64_t pfn_s, pfn_e; >> >> - lp = paging_start + lx[NUMBER_OF_LEVELS - 1]; >> - for (l = NUMBER_OF_LEVELS - 1; l >= 1; l--) >> + if (level > 0) >> + return INTERMEDIATE_OR; >> + for (m = 0; m < n_mappings; m++) >> { >> - if (lxs[l] || pr) >> - pg[0] = page2offset (mfn_list[lp++]) | INTERMEDIATE_OR; >> - if (pr) >> - pg += POINTERS_PER_PAGE; >> - for (j = 0; j < lx[l - 1]; j++) >> - pg[j + lxs[l]] = page2offset (mfn_list[lp++]) | INTERMEDIATE_OR; >> - pg += lx[l] * POINTERS_PER_PAGE; >> - if (lxs[l]) >> - pr = 1; >> + pfn_s = mappings[m].lvls[NUMBER_OF_LEVELS - 1].pfn_start; >> + pfn_e = mappings[m].area.n_pt_pages + pfn_s; >> + if (pfn >= pfn_s && pfn < pfn_e) >> + return 5; >> } >> + return 7; > > What 7 and 5 means? Page table protection bits. > I am exhausted. Please fix above stuff and I will review this patch > again after fixes. I'd really like to have the maintainer's opinion on usage of constants vs. pure numbers. Up to now I have the impression that constants are used only to abstract i386 vs. x86-64. I wouldn't mind changing that, but I don't like modifying all my patches which are then rejected due to that change. Thanks for doing the review, Juergen
On Thu, Feb 11, 2016 at 03:35:45PM +0100, Juergen Gross wrote: > On 11/02/16 13:47, Daniel Kiper wrote: > > On Thu, Feb 11, 2016 at 08:53:25AM +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. > > > > I hope that older kernels could be loaded as is and everything works as expected. > > See Patch 0/6. I have tested old kernels, too. That is great! I just wanted to be sure. > >> Signed-off-by: Juergen Gross <jgross@suse.com> > >> --- > >> grub-core/lib/i386/xen/relocator.S | 47 +++--- > >> grub-core/lib/x86_64/xen/relocator.S | 44 +++-- > >> grub-core/lib/xen/relocator.c | 22 ++- > >> grub-core/loader/i386/xen.c | 313 +++++++++++++++++++++++------------ > >> include/grub/xen/relocator.h | 6 +- > >> 5 files changed, 277 insertions(+), 155 deletions(-) > >> > >> diff --git a/grub-core/lib/i386/xen/relocator.S b/grub-core/lib/i386/xen/relocator.S > >> index 694a54c..c23b405 100644 > >> --- a/grub-core/lib/i386/xen/relocator.S > >> +++ b/grub-core/lib/i386/xen/relocator.S > >> @@ -50,41 +50,45 @@ VARIABLE(grub_relocator_xen_remapper_map_high) > >> jmp *%ebx > >> > >> LOCAL(cont): > >> - xorl %eax, %eax > >> - movl %eax, %ebp > >> + /* mov imm32, %eax */ > >> + .byte 0xb8 > >> +VARIABLE(grub_relocator_xen_paging_areas_addr) > >> + .long 0 > >> + movl %eax, %ebx > >> 1: > >> - > >> + movl 0(%ebx), %ebp > >> + movl 4(%ebx), %ecx > > > > Oh... No, please use constants not plain numbers and > > explain in comments what is going on. Otherwise it > > is unreadable. > > Hmm, yes, some comments wouldn't hurt. :-) > Regarding constants: do you really think I should introduce their > first usage in this file with my patch? I would not afraid that. At least in code you change/touch. If you are not sure please ask maintainer about that. > >> + testl %ecx, %ecx > >> + jz 3f > >> + addl $8, %ebx > >> + movl %ebx, %esp > >> + > >> +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 > >> + movl %ebp, %ebx > >> + shll $12, %ebx > > > > Ditto. > > Look at the removed line above: I just switched register usage. It does not hurt to change number to constant here too. > >> movl %ecx, %edx > >> shll $12, %ecx > >> shrl $20, %edx > >> orl $5, %ecx > >> movl $2, %esi > >> movl $__HYPERVISOR_update_va_mapping, %eax > >> - int $0x82 > >> + int $0x82 /* parameters: eax, ebx, ecx, edx, esi */ > > > > Please use more verbose comments. > > :-) > > > > >> > >> incl %ebp > >> - /* mov imm32, %ecx */ > >> - .byte 0xb9 > >> -VARIABLE(grub_relocator_xen_paging_size) > >> - .long 0 > >> - cmpl %ebp, %ecx > >> + movl %edi, %ecx > >> + > >> + loop 2b > >> > >> - ja 1b > >> + mov %esp, %ebx > >> + jmp 1b > >> > >> +3: > >> /* mov imm32, %ebx */ > >> .byte 0xbb > >> VARIABLE(grub_relocator_xen_mmu_op_addr) > >> @@ -102,6 +106,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 > >> > >> diff --git a/grub-core/lib/x86_64/xen/relocator.S b/grub-core/lib/x86_64/xen/relocator.S > >> index 92e9e72..dbb90c7 100644 > >> --- a/grub-core/lib/x86_64/xen/relocator.S > >> +++ b/grub-core/lib/x86_64/xen/relocator.S > > > > Is to possible to split this patch to i386 and x86-64 stuff patches? > > I don't think so. The C-part is common and assembler sources must both > match. Yep, I am aware of that. However, I was able to that in my patch series. So, maybe it is possible here. Could you try it? > >> @@ -50,31 +50,24 @@ VARIABLE(grub_relocator_xen_remapper_map) > >> > >> LOCAL(cont): > >> > >> - /* mov imm64, %rcx */ > >> - .byte 0x48 > >> - .byte 0xb9 > >> -VARIABLE(grub_relocator_xen_paging_size) > >> - .quad 0 > >> - > >> - /* mov imm64, %rax */ > >> - .byte 0x48 > >> - .byte 0xb8 > >> -VARIABLE(grub_relocator_xen_paging_start) > >> - .quad 0 > >> - > >> - movq %rax, %r12 > >> - > >> /* mov imm64, %rax */ > >> .byte 0x48 > >> .byte 0xb8 > >> VARIABLE(grub_relocator_xen_mfn_list) > >> .quad 0 > >> > >> - movq %rax, %rsi > >> + movq %rax, %rbx > >> + leaq EXT_C(grub_relocator_xen_paging_areas) (%rip), %r8 > >> + > >> 1: > >> + movq 0(%r8), %r12 > >> + movq 8(%r8), %rcx > > > > Ditto. > > > >> + testq %rcx, %rcx > >> + jz 3f > >> +2: > >> movq %r12, %rdi > >> - movq %rsi, %rbx > >> - movq 0(%rsi), %rsi > >> + shlq $12, %rdi > >> + movq (%rbx, %r12, 8), %rsi > > > > Ditto. > > > >> shlq $12, %rsi > >> orq $5, %rsi > >> movq $2, %rdx > >> @@ -83,13 +76,15 @@ VARIABLE(grub_relocator_xen_mfn_list) > >> syscall > >> > >> movq %r9, %rcx > >> - addq $8, %rbx > >> - addq $4096, %r12 > >> - movq %rbx, %rsi > >> + incq %r12 > >> + > >> + loop 2b > >> > >> - loop 1b > >> + addq $16, %r8 > > > > Ditto. > > > >> + jmp 1b > >> > >> - leaq LOCAL(mmu_op) (%rip), %rdi > >> +3: > >> + leaq EXT_C(grub_relocator_xen_mmu_op) (%rip), %rdi > >> movq $3, %rsi > >> movq $0, %rdx > >> movq $0x7FF0, %r10 > > > > Ugh... Please fix this too. Probably in separate patch. > > I don't mind cleaning the assembler sources by using more comments > and constants. I just don't think this should be part of this series. I think that you can safely fix that issue in code you touch. I do not insist on fixing rest of the code in the same patch. However, it would be nice if you remove such worst/obvious warts at least at the end of this patch series. > >> @@ -104,7 +99,10 @@ VARIABLE(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 > >> > >> diff --git a/grub-core/lib/xen/relocator.c b/grub-core/lib/xen/relocator.c > >> index 8f427d3..bc29055 100644 > >> --- a/grub-core/lib/xen/relocator.c > >> +++ b/grub-core/lib/xen/relocator.c > >> @@ -36,15 +36,15 @@ 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 grub_xen_reg_t grub_relocator_xen_paging_areas[2 * XEN_MAX_MAPPINGS]; > >> extern grub_xen_reg_t grub_relocator_xen_remap_continue; > >> #ifdef __i386__ > >> extern grub_xen_reg_t grub_relocator_xen_mmu_op_addr; > >> +extern grub_xen_reg_t grub_relocator_xen_paging_areas_addr; > >> extern grub_xen_reg_t grub_relocator_xen_remapper_map_high; > >> #endif > >> extern mmuext_op_t grub_relocator_xen_mmu_op[3]; > >> @@ -61,6 +61,7 @@ grub_relocator_xen_boot (struct grub_relocator *rel, > >> { > >> grub_err_t err; > >> void *relst; > >> + int i; > >> grub_relocator_chunk_t ch, ch_tramp; > >> grub_xen_mfn_t *mfn_list = > >> (grub_xen_mfn_t *) grub_xen_start_page_addr->mfn_list; > >> @@ -77,8 +78,11 @@ grub_relocator_xen_boot (struct grub_relocator *rel, > >> grub_relocator_xen_stack = state.stack; > >> grub_relocator_xen_start_info = state.start_info; > >> grub_relocator_xen_entry_point = state.entry_point; > >> - grub_relocator_xen_paging_start = state.paging_start << 12; > >> - grub_relocator_xen_paging_size = state.paging_size; > >> + for (i = 0; i < XEN_MAX_MAPPINGS; i++) > >> + { > >> + grub_relocator_xen_paging_areas[2 * i] = state.paging_start[i]; > >> + grub_relocator_xen_paging_areas[2 * i + 1] = state.paging_size[i]; > > > > Why 2 and 1? Constants? > > I guess I should probably define grub_relocator_xen_paging_areas as > an array of struct { grub_xen_reg_t start; grub_xen_reg_t size; }; Make sense for me. > >> + } > >> grub_relocator_xen_remapper_virt = remapper_virt; > >> grub_relocator_xen_remapper_virt2 = remapper_virt; > >> grub_relocator_xen_remap_continue = trampoline_virt; > >> @@ -88,10 +92,12 @@ grub_relocator_xen_boot (struct grub_relocator *rel, > >> grub_relocator_xen_remapper_map_high = (mfn_list[remapper_pfn] >> 20); > >> grub_relocator_xen_mmu_op_addr = (char *) &grub_relocator_xen_mmu_op > >> - (char *) &grub_relocator_xen_remap_start + remapper_virt; > >> + grub_relocator_xen_paging_areas_addr = > >> + (char *) &grub_relocator_xen_paging_areas > >> + - (char *) &grub_relocator_xen_remap_start + remapper_virt; > >> #endif > >> > >> - grub_relocator_xen_mfn_list = state.mfn_list > >> - + state.paging_start * sizeof (grub_addr_t); > >> + grub_relocator_xen_mfn_list = state.mfn_list; > >> > >> grub_memset (grub_relocator_xen_mmu_op, 0, > >> sizeof (grub_relocator_xen_mmu_op)); > >> @@ -100,9 +106,9 @@ grub_relocator_xen_boot (struct grub_relocator *rel, > >> #else > >> grub_relocator_xen_mmu_op[0].cmd = MMUEXT_PIN_L4_TABLE; > >> #endif > >> - grub_relocator_xen_mmu_op[0].arg1.mfn = mfn_list[state.paging_start]; > >> + grub_relocator_xen_mmu_op[0].arg1.mfn = mfn_list[state.paging_start[0]]; > >> grub_relocator_xen_mmu_op[1].cmd = MMUEXT_NEW_BASEPTR; > >> - grub_relocator_xen_mmu_op[1].arg1.mfn = mfn_list[state.paging_start]; > >> + grub_relocator_xen_mmu_op[1].arg1.mfn = mfn_list[state.paging_start[0]]; > >> grub_relocator_xen_mmu_op[2].cmd = MMUEXT_UNPIN_TABLE; > >> grub_relocator_xen_mmu_op[2].arg1.mfn = > >> mfn_list[grub_xen_start_page_addr->pt_base >> 12]; > >> diff --git a/grub-core/loader/i386/xen.c b/grub-core/loader/i386/xen.c > >> index 0f41048..5e10420 100644 > >> --- a/grub-core/loader/i386/xen.c > >> +++ b/grub-core/loader/i386/xen.c > >> @@ -42,6 +42,29 @@ > >> > >> GRUB_MOD_LICENSE ("GPLv3+"); > >> > >> +#ifdef __x86_64__ > >> +#define NUMBER_OF_LEVELS 4 > >> +#define INTERMEDIATE_OR 7 > >> +#define VIRT_MASK 0x0000ffffffffffffULL > >> +#else > >> +#define NUMBER_OF_LEVELS 3 > >> +#define INTERMEDIATE_OR 3 > >> +#define VIRT_MASK 0x00000000ffffffffULL > > > > Long expected constants... Nice! > > Just to please you. :-) > They were just moved up some lines as they are used in the new > structure definitions. > > Using constants was unavoidable here. ;-) Thanks a lot! :-))) > >> +#endif > >> + > >> +struct grub_xen_mapping_lvl { > >> + grub_uint64_t virt_start; > >> + grub_uint64_t virt_end; > >> + grub_uint64_t pfn_start; > >> + grub_uint64_t n_pt_pages; > >> +}; > >> + > >> +struct grub_xen_mapping { > >> + grub_uint64_t *where; > >> + struct grub_xen_mapping_lvl area; > >> + struct grub_xen_mapping_lvl lvls[NUMBER_OF_LEVELS]; > >> +}; > >> + > >> static struct grub_relocator *relocator = NULL; > >> static grub_uint64_t max_addr; > >> static grub_dl_t my_mod; > >> @@ -56,9 +79,10 @@ static struct grub_relocator_xen_state state; > >> static grub_xen_mfn_t *virt_mfn_list; > >> static struct start_info *virt_start_info; > >> static grub_xen_mfn_t console_pfn; > >> -static grub_uint64_t *virt_pgtable; > >> -static grub_uint64_t pgtbl_start; > >> static grub_uint64_t pgtbl_end; > >> +static struct grub_xen_mapping mappings[XEN_MAX_MAPPINGS]; > >> +static int n_mappings; > >> +static struct grub_xen_mapping *map_reloc; > >> > >> #define PAGE_SIZE 4096 > >> #define MAX_MODULES (PAGE_SIZE / sizeof (struct xen_multiboot_mod_list)) > >> @@ -75,100 +99,162 @@ page2offset (grub_uint64_t page) > >> return page << PAGE_SHIFT; > >> } > >> > >> -#ifdef __x86_64__ > >> -#define NUMBER_OF_LEVELS 4 > >> -#define INTERMEDIATE_OR 7 > >> -#else > >> -#define NUMBER_OF_LEVELS 3 > >> -#define INTERMEDIATE_OR 3 > >> -#endif > >> - > >> -static grub_uint64_t > >> -get_pgtable_size (grub_uint64_t total_pages, grub_uint64_t virt_base) > >> +static grub_err_t > >> +get_pgtable_size (grub_uint64_t from, grub_uint64_t to, grub_uint64_t pfn) > >> { > >> - if (!virt_base) > >> - total_pages++; > >> - grub_uint64_t ret = 0; > >> - grub_uint64_t ll = total_pages; > >> - int i; > >> - for (i = 0; i < NUMBER_OF_LEVELS; i++) > >> - { > >> - ll = (ll + POINTERS_PER_PAGE - 1) >> LOG_POINTERS_PER_PAGE; > >> - /* PAE wants all 4 root directories present. */ > >> -#ifdef __i386__ > >> - if (i == 1) > >> - ll = 4; > >> -#endif > >> - ret += ll; > >> - } > >> - for (i = 1; i < NUMBER_OF_LEVELS; i++) > >> - if (virt_base >> (PAGE_SHIFT + i * LOG_POINTERS_PER_PAGE)) > >> - ret++; > >> - return ret; > >> -} > >> + struct grub_xen_mapping *map, *map_cmp; > >> + grub_uint64_t mask, bits; > >> + int i, m; > >> > >> -static void > >> -generate_page_table (grub_uint64_t *where, grub_uint64_t paging_start, > >> - grub_uint64_t paging_end, grub_uint64_t total_pages, > >> - grub_uint64_t virt_base, grub_xen_mfn_t *mfn_list) > >> -{ > >> - if (!virt_base) > >> - paging_end++; > >> + if (n_mappings == XEN_MAX_MAPPINGS) > >> + return grub_error (GRUB_ERR_BUG, "too many mapped areas"); > >> + > >> + grub_dprintf ("xen", "get_pgtable_size %d from=%llx, to=%llx, pfn=%llx\n", > >> + n_mappings, (unsigned long long) from, (unsigned long long) to, > >> + (unsigned long long) pfn); > >> > >> - grub_uint64_t lx[NUMBER_OF_LEVELS], lxs[NUMBER_OF_LEVELS]; > >> - grub_uint64_t nlx, nls, sz = 0; > >> - int l; > >> + map = mappings + n_mappings; > >> + grub_memset (map, 0, sizeof (*map)); > >> > >> - nlx = paging_end; > >> - nls = virt_base >> PAGE_SHIFT; > >> - for (l = 0; l < NUMBER_OF_LEVELS; l++) > >> + map->area.virt_start = from & VIRT_MASK; > >> + map->area.virt_end = (to - 1) & VIRT_MASK; > >> + map->area.n_pt_pages = 0; > >> + > >> + for (i = NUMBER_OF_LEVELS - 1; i >= 0; i--) > >> { > >> - nlx = (nlx + POINTERS_PER_PAGE - 1) >> LOG_POINTERS_PER_PAGE; > >> - /* PAE wants all 4 root directories present. */ > >> + map->lvls[i].pfn_start = pfn + map->area.n_pt_pages; > >> + if (i == NUMBER_OF_LEVELS - 1) > >> + { > >> + if (n_mappings == 0) > >> + { > >> + map->lvls[i].virt_start = 0; > >> + map->lvls[i].virt_end = VIRT_MASK; > >> + map->lvls[i].n_pt_pages = 1; > >> + map->area.n_pt_pages++; > >> + } > >> + continue; > >> + } > >> + > >> + bits = PAGE_SHIFT + (i + 1) * LOG_POINTERS_PER_PAGE; > >> + mask = (1ULL << bits) - 1; > >> + map->lvls[i].virt_start = map->area.virt_start & ~mask; > >> + map->lvls[i].virt_end = map->area.virt_end | mask; > >> #ifdef __i386__ > >> - if (l == 1) > >> - nlx = 4; > >> + /* PAE wants last root directory present. */ > >> + if (i == 1 && to <= 0xc0000000 && n_mappings == 0) > > > > Ditto. > > > >> + map->lvls[i].virt_end = VIRT_MASK; > >> #endif > >> - lx[l] = nlx; > >> - sz += lx[l]; > >> - lxs[l] = nls & (POINTERS_PER_PAGE - 1); > >> - if (nls && l != 0) > >> - sz++; > >> - nls >>= LOG_POINTERS_PER_PAGE; > >> + for (m = 0; m < n_mappings; m++) > >> + { > >> + map_cmp = mappings + m; > >> + if (map_cmp->lvls[i].virt_start == map_cmp->lvls[i].virt_end) > >> + continue; > >> + if (map->lvls[i].virt_start >= map_cmp->lvls[i].virt_start && > >> + map->lvls[i].virt_end <= map_cmp->lvls[i].virt_end) > >> + { > >> + map->lvls[i].virt_start = 0; > >> + map->lvls[i].virt_end = 0; > >> + break; > >> + } > >> + if (map->lvls[i].virt_start >= map_cmp->lvls[i].virt_start && > >> + map->lvls[i].virt_start <= map_cmp->lvls[i].virt_end) > >> + map->lvls[i].virt_start = map_cmp->lvls[i].virt_end + 1; > >> + if (map->lvls[i].virt_end >= map_cmp->lvls[i].virt_start && > >> + map->lvls[i].virt_end <= map_cmp->lvls[i].virt_end) > >> + map->lvls[i].virt_end = map_cmp->lvls[i].virt_start - 1; > >> + } > >> + if (map->lvls[i].virt_start < map->lvls[i].virt_end) > >> + map->lvls[i].n_pt_pages = > >> + ((map->lvls[i].virt_end - map->lvls[i].virt_start) >> bits) + 1; > >> + map->area.n_pt_pages += map->lvls[i].n_pt_pages; > >> + grub_dprintf ("xen", "get_pgtable_size level %d: virt %llx-%llx %d pts\n", > >> + i, (unsigned long long) map->lvls[i].virt_start, > >> + (unsigned long long) map->lvls[i].virt_end, > >> + (int) map->lvls[i].n_pt_pages); > >> } > >> > >> - grub_uint64_t lp; > >> - grub_uint64_t j; > >> - grub_uint64_t *pg = (grub_uint64_t *) where; > >> - int pr = 0; > >> + grub_dprintf ("xen", "get_pgtable_size return: %d page tables\n", > >> + (int) map->area.n_pt_pages); > >> + > >> + state.paging_start[n_mappings] = pfn; > >> + state.paging_size[n_mappings] = map->area.n_pt_pages; > >> + > >> + return GRUB_ERR_NONE; > >> +} > >> + > >> +static grub_uint64_t * > >> +get_pg_table_virt (int mapping, int level) > >> +{ > >> + grub_uint64_t pfn; > >> + struct grub_xen_mapping *map; > >> + > >> + map = mappings + mapping; > >> + pfn = map->lvls[level].pfn_start - map->lvls[NUMBER_OF_LEVELS - 1].pfn_start; > >> + return map->where + pfn * POINTERS_PER_PAGE; > >> +} > >> > >> - grub_memset (pg, 0, sz * PAGE_SIZE); > >> +static grub_uint64_t > >> +get_pg_table_prot (int level, grub_uint64_t pfn) > >> +{ > >> + int m; > >> + grub_uint64_t pfn_s, pfn_e; > >> > >> - lp = paging_start + lx[NUMBER_OF_LEVELS - 1]; > >> - for (l = NUMBER_OF_LEVELS - 1; l >= 1; l--) > >> + if (level > 0) > >> + return INTERMEDIATE_OR; > >> + for (m = 0; m < n_mappings; m++) > >> { > >> - if (lxs[l] || pr) > >> - pg[0] = page2offset (mfn_list[lp++]) | INTERMEDIATE_OR; > >> - if (pr) > >> - pg += POINTERS_PER_PAGE; > >> - for (j = 0; j < lx[l - 1]; j++) > >> - pg[j + lxs[l]] = page2offset (mfn_list[lp++]) | INTERMEDIATE_OR; > >> - pg += lx[l] * POINTERS_PER_PAGE; > >> - if (lxs[l]) > >> - pr = 1; > >> + pfn_s = mappings[m].lvls[NUMBER_OF_LEVELS - 1].pfn_start; > >> + pfn_e = mappings[m].area.n_pt_pages + pfn_s; > >> + if (pfn >= pfn_s && pfn < pfn_e) > >> + return 5; > >> } > >> + return 7; > > > > What 7 and 5 means? > > Page table protection bits. Constants or at least comments please. Yep, I am boring. I know. That is my style. > > I am exhausted. Please fix above stuff and I will review this patch > > again after fixes. > > I'd really like to have the maintainer's opinion on usage of constants > vs. pure numbers. Up to now I have the impression that constants are > used only to abstract i386 vs. x86-64. I wouldn't mind changing that, > but I don't like modifying all my patches which are then rejected due to > that change. OK, let's wait for maintainer opinion. However, I feel that using constants ease code reading. Though it does not mean that I think that every number/string should be changed to constant. We should find a proper balance and if plain numbers are used then there should be at least one line comment what this number means. > Thanks for doing the review, You are welcome. Daniel
diff --git a/grub-core/lib/i386/xen/relocator.S b/grub-core/lib/i386/xen/relocator.S index 694a54c..c23b405 100644 --- a/grub-core/lib/i386/xen/relocator.S +++ b/grub-core/lib/i386/xen/relocator.S @@ -50,41 +50,45 @@ VARIABLE(grub_relocator_xen_remapper_map_high) jmp *%ebx LOCAL(cont): - xorl %eax, %eax - movl %eax, %ebp + /* mov imm32, %eax */ + .byte 0xb8 +VARIABLE(grub_relocator_xen_paging_areas_addr) + .long 0 + movl %eax, %ebx 1: - + movl 0(%ebx), %ebp + movl 4(%ebx), %ecx + testl %ecx, %ecx + jz 3f + addl $8, %ebx + movl %ebx, %esp + +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 + movl %ebp, %ebx + shll $12, %ebx movl %ecx, %edx shll $12, %ecx shrl $20, %edx orl $5, %ecx movl $2, %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 + movl %edi, %ecx + + loop 2b - ja 1b + mov %esp, %ebx + jmp 1b +3: /* mov imm32, %ebx */ .byte 0xbb VARIABLE(grub_relocator_xen_mmu_op_addr) @@ -102,6 +106,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 diff --git a/grub-core/lib/x86_64/xen/relocator.S b/grub-core/lib/x86_64/xen/relocator.S index 92e9e72..dbb90c7 100644 --- a/grub-core/lib/x86_64/xen/relocator.S +++ b/grub-core/lib/x86_64/xen/relocator.S @@ -50,31 +50,24 @@ VARIABLE(grub_relocator_xen_remapper_map) LOCAL(cont): - /* mov imm64, %rcx */ - .byte 0x48 - .byte 0xb9 -VARIABLE(grub_relocator_xen_paging_size) - .quad 0 - - /* mov imm64, %rax */ - .byte 0x48 - .byte 0xb8 -VARIABLE(grub_relocator_xen_paging_start) - .quad 0 - - movq %rax, %r12 - /* mov imm64, %rax */ .byte 0x48 .byte 0xb8 VARIABLE(grub_relocator_xen_mfn_list) .quad 0 - movq %rax, %rsi + movq %rax, %rbx + leaq EXT_C(grub_relocator_xen_paging_areas) (%rip), %r8 + 1: + movq 0(%r8), %r12 + movq 8(%r8), %rcx + testq %rcx, %rcx + jz 3f +2: movq %r12, %rdi - movq %rsi, %rbx - movq 0(%rsi), %rsi + shlq $12, %rdi + movq (%rbx, %r12, 8), %rsi shlq $12, %rsi orq $5, %rsi movq $2, %rdx @@ -83,13 +76,15 @@ VARIABLE(grub_relocator_xen_mfn_list) syscall movq %r9, %rcx - addq $8, %rbx - addq $4096, %r12 - movq %rbx, %rsi + incq %r12 + + loop 2b - loop 1b + addq $16, %r8 + jmp 1b - leaq LOCAL(mmu_op) (%rip), %rdi +3: + leaq EXT_C(grub_relocator_xen_mmu_op) (%rip), %rdi movq $3, %rsi movq $0, %rdx movq $0x7FF0, %r10 @@ -104,7 +99,10 @@ VARIABLE(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 diff --git a/grub-core/lib/xen/relocator.c b/grub-core/lib/xen/relocator.c index 8f427d3..bc29055 100644 --- a/grub-core/lib/xen/relocator.c +++ b/grub-core/lib/xen/relocator.c @@ -36,15 +36,15 @@ 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 grub_xen_reg_t grub_relocator_xen_paging_areas[2 * XEN_MAX_MAPPINGS]; extern grub_xen_reg_t grub_relocator_xen_remap_continue; #ifdef __i386__ extern grub_xen_reg_t grub_relocator_xen_mmu_op_addr; +extern grub_xen_reg_t grub_relocator_xen_paging_areas_addr; extern grub_xen_reg_t grub_relocator_xen_remapper_map_high; #endif extern mmuext_op_t grub_relocator_xen_mmu_op[3]; @@ -61,6 +61,7 @@ grub_relocator_xen_boot (struct grub_relocator *rel, { grub_err_t err; void *relst; + int i; grub_relocator_chunk_t ch, ch_tramp; grub_xen_mfn_t *mfn_list = (grub_xen_mfn_t *) grub_xen_start_page_addr->mfn_list; @@ -77,8 +78,11 @@ grub_relocator_xen_boot (struct grub_relocator *rel, grub_relocator_xen_stack = state.stack; grub_relocator_xen_start_info = state.start_info; grub_relocator_xen_entry_point = state.entry_point; - grub_relocator_xen_paging_start = state.paging_start << 12; - grub_relocator_xen_paging_size = state.paging_size; + for (i = 0; i < XEN_MAX_MAPPINGS; i++) + { + grub_relocator_xen_paging_areas[2 * i] = state.paging_start[i]; + grub_relocator_xen_paging_areas[2 * i + 1] = state.paging_size[i]; + } grub_relocator_xen_remapper_virt = remapper_virt; grub_relocator_xen_remapper_virt2 = remapper_virt; grub_relocator_xen_remap_continue = trampoline_virt; @@ -88,10 +92,12 @@ grub_relocator_xen_boot (struct grub_relocator *rel, grub_relocator_xen_remapper_map_high = (mfn_list[remapper_pfn] >> 20); grub_relocator_xen_mmu_op_addr = (char *) &grub_relocator_xen_mmu_op - (char *) &grub_relocator_xen_remap_start + remapper_virt; + grub_relocator_xen_paging_areas_addr = + (char *) &grub_relocator_xen_paging_areas + - (char *) &grub_relocator_xen_remap_start + remapper_virt; #endif - grub_relocator_xen_mfn_list = state.mfn_list - + state.paging_start * sizeof (grub_addr_t); + grub_relocator_xen_mfn_list = state.mfn_list; grub_memset (grub_relocator_xen_mmu_op, 0, sizeof (grub_relocator_xen_mmu_op)); @@ -100,9 +106,9 @@ grub_relocator_xen_boot (struct grub_relocator *rel, #else grub_relocator_xen_mmu_op[0].cmd = MMUEXT_PIN_L4_TABLE; #endif - grub_relocator_xen_mmu_op[0].arg1.mfn = mfn_list[state.paging_start]; + grub_relocator_xen_mmu_op[0].arg1.mfn = mfn_list[state.paging_start[0]]; grub_relocator_xen_mmu_op[1].cmd = MMUEXT_NEW_BASEPTR; - grub_relocator_xen_mmu_op[1].arg1.mfn = mfn_list[state.paging_start]; + grub_relocator_xen_mmu_op[1].arg1.mfn = mfn_list[state.paging_start[0]]; grub_relocator_xen_mmu_op[2].cmd = MMUEXT_UNPIN_TABLE; grub_relocator_xen_mmu_op[2].arg1.mfn = mfn_list[grub_xen_start_page_addr->pt_base >> 12]; diff --git a/grub-core/loader/i386/xen.c b/grub-core/loader/i386/xen.c index 0f41048..5e10420 100644 --- a/grub-core/loader/i386/xen.c +++ b/grub-core/loader/i386/xen.c @@ -42,6 +42,29 @@ GRUB_MOD_LICENSE ("GPLv3+"); +#ifdef __x86_64__ +#define NUMBER_OF_LEVELS 4 +#define INTERMEDIATE_OR 7 +#define VIRT_MASK 0x0000ffffffffffffULL +#else +#define NUMBER_OF_LEVELS 3 +#define INTERMEDIATE_OR 3 +#define VIRT_MASK 0x00000000ffffffffULL +#endif + +struct grub_xen_mapping_lvl { + grub_uint64_t virt_start; + grub_uint64_t virt_end; + grub_uint64_t pfn_start; + grub_uint64_t n_pt_pages; +}; + +struct grub_xen_mapping { + grub_uint64_t *where; + struct grub_xen_mapping_lvl area; + struct grub_xen_mapping_lvl lvls[NUMBER_OF_LEVELS]; +}; + static struct grub_relocator *relocator = NULL; static grub_uint64_t max_addr; static grub_dl_t my_mod; @@ -56,9 +79,10 @@ static struct grub_relocator_xen_state state; static grub_xen_mfn_t *virt_mfn_list; static struct start_info *virt_start_info; static grub_xen_mfn_t console_pfn; -static grub_uint64_t *virt_pgtable; -static grub_uint64_t pgtbl_start; static grub_uint64_t pgtbl_end; +static struct grub_xen_mapping mappings[XEN_MAX_MAPPINGS]; +static int n_mappings; +static struct grub_xen_mapping *map_reloc; #define PAGE_SIZE 4096 #define MAX_MODULES (PAGE_SIZE / sizeof (struct xen_multiboot_mod_list)) @@ -75,100 +99,162 @@ page2offset (grub_uint64_t page) return page << PAGE_SHIFT; } -#ifdef __x86_64__ -#define NUMBER_OF_LEVELS 4 -#define INTERMEDIATE_OR 7 -#else -#define NUMBER_OF_LEVELS 3 -#define INTERMEDIATE_OR 3 -#endif - -static grub_uint64_t -get_pgtable_size (grub_uint64_t total_pages, grub_uint64_t virt_base) +static grub_err_t +get_pgtable_size (grub_uint64_t from, grub_uint64_t to, grub_uint64_t pfn) { - if (!virt_base) - total_pages++; - grub_uint64_t ret = 0; - grub_uint64_t ll = total_pages; - int i; - for (i = 0; i < NUMBER_OF_LEVELS; i++) - { - ll = (ll + POINTERS_PER_PAGE - 1) >> LOG_POINTERS_PER_PAGE; - /* PAE wants all 4 root directories present. */ -#ifdef __i386__ - if (i == 1) - ll = 4; -#endif - ret += ll; - } - for (i = 1; i < NUMBER_OF_LEVELS; i++) - if (virt_base >> (PAGE_SHIFT + i * LOG_POINTERS_PER_PAGE)) - ret++; - return ret; -} + struct grub_xen_mapping *map, *map_cmp; + grub_uint64_t mask, bits; + int i, m; -static void -generate_page_table (grub_uint64_t *where, grub_uint64_t paging_start, - grub_uint64_t paging_end, grub_uint64_t total_pages, - grub_uint64_t virt_base, grub_xen_mfn_t *mfn_list) -{ - if (!virt_base) - paging_end++; + if (n_mappings == XEN_MAX_MAPPINGS) + return grub_error (GRUB_ERR_BUG, "too many mapped areas"); + + grub_dprintf ("xen", "get_pgtable_size %d from=%llx, to=%llx, pfn=%llx\n", + n_mappings, (unsigned long long) from, (unsigned long long) to, + (unsigned long long) pfn); - grub_uint64_t lx[NUMBER_OF_LEVELS], lxs[NUMBER_OF_LEVELS]; - grub_uint64_t nlx, nls, sz = 0; - int l; + map = mappings + n_mappings; + grub_memset (map, 0, sizeof (*map)); - nlx = paging_end; - nls = virt_base >> PAGE_SHIFT; - for (l = 0; l < NUMBER_OF_LEVELS; l++) + map->area.virt_start = from & VIRT_MASK; + map->area.virt_end = (to - 1) & VIRT_MASK; + map->area.n_pt_pages = 0; + + for (i = NUMBER_OF_LEVELS - 1; i >= 0; i--) { - nlx = (nlx + POINTERS_PER_PAGE - 1) >> LOG_POINTERS_PER_PAGE; - /* PAE wants all 4 root directories present. */ + map->lvls[i].pfn_start = pfn + map->area.n_pt_pages; + if (i == NUMBER_OF_LEVELS - 1) + { + if (n_mappings == 0) + { + map->lvls[i].virt_start = 0; + map->lvls[i].virt_end = VIRT_MASK; + map->lvls[i].n_pt_pages = 1; + map->area.n_pt_pages++; + } + continue; + } + + bits = PAGE_SHIFT + (i + 1) * LOG_POINTERS_PER_PAGE; + mask = (1ULL << bits) - 1; + map->lvls[i].virt_start = map->area.virt_start & ~mask; + map->lvls[i].virt_end = map->area.virt_end | mask; #ifdef __i386__ - if (l == 1) - nlx = 4; + /* PAE wants last root directory present. */ + if (i == 1 && to <= 0xc0000000 && n_mappings == 0) + map->lvls[i].virt_end = VIRT_MASK; #endif - lx[l] = nlx; - sz += lx[l]; - lxs[l] = nls & (POINTERS_PER_PAGE - 1); - if (nls && l != 0) - sz++; - nls >>= LOG_POINTERS_PER_PAGE; + for (m = 0; m < n_mappings; m++) + { + map_cmp = mappings + m; + if (map_cmp->lvls[i].virt_start == map_cmp->lvls[i].virt_end) + continue; + if (map->lvls[i].virt_start >= map_cmp->lvls[i].virt_start && + map->lvls[i].virt_end <= map_cmp->lvls[i].virt_end) + { + map->lvls[i].virt_start = 0; + map->lvls[i].virt_end = 0; + break; + } + if (map->lvls[i].virt_start >= map_cmp->lvls[i].virt_start && + map->lvls[i].virt_start <= map_cmp->lvls[i].virt_end) + map->lvls[i].virt_start = map_cmp->lvls[i].virt_end + 1; + if (map->lvls[i].virt_end >= map_cmp->lvls[i].virt_start && + map->lvls[i].virt_end <= map_cmp->lvls[i].virt_end) + map->lvls[i].virt_end = map_cmp->lvls[i].virt_start - 1; + } + if (map->lvls[i].virt_start < map->lvls[i].virt_end) + map->lvls[i].n_pt_pages = + ((map->lvls[i].virt_end - map->lvls[i].virt_start) >> bits) + 1; + map->area.n_pt_pages += map->lvls[i].n_pt_pages; + grub_dprintf ("xen", "get_pgtable_size level %d: virt %llx-%llx %d pts\n", + i, (unsigned long long) map->lvls[i].virt_start, + (unsigned long long) map->lvls[i].virt_end, + (int) map->lvls[i].n_pt_pages); } - grub_uint64_t lp; - grub_uint64_t j; - grub_uint64_t *pg = (grub_uint64_t *) where; - int pr = 0; + grub_dprintf ("xen", "get_pgtable_size return: %d page tables\n", + (int) map->area.n_pt_pages); + + state.paging_start[n_mappings] = pfn; + state.paging_size[n_mappings] = map->area.n_pt_pages; + + return GRUB_ERR_NONE; +} + +static grub_uint64_t * +get_pg_table_virt (int mapping, int level) +{ + grub_uint64_t pfn; + struct grub_xen_mapping *map; + + map = mappings + mapping; + pfn = map->lvls[level].pfn_start - map->lvls[NUMBER_OF_LEVELS - 1].pfn_start; + return map->where + pfn * POINTERS_PER_PAGE; +} - grub_memset (pg, 0, sz * PAGE_SIZE); +static grub_uint64_t +get_pg_table_prot (int level, grub_uint64_t pfn) +{ + int m; + grub_uint64_t pfn_s, pfn_e; - lp = paging_start + lx[NUMBER_OF_LEVELS - 1]; - for (l = NUMBER_OF_LEVELS - 1; l >= 1; l--) + if (level > 0) + return INTERMEDIATE_OR; + for (m = 0; m < n_mappings; m++) { - if (lxs[l] || pr) - pg[0] = page2offset (mfn_list[lp++]) | INTERMEDIATE_OR; - if (pr) - pg += POINTERS_PER_PAGE; - for (j = 0; j < lx[l - 1]; j++) - pg[j + lxs[l]] = page2offset (mfn_list[lp++]) | INTERMEDIATE_OR; - pg += lx[l] * POINTERS_PER_PAGE; - if (lxs[l]) - pr = 1; + pfn_s = mappings[m].lvls[NUMBER_OF_LEVELS - 1].pfn_start; + pfn_e = mappings[m].area.n_pt_pages + pfn_s; + if (pfn >= pfn_s && pfn < pfn_e) + return 5; } + return 7; +} + +static void +generate_page_table (grub_xen_mfn_t *mfn_list) +{ + int l, m1, m2; + long p, p_s, p_e; + grub_uint64_t start, end, pfn; + grub_uint64_t *pg; + struct grub_xen_mapping_lvl *lvl; - if (lxs[0] || pr) - pg[0] = page2offset (mfn_list[total_pages]) | 5; - if (pr) - pg += POINTERS_PER_PAGE; + for (m1 = 0; m1 < n_mappings; m1++) + grub_memset (mappings[m1].where, 0, + mappings[m1].area.n_pt_pages * PAGE_SIZE); - for (j = 0; j < paging_end; j++) + for (l = NUMBER_OF_LEVELS - 1; l >= 0; l--) { - if (j >= paging_start && j < lp) - pg[j + lxs[0]] = page2offset (mfn_list[j]) | 5; - else - pg[j + lxs[0]] = page2offset (mfn_list[j]) | 7; + for (m1 = 0; m1 < n_mappings; m1++) + { + start = mappings[m1].lvls[l].virt_start; + end = mappings[m1].lvls[l].virt_end; + pg = get_pg_table_virt(m1, l); + for (m2 = 0; m2 < n_mappings; m2++) + { + lvl = (l > 0) ? mappings[m2].lvls + l - 1 : &mappings[m2].area; + if (l > 0 && lvl->n_pt_pages == 0) + continue; + if (lvl->virt_start >= end || lvl->virt_end <= start) + continue; + p_s = (grub_max (start, lvl->virt_start) - start) >> + (PAGE_SHIFT + l * LOG_POINTERS_PER_PAGE); + p_e = (grub_min (end, lvl->virt_end) - start) >> + (PAGE_SHIFT + l * LOG_POINTERS_PER_PAGE); + pfn = ((grub_max (start, lvl->virt_start) - lvl->virt_start) >> + (PAGE_SHIFT + l * LOG_POINTERS_PER_PAGE)) + lvl->pfn_start; + grub_dprintf ("xen", "write page table entries level %d pg %p " + "mapping %d/%d index %lx-%lx pfn %llx\n", + l, pg, m1, m2, p_s, p_e, (unsigned long long) pfn); + for (p = p_s; p <= p_e; p++) + { + pg[p] = page2offset (mfn_list[pfn]) | + get_pg_table_prot (l, pfn); + pfn++; + } + } + } } } @@ -269,39 +355,62 @@ grub_xen_pt_alloc (void) grub_relocator_chunk_t ch; grub_err_t err; grub_uint64_t nr_info_pages; - grub_uint64_t nr_pages, nr_pt_pages, nr_need_pages; + grub_uint64_t nr_need_pages; + grub_uint64_t try_virt_end; + struct grub_xen_mapping *map; - next_start.pt_base = max_addr + xen_inf.virt_base; - state.paging_start = max_addr >> PAGE_SHIFT; + map = mappings + n_mappings; + map_reloc = map + 1; + next_start.pt_base = max_addr + xen_inf.virt_base; nr_info_pages = max_addr >> PAGE_SHIFT; - nr_pages = nr_info_pages; + nr_need_pages = nr_info_pages; while (1) { - nr_pages = ALIGN_UP (nr_pages, (ALIGN_SIZE >> PAGE_SHIFT)); - nr_pt_pages = get_pgtable_size (nr_pages, xen_inf.virt_base); - nr_need_pages = - nr_info_pages + nr_pt_pages + - ((ADDITIONAL_SIZE + STACK_SIZE) >> PAGE_SHIFT); - if (nr_pages >= nr_need_pages) + try_virt_end = ALIGN_UP (xen_inf.virt_base + page2offset (nr_need_pages) + + ADDITIONAL_SIZE + STACK_SIZE, ALIGN_SIZE); + if (!xen_inf.virt_base) + try_virt_end += PAGE_SIZE; + + err = get_pgtable_size (xen_inf.virt_base, try_virt_end, nr_info_pages); + if (err) + return err; + n_mappings++; + + /* Map the relocator page either at virtual 0 or after end of area. */ + nr_need_pages = nr_info_pages + map->area.n_pt_pages; + if (xen_inf.virt_base) + err = get_pgtable_size (0, PAGE_SIZE, nr_need_pages); + else + err = get_pgtable_size (try_virt_end - PAGE_SIZE, try_virt_end, + nr_need_pages); + if (err) + return err; + nr_need_pages += map_reloc->area.n_pt_pages; + + if (xen_inf.virt_base + page2offset (nr_need_pages) <= try_virt_end) break; - nr_pages = nr_need_pages; + + n_mappings--; } - err = grub_relocator_alloc_chunk_addr (relocator, &ch, - max_addr, page2offset (nr_pt_pages)); + n_mappings++; + nr_need_pages = map->area.n_pt_pages + map_reloc->area.n_pt_pages; + err = grub_relocator_alloc_chunk_addr (relocator, &ch, max_addr, + page2offset (nr_need_pages)); if (err) return err; - virt_pgtable = get_virtual_current_address (ch); - pgtbl_start = max_addr >> PAGE_SHIFT; - max_addr += page2offset (nr_pt_pages); + map->where = get_virtual_current_address (ch); + map->area.pfn_start = 0; + max_addr += page2offset (nr_need_pages); state.stack = max_addr + STACK_SIZE + xen_inf.virt_base; - state.paging_size = nr_pt_pages; - next_start.nr_pt_frames = nr_pt_pages; - max_addr = page2offset (nr_pages); - pgtbl_end = nr_pages; + next_start.nr_pt_frames = nr_need_pages; + max_addr = try_virt_end - xen_inf.virt_base; + pgtbl_end = max_addr >> PAGE_SHIFT; + map_reloc->where = (grub_uint64_t *) ((char *) map->where + + page2offset (map->area.n_pt_pages)); return GRUB_ERR_NONE; } @@ -354,8 +463,8 @@ grub_xen_boot (void) (unsigned long long) xen_inf.virt_base, (unsigned long long) page2offset (nr_pages)); - generate_page_table (virt_pgtable, pgtbl_start, pgtbl_end, nr_pages, - xen_inf.virt_base, virt_mfn_list); + map_reloc->area.pfn_start = nr_pages; + generate_page_table (virt_mfn_list); state.entry_point = xen_inf.entry_point; diff --git a/include/grub/xen/relocator.h b/include/grub/xen/relocator.h index ae45dce..35a0ad9 100644 --- a/include/grub/xen/relocator.h +++ b/include/grub/xen/relocator.h @@ -23,11 +23,13 @@ #include <grub/err.h> #include <grub/relocator.h> +#define XEN_MAX_MAPPINGS 3 + struct grub_relocator_xen_state { grub_addr_t start_info; - grub_addr_t paging_start; - grub_addr_t paging_size; + grub_addr_t paging_start[XEN_MAX_MAPPINGS]; + grub_addr_t paging_size[XEN_MAX_MAPPINGS]; grub_addr_t mfn_list; grub_addr_t stack; grub_addr_t entry_point;
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@suse.com> --- grub-core/lib/i386/xen/relocator.S | 47 +++--- grub-core/lib/x86_64/xen/relocator.S | 44 +++-- grub-core/lib/xen/relocator.c | 22 ++- grub-core/loader/i386/xen.c | 313 +++++++++++++++++++++++------------ include/grub/xen/relocator.h | 6 +- 5 files changed, 277 insertions(+), 155 deletions(-)