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

[Juergen Gross <jgross@suse.com>]

On 22/02/16 13:18, Daniel Kiper wrote:
> On Mon, Feb 22, 2016 at 10:29:04AM +0100, Juergen Gross wrote:
>> 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.
> 
> I do not think that it pays here. However, if you are going
> to leave .p2align please align other variables too.

Which?

The only candidate would be grub_relocator_xen_mmu_op which already
is aligned due to above p2align. All other variables must be kept
as they are, because they are part of instructions and alignment would
break the code.

> 
>>>> 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.
> 
> You are right but we should add this here just in case.

Sorry, I don't think this makes any sense. The C standard is very clear
in this case: a type requiring a special alignment has always a length
being a multiple of that alignment. Otherwise arrays wouldn't work.
Adding GRUB_PACKED would make the code less clear IMO. Finding such a
qualifier in some code I want to modify would make me search for the
reason for it which isn't existing.

Juergen