| Message ID | 1456245085-2302-4-git-send-email-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 23.02.16 at 17:31, <andrew.cooper3@citrix.com> wrote: > --- > xen/arch/x86/boot/head.S | 18 +++++------------- > xen/arch/x86/boot/x86_64.S | 19 +++++++++++++++++++ > xen/arch/x86/x86_64/mm.c | 4 ---- > 3 files changed, 24 insertions(+), 17 deletions(-) Is this intentionally leaving the EFI equivalent untouched? > --- a/xen/arch/x86/boot/x86_64.S > +++ b/xen/arch/x86/boot/x86_64.S > @@ -184,3 +184,22 @@ GLOBAL(idle_pg_table) > .size idle_pg_table, . - idle_pg_table > > GLOBAL(__page_tables_end) > + > +/* Init pagetables. Enough page directories to map into the bottom 1GB. */ > + .section .init.data, "a", @progbits > + .align PAGE_SIZE, 0 > + > +GLOBAL(l2_bootmap) > + .quad sym_phys(l1_identmap) + __PAGE_HYPERVISOR The relocation needed for this and ... > + idx = 1 > + .rept 7 > + .quad (idx << L2_PAGETABLE_SHIFT) | __PAGE_HYPERVISOR | _PAGE_PSE > + idx = idx + 1 > + .endr > + .fill L2_PAGETABLE_ENTRIES - 8, 8, 0 > + .size l2_bootmap, . - l2_bootmap > + > +GLOBAL(l3_bootmap) > + .quad sym_phys(l2_bootmap) + __PAGE_HYPERVISOR ... this won't get properly adjusted by efi_arch_relocate_image(), due to living outside of [__page_tables_start, __page_tables_end). Jan
On 24/02/16 11:34, Jan Beulich wrote: >>>> On 23.02.16 at 17:31, <andrew.cooper3@citrix.com> wrote: >> --- >> xen/arch/x86/boot/head.S | 18 +++++------------- >> xen/arch/x86/boot/x86_64.S | 19 +++++++++++++++++++ >> xen/arch/x86/x86_64/mm.c | 4 ---- >> 3 files changed, 24 insertions(+), 17 deletions(-) > Is this intentionally leaving the EFI equivalent untouched? Yes. > >> --- a/xen/arch/x86/boot/x86_64.S >> +++ b/xen/arch/x86/boot/x86_64.S >> @@ -184,3 +184,22 @@ GLOBAL(idle_pg_table) >> .size idle_pg_table, . - idle_pg_table >> >> GLOBAL(__page_tables_end) >> + >> +/* Init pagetables. Enough page directories to map into the bottom 1GB. */ >> + .section .init.data, "a", @progbits >> + .align PAGE_SIZE, 0 >> + >> +GLOBAL(l2_bootmap) >> + .quad sym_phys(l1_identmap) + __PAGE_HYPERVISOR > The relocation needed for this and ... > >> + idx = 1 >> + .rept 7 >> + .quad (idx << L2_PAGETABLE_SHIFT) | __PAGE_HYPERVISOR | _PAGE_PSE >> + idx = idx + 1 >> + .endr >> + .fill L2_PAGETABLE_ENTRIES - 8, 8, 0 >> + .size l2_bootmap, . - l2_bootmap >> + >> +GLOBAL(l3_bootmap) >> + .quad sym_phys(l2_bootmap) + __PAGE_HYPERVISOR > ... this won't get properly adjusted by efi_arch_relocate_image(), > due to living outside of [__page_tables_start, __page_tables_end). Deliberately so. The EFI needs to relocate the tables anyway. It currently (re)writes them fresh at the relocated address, and leaving this be is the more simple option. ~Andrew
>>> On 24.02.16 at 12:40, <andrew.cooper3@citrix.com> wrote: > On 24/02/16 11:34, Jan Beulich wrote: >>>>> On 23.02.16 at 17:31, <andrew.cooper3@citrix.com> wrote: >>> --- >>> xen/arch/x86/boot/head.S | 18 +++++------------- >>> xen/arch/x86/boot/x86_64.S | 19 +++++++++++++++++++ >>> xen/arch/x86/x86_64/mm.c | 4 ---- >>> 3 files changed, 24 insertions(+), 17 deletions(-) >> Is this intentionally leaving the EFI equivalent untouched? > > Yes. > >> >>> --- a/xen/arch/x86/boot/x86_64.S >>> +++ b/xen/arch/x86/boot/x86_64.S >>> @@ -184,3 +184,22 @@ GLOBAL(idle_pg_table) >>> .size idle_pg_table, . - idle_pg_table >>> >>> GLOBAL(__page_tables_end) >>> + >>> +/* Init pagetables. Enough page directories to map into the bottom 1GB. */ >>> + .section .init.data, "a", @progbits >>> + .align PAGE_SIZE, 0 >>> + >>> +GLOBAL(l2_bootmap) >>> + .quad sym_phys(l1_identmap) + __PAGE_HYPERVISOR >> The relocation needed for this and ... >> >>> + idx = 1 >>> + .rept 7 >>> + .quad (idx << L2_PAGETABLE_SHIFT) | __PAGE_HYPERVISOR | _PAGE_PSE >>> + idx = idx + 1 >>> + .endr >>> + .fill L2_PAGETABLE_ENTRIES - 8, 8, 0 >>> + .size l2_bootmap, . - l2_bootmap >>> + >>> +GLOBAL(l3_bootmap) >>> + .quad sym_phys(l2_bootmap) + __PAGE_HYPERVISOR >> ... this won't get properly adjusted by efi_arch_relocate_image(), >> due to living outside of [__page_tables_start, __page_tables_end). > > Deliberately so. > > The EFI needs to relocate the tables anyway. It currently (re)writes > them fresh at the relocated address, and leaving this be is the more > simple option. Okay, I guess you mean to say that both together continue to work, which without it being said explicitly (allowing the implication that this also has got tested) I was rather unsure about. Hence I'd like to at least ask for an explicit respective statement in the commit message. Jan
On 24/02/16 11:50, Jan Beulich wrote: >>>> On 24.02.16 at 12:40, <andrew.cooper3@citrix.com> wrote: >> On 24/02/16 11:34, Jan Beulich wrote: >>>>>> On 23.02.16 at 17:31, <andrew.cooper3@citrix.com> wrote: >>>> --- >>>> xen/arch/x86/boot/head.S | 18 +++++------------- >>>> xen/arch/x86/boot/x86_64.S | 19 +++++++++++++++++++ >>>> xen/arch/x86/x86_64/mm.c | 4 ---- >>>> 3 files changed, 24 insertions(+), 17 deletions(-) >>> Is this intentionally leaving the EFI equivalent untouched? >> Yes. >> >>>> --- a/xen/arch/x86/boot/x86_64.S >>>> +++ b/xen/arch/x86/boot/x86_64.S >>>> @@ -184,3 +184,22 @@ GLOBAL(idle_pg_table) >>>> .size idle_pg_table, . - idle_pg_table >>>> >>>> GLOBAL(__page_tables_end) >>>> + >>>> +/* Init pagetables. Enough page directories to map into the bottom 1GB. */ >>>> + .section .init.data, "a", @progbits >>>> + .align PAGE_SIZE, 0 >>>> + >>>> +GLOBAL(l2_bootmap) >>>> + .quad sym_phys(l1_identmap) + __PAGE_HYPERVISOR >>> The relocation needed for this and ... >>> >>>> + idx = 1 >>>> + .rept 7 >>>> + .quad (idx << L2_PAGETABLE_SHIFT) | __PAGE_HYPERVISOR | _PAGE_PSE >>>> + idx = idx + 1 >>>> + .endr >>>> + .fill L2_PAGETABLE_ENTRIES - 8, 8, 0 >>>> + .size l2_bootmap, . - l2_bootmap >>>> + >>>> +GLOBAL(l3_bootmap) >>>> + .quad sym_phys(l2_bootmap) + __PAGE_HYPERVISOR >>> ... this won't get properly adjusted by efi_arch_relocate_image(), >>> due to living outside of [__page_tables_start, __page_tables_end). >> Deliberately so. >> >> The EFI needs to relocate the tables anyway. It currently (re)writes >> them fresh at the relocated address, and leaving this be is the more >> simple option. > Okay, I guess you mean to say that both together continue to > work, which without it being said explicitly (allowing the implication > that this also has got tested) I was rather unsure about. Hence > I'd like to at least ask for an explicit respective statement in the > commit message. Ok. The other reason why leaving the EFI side along is that these tables can't both be in .init.data and covered by [__page_tables_start, __page_tables_end). ~Andrew
diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S index ac4962b..f3501fd 100644 --- a/xen/arch/x86/boot/head.S +++ b/xen/arch/x86/boot/head.S @@ -150,21 +150,13 @@ __start: mov %eax,sym_phys(boot_tsc_stamp) mov %edx,sym_phys(boot_tsc_stamp+4) - /* Initialise L2 boot-map page table entries (16MB). */ - mov $sym_phys(l2_bootmap),%edx - mov $PAGE_HYPERVISOR|_PAGE_PSE,%eax - mov $8,%ecx -1: mov %eax,(%edx) - add $8,%edx - add $(1<<L2_PAGETABLE_SHIFT),%eax - loop 1b - /* Initialise L3 boot-map page directory entry. */ - mov $sym_phys(l2_bootmap)+__PAGE_HYPERVISOR,%eax - mov %eax,sym_phys(l3_bootmap) + 0*8 - /* Hook 4kB mappings of first 2MB of memory into L2. */ + /* + * During boot, hook 4kB mappings of first 2MB of memory into L2. + * This avoids mixing cachability for the legacy VGA region, and is + * corrected when Xen relocates itself. + */ mov $sym_phys(l1_identmap)+__PAGE_HYPERVISOR,%edi mov %edi,sym_phys(l2_xenmap) - mov %edi,sym_phys(l2_bootmap) /* Apply relocations to bootstrap trampoline. */ mov sym_phys(trampoline_phys),%edx diff --git a/xen/arch/x86/boot/x86_64.S b/xen/arch/x86/boot/x86_64.S index cc10932..4fea3f0 100644 --- a/xen/arch/x86/boot/x86_64.S +++ b/xen/arch/x86/boot/x86_64.S @@ -184,3 +184,22 @@ GLOBAL(idle_pg_table) .size idle_pg_table, . - idle_pg_table GLOBAL(__page_tables_end) + +/* Init pagetables. Enough page directories to map into the bottom 1GB. */ + .section .init.data, "a", @progbits + .align PAGE_SIZE, 0 + +GLOBAL(l2_bootmap) + .quad sym_phys(l1_identmap) + __PAGE_HYPERVISOR + idx = 1 + .rept 7 + .quad (idx << L2_PAGETABLE_SHIFT) | __PAGE_HYPERVISOR | _PAGE_PSE + idx = idx + 1 + .endr + .fill L2_PAGETABLE_ENTRIES - 8, 8, 0 + .size l2_bootmap, . - l2_bootmap + +GLOBAL(l3_bootmap) + .quad sym_phys(l2_bootmap) + __PAGE_HYPERVISOR + .fill L3_PAGETABLE_ENTRIES - 1, 8, 0 + .size l3_bootmap, . - l3_bootmap diff --git a/xen/arch/x86/x86_64/mm.c b/xen/arch/x86/x86_64/mm.c index 2228898..e07e69e 100644 --- a/xen/arch/x86/x86_64/mm.c +++ b/xen/arch/x86/x86_64/mm.c @@ -42,10 +42,6 @@ asm(".file \"" __FILE__ "\""); unsigned int __read_mostly m2p_compat_vstart = __HYPERVISOR_COMPAT_VIRT_START; -/* Enough page directories to map into the bottom 1GB. */ -l3_pgentry_t __section(".bss.page_aligned") l3_bootmap[L3_PAGETABLE_ENTRIES]; -l2_pgentry_t __section(".bss.page_aligned") l2_bootmap[L2_PAGETABLE_ENTRIES]; - l2_pgentry_t *compat_idle_pg_table_l2; void *do_page_walk(struct vcpu *v, unsigned long addr)
... rather than at runtime. The bootmaps are discarded in zap_low_mappings(), so the tables themselves can live in .init.data and be reclaimed after boot. Hooking the l1_identmap into l2_xenmap stays for safety, along with a longer comment explaining why. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> New in v2 --- xen/arch/x86/boot/head.S | 18 +++++------------- xen/arch/x86/boot/x86_64.S | 19 +++++++++++++++++++ xen/arch/x86/x86_64/mm.c | 4 ---- 3 files changed, 24 insertions(+), 17 deletions(-)