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

[Juergen Gross <jgross@suse.com>]

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 <jgross@suse.com>
>> ---
>> 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