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

[Daniel Kiper]

On Mon, Feb 22, 2016 at 01:30:30PM +0100, Juergen Gross wrote:
> 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 <address@hidden>
> >>>> ---
> >>>> 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

If that is all then OK. However, IMO, it does not pays here and there.

> as they are, because they are part of instructions and alignment would
> break the code.

I am aware of that.

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

Sorry, I am not sure what do you mean by that.

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

If maintainers do not object I am not going to insist on that any longer.

Daniel