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

[Juergen Gross]

On 18/02/16 17:40, Daniel Kiper wrote:
> 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 <address@hidden>
>> ---
>> V3: use constants instead of numbers as requested by Daniel Kiper
>>     add lots of comments to assembly code as requested by Daniel Kiper

...

>>  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
>>

This would require to open code the loop statement here with %r9 as
count register.

...

>> 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?

Why packed? Aah, I think I should align the variable in the assembly
source instead.

Regarding type: sure I could.


Juergen