Re: [Xen-devel] [PATCH 2/6] xen/pvh: place the trampoline at page 0x1

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, Jan 17, 2018 at 10:55:22AM +0000, Wei Liu wrote:
> On Wed, Jan 17, 2018 at 09:48:10AM +0000, Roger Pau Monne wrote:
> > diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
> > index 4fe5a776b1..7829e3f07c 100644
> > --- a/xen/arch/x86/boot/head.S
> > +++ b/xen/arch/x86/boot/head.S
> > @@ -411,6 +411,9 @@ __pvh_start:
> >          /* Skip bootloader setup and bios setup, go straight to trampoline 
> > */
> >          movb    $1, sym_esi(pvh_boot)
> >          movb    $1, sym_esi(skip_realmode)
> > +
> > +        /* Set trampoline_phys to use page 1. */
> 
> Could you please add the rationale here -- to avoid having VA 0 mapped.

Sure.

> > +        movw    $0x1000, sym_esi(trampoline_phys)
> >          jmp     trampoline_setup
> >  
> >  #endif /* CONFIG_PVH_GUEST */
> > diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> > index 1147a1afb1..586af2ba9a 100644
> > --- a/xen/arch/x86/mm.c
> > +++ b/xen/arch/x86/mm.c
> > @@ -292,9 +292,13 @@ void __init arch_init_memory(void)
> >      /*
> >       * First 1MB of RAM is historically marked as I/O.  If we booted PVH,
> >       * reclaim the space.  Irrespective, leave MFN 0 as special for the 
> > sake
> > -     * of 0 being a very common default value.
> > +     * of 0 being a very common default value. Also reserve page 0x1 which 
> > is
> > +     * used by the trampoline code.
> >       */
> > -    for ( i = 0; i < (pvh_boot ? 1 : 0x100); i++ )
> > +    for ( i = 0;
> > +          i < (pvh_boot ? (1 + PFN_UP(trampoline_end - trampoline_start))
> > +                        : 0x100);
> > +          i++ )
> 
> There is now a hardcoded assumption that the trampoline is mapped at
> page 0x1. Maybe there should be a dedicated loop to share the trampoline
> with dom_io?

I could add a loop like:

for ( i = PFN_DOWN(trampoline_phys);
      i < (pvh_boot ? (PFN_UP(trampoline_phys + trampoline_end - 
trampoline_start))
                    : 0);
      i++ )

But it seems quite redundant.

The comment above the loop already explains the position of the
trampoline for PVH, do you think it would be fine to add something
like:

BUG_ON(pvh_boot && trampoline_phys != 0x1000);

Just before the loop?

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel