[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v2 4/5] x86/boot: Simplify pagetable manipulation loops
On 20/01/2020 10:46, Jan Beulich wrote: > On 17.01.2020 21:42, Andrew Cooper wrote: >> For __page_tables_{start,end} and L3 bootmap initialisation, the logic is >> unnecesserily complicated owing to its attempt to use the LOOP instruction, >> which results in an off-by-8 memory address owing to LOOP's termination >> condition. >> >> Rewrite both loops for improved clarity and speed. >> >> Misc notes: >> * TEST $IMM, MEM can't macrofuse. The loop has 0x1200 iterations, so pull >> the $_PAGE_PRESENT constant out into a spare register to turn the TEST >> into >> its %REG, MEM form, which can macrofuse. >> * Avoid the use of %fs-relative references. %esi-relative is the more >> common >> form in the code, and doesn't suffer an address generation overhead. >> * Avoid LOOP. CMP/JB isn't microcoded and faster to execute in all cases. >> * For a 4 interation trivial loop, even compilers unroll these. The >> generated code size is a fraction larger, but this is init and the asm is >> far easier to follow. >> * Reposition the l2=>l1 bootmap construction so the asm reads in pagetable >> level order. >> >> No functional change. >> >> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx> > Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx> > with two remarks/questions, but leaving it up to you whether > you want to adjust the code: > >> --- a/xen/arch/x86/boot/head.S >> +++ b/xen/arch/x86/boot/head.S >> @@ -662,11 +662,17 @@ trampoline_setup: >> mov %edx,sym_fs(boot_tsc_stamp)+4 >> >> /* Relocate pagetables to point at Xen's current location in >> memory. */ >> - mov $((__page_tables_end-__page_tables_start)/8),%ecx >> -1: testl $_PAGE_PRESENT,sym_fs(__page_tables_start)-8(,%ecx,8) >> + mov $_PAGE_PRESENT, %edx >> + lea sym_esi(__page_tables_start), %eax >> + lea sym_esi(__page_tables_end), %edi >> + >> +1: testb %dl, (%eax) /* if page present */ > When it's an immediate, using TESTB is generally helpful because > there's no (sign- or whatever-)extended immediate form of it. > When using a register, I think it would generally be better to > use native size, even if for register reads the partial register > access penalty may (today) be zero. I don't think it is plausible that partial access penalties will be introduced. Partial merge penalties occur as a consequence of making register reads consistent under renaming, and implicit zeroing behaviour exists to remove merge penalties. Any 32bit or larger register write results in allocating a fresh physical register entry, filling it with the data provided, and updating the register allocation table. For 16bit or 8bit writes, either the physical register file needs to support RMW updates to an architectural register, or an extra set of uops are needed to perform the merge in the pipeline itself, before making a 32bit writeback. What matters in this case is the size of the memory access, and whether 8bit vs 32bit within the same cache line will ever be different. However, we should switch to a 32bit access here, so we don't intermix an 8bit read with a 32bit RMW. Memory disambiguation speculation will have an easier time of it on some parts, which will make an overall difference. > >> @@ -701,22 +707,27 @@ trampoline_setup: >> cmp %edx, %ecx >> jbe 1b >> >> - /* Initialize L3 boot-map page directory entries. */ >> - lea >> __PAGE_HYPERVISOR+(L2_PAGETABLE_ENTRIES*8)*3+sym_esi(l2_bootmap),%eax >> - mov $4,%ecx >> -1: mov %eax,sym_fs(l3_bootmap)-8(,%ecx,8) >> - sub $(L2_PAGETABLE_ENTRIES*8),%eax >> - loop 1b >> - >> - /* Map the permanent trampoline page into l{1,2}_bootmap[]. */ >> + /* Map 4x l2_bootmap[] into l3_bootmap[0...3] */ >> + lea __PAGE_HYPERVISOR + sym_esi(l2_bootmap), %eax >> + mov $PAGE_SIZE, %edx >> + mov %eax, 0 + sym_esi(l3_bootmap) >> + add %edx, %eax >> + mov %eax, 8 + sym_esi(l3_bootmap) >> + add %edx, %eax >> + mov %eax, 16 + sym_esi(l3_bootmap) >> + add %edx, %eax >> + mov %eax, 24 + sym_esi(l3_bootmap) > It took me a moment to realize the code is correct despite there > not being any mention of PAGE_SIZE between each of the MOVs. As > you don't view code size as a (primary) concern, perhaps worth > using > > add $PAGE_SIZE, %eax > > everywhere, the more that this has a special, ModR/M-less > encoding? I had it that way first time around. Sadly, $PAGE_SIZE can't be expressed as imm8, which is why I switched to using %edx. I'm not overly fussed either way, so given the confusion, I'll switch back to this form. ~Andrew _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxxxxxxxxx https://lists.xenproject.org/mailman/listinfo/xen-devel
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |