Re: [PATCH v4 10/11] xen: modify page table construction

[Juergen Gross <jgross@suse.com>]

On 22/02/16 10:17, Daniel Kiper wrote:
> On Mon, Feb 22, 2016 at 07:03:18AM +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>
>> ---
>> V4: align variables in assembly sources
>>     use separate structure define as requested by Daniel Kiper
>>
>> V3: use constants instead of numbers as requested by Daniel Kiper
>>     add lots of comments to assembly code as requested by Daniel Kiper
>> ---
>>  grub-core/lib/i386/xen/relocator.S   |  88 ++++++----
>>  grub-core/lib/x86_64/xen/relocator.S | 135 ++++++---------
>>  grub-core/lib/xen/relocator.c        |  28 ++-
>>  grub-core/loader/i386/xen.c          | 325 ++++++++++++++++++++++++-----------
>>  include/grub/i386/memory.h           |   7 +
>>  include/grub/xen/relocator.h         |   6 +-
>>  6 files changed, 359 insertions(+), 230 deletions(-)
>>
>> diff --git a/grub-core/lib/i386/xen/relocator.S b/grub-core/lib/i386/xen/relocator.S
>> index 694a54c..f8d135a 100644
>> --- a/grub-core/lib/i386/xen/relocator.S
>> +++ b/grub-core/lib/i386/xen/relocator.S
>> @@ -102,6 +112,10 @@ VARIABLE(grub_relocator_xen_remap_continue)
>>
>>  	jmp *%eax
>>
>> +	.p2align 2
> 
> I do not think this is needed but...

Even if the processor does allow unaligned loads: it's cleaner to
avoid them.

> 
>> diff --git a/grub-core/lib/x86_64/xen/relocator.S b/grub-core/lib/x86_64/xen/relocator.S
>> index 92e9e72..38ae157 100644
>> --- a/grub-core/lib/x86_64/xen/relocator.S
>> +++ b/grub-core/lib/x86_64/xen/relocator.S
>>
>>  	jmp *%rax
>>
>> -LOCAL(mmu_op):
>> +	.p2align 3
> 
> Ditto...
> 
>> diff --git a/grub-core/lib/xen/relocator.c b/grub-core/lib/xen/relocator.c
>> index 8f427d3..a05b253 100644
>> --- a/grub-core/lib/xen/relocator.c
>> +++ b/grub-core/lib/xen/relocator.c
>> @@ -29,6 +29,11 @@
>>
>>  typedef grub_addr_t grub_xen_reg_t;
>>
>> +struct grub_relocator_xen_paging_area {
>> +  grub_xen_reg_t start;
>> +  grub_xen_reg_t size;
>> +};
>> +
> 
> ... this should have GRUB_PACKED because compiler may
> add padding to align size member.

Why would the compiler add padding to a structure containing two items
of the same type? I don't think the C standard would allow this.

grub_xen_reg_t is either unsigned (32 bit) or unsigned long (64 bit).
There is no way this could require any padding.

Juergen