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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 17.01.18 at 13:00, <roger.pau@xxxxxxxxxx> wrote:
> Since PVH guest jump straight into trampoline_setup trampoline_phys is
> not initialized, thus the trampoline is relocated to address 0.
> 
> This works, but has the undesirable effect of having VA 0 mapped to
> MFN 0, which means NULL pointed dereferences no longer trigger a page
> fault.
> 
> In order to solve this, place the trampoline at page 0x1 and reserve
> the memory used by it.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> ---
> Cc: Jan Beulich <jbeulich@xxxxxxxx>
> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Cc: Wei Liu <wei.liu2@xxxxxxxxxx>
> Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> ---
> Changes since v1:
>  - Expand comment in head.S.
>  - Add a BUG_ON to check trampoline_phys value in the PVH case.
> ---
> Should be backported to the 4.10.0-shim-comet branch.
> ---
>  xen/arch/x86/boot/head.S | 3 +++
>  xen/arch/x86/mm.c        | 9 +++++++--
>  2 files changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
> index 4fe5a776b1..0f652cea11 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 mfn 1 to avoid having a mapping at VA 
> 0 */
> +        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..356f939f64 100644
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -292,9 +292,14 @@ 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

This isn't in line with ...

> +     * used by the trampoline code on PVH.
>       */
> -    for ( i = 0; i < (pvh_boot ? 1 : 0x100); i++ )
> +    BUG_ON(pvh_boot && trampoline_phys != 0x1000);
> +    for ( i = 0;
> +          i < (pvh_boot ? (1 + PFN_UP(trampoline_end - trampoline_start))

... this. How about having the comment say "Also reserve pages
which are used ..."? Could certainly be addressed while committing;
with this or some similar wording
Acked-by: Jan Beulich <jbeulich@xxxxxxxx>

Jan

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