Re: [Xen-devel] [PATCH v4 06/35] OvmfPkg/XenResetVector: Add new entry point for Xen PVH

From: "Roger Pau Monné" <roger.pau@citrix.com>
To: Anthony PERARD <anthony.perard@citrix.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>,
	Jordan Justen <jordan.l.justen@intel.com>,
	devel@edk2.groups.io, Julien Grall <julien.grall@arm.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	xen-devel@lists.xenproject.org, Laszlo Ersek <lersek@redhat.com>
Subject: Re: [Xen-devel] [PATCH v4 06/35] OvmfPkg/XenResetVector: Add new entry point for Xen PVH
Date: Wed, 7 Aug 2019 13:37:26 +0200	[thread overview]
Message-ID: <20190807113726.7is4txa6z3onexct@Air-de-Roger> (raw)
In-Reply-To: <20190729153944.24239-7-anthony.perard@citrix.com>

On Mon, Jul 29, 2019 at 04:39:15PM +0100, Anthony PERARD wrote:
> Add a new entry point for Xen PVH that enter directly in 32bits.
> 
> Information on the expected state of the machine when this entry point
> is used can be found at:
> https://xenbits.xenproject.org/docs/unstable/misc/pvh.html
> 
> Also, compare to the original file [1], the two `nop' of the "resetVector"
> entry point are removed. There were introduced by 8332983e2e33
> ("UefiCpuPkg: Replace the un-necessary WBINVD instruction at the reset
> vector with two NOPs in VTF0.", 2011-08-04), but don't seems to be
> useful. This is the entry point used by HVM guest (hvmloader).
> 
> [1] UefiCpuPkg/ResetVector/Vtf0/Ia16/ResetVectorVtf0.asm
> 
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1689
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> Acked-by: Laszlo Ersek <lersek@redhat.com>
> ---
> 
> Notes:
>     v4:
>     - remove the two nop in the HVM entry point
>     
>     v3:
>     - rebased, SPDX
>     - remove `cli' as via PVH the interrupts are guaranteed to be off
>     - rewrite some comments
> 
>  .../XenResetVector/Ia16/ResetVectorVtf0.asm   | 79 +++++++++++++++++++
>  OvmfPkg/XenResetVector/Ia32/XenPVHMain.asm    | 49 ++++++++++++
>  OvmfPkg/XenResetVector/XenResetVector.nasmb   |  1 +
>  3 files changed, 129 insertions(+)
>  create mode 100644 OvmfPkg/XenResetVector/Ia16/ResetVectorVtf0.asm
>  create mode 100644 OvmfPkg/XenResetVector/Ia32/XenPVHMain.asm
> 
> diff --git a/OvmfPkg/XenResetVector/Ia16/ResetVectorVtf0.asm b/OvmfPkg/XenResetVector/Ia16/ResetVectorVtf0.asm
> new file mode 100644
> index 0000000000..56749bdbc9
> --- /dev/null
> +++ b/OvmfPkg/XenResetVector/Ia16/ResetVectorVtf0.asm
> @@ -0,0 +1,79 @@
> +;------------------------------------------------------------------------------
> +; @file
> +; First code executed by processor after resetting.
> +;
> +; Copyright (c) 2008 - 2014, Intel Corporation. All rights reserved.<BR>
> +; Copyright (c) 2019, Citrix Systems, Inc.
> +;
> +; SPDX-License-Identifier: BSD-2-Clause-Patent
> +;
> +;------------------------------------------------------------------------------
> +
> +BITS    16
> +
> +ALIGN   16
> +
> +;
> +; Pad the image size to 4k when page tables are in VTF0
> +;
> +; If the VTF0 image has page tables built in, then we need to make
> +; sure the end of VTF0 is 4k above where the page tables end.
> +;
> +; This is required so the page tables will be 4k aligned when VTF0 is
> +; located just below 0x100000000 (4GB) in the firmware device.
> +;
> +%ifdef ALIGN_TOP_TO_4K_FOR_PAGING
> +    TIMES (0x1000 - ($ - EndOfPageTables) - (fourGigabytes - xenPVHEntryPoint)) DB 0
> +%endif
> +
> +BITS    32

I think you likely want an ALIGN 32 here in case
ALIGN_TOP_TO_4K_FOR_PAGING is not defined, so that xenPVHEntryPoint is
aligned to a double word boundary?

(not that it's mandatory for this to work)

The rest LGTM.

Thanks, Roger.

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