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

[Daniel Kiper <daniel.kiper@xxxxxxxxxx>]

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

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

Daniel

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel