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

2019-04-15 Thread Andrew Cooper 
On 15/04/2019 12:25, Anthony PERARD wrote:
> On Thu, Apr 11, 2019 at 01:52:27PM +0100, Andrew Cooper wrote:
>> On 09/04/2019 12:08, Anthony PERARD wrote:
>>> diff --git a/OvmfPkg/XenResetVector/Ia32/XenPVHMain.asm 
>>> b/OvmfPkg/XenResetVector/Ia32/XenPVHMain.asm
>>> new file mode 100644
>>> index 00..c4802bf4d1
>>> --- /dev/null
>>> +++ b/OvmfPkg/XenResetVector/Ia32/XenPVHMain.asm
>>> @@ -0,0 +1,47 @@
>>> +;--
>>> +; @file
>>> +; An entry point use by Xen when a guest is started in PVH mode.
>>> +;
>>> +; Copyright (c) 2019, Citrix Systems, Inc.
>>> +;
>>> +; This program and the accompanying materials are licensed and made 
>>> available
>>> +; under the terms and conditions of the BSD License which accompanies this
>>> +; distribution.  The full text of the license may be found at
>>> +; http://opensource.org/licenses/bsd-license.php
>>> +;
>>> +; THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, 
>>> WITHOUT
>>> +; WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>>> +;
>>> +;--
>>> +
>>> +BITS32
>>> +
>>> +xenPVHMain:
>>> +mov di, 'BP'
>>> +
>>> +; ESP -  Initial value of the EAX register (BIST: Built-in Self Test)
>>> +mov esp, eax
>> Where is the ABI described?  Xen has no BIST, so this will always have
>> the value 0. Stashing it in the stack pointer seems like a weird choice,
>> and a recipe for subtle bugs.
> I've just copied over what was done in the HVM case. I'll change that
> and ignore the value in `eax'.

The ASCII BP in %di is also bizarre without an explanation of where it
is intended to be used.

>>> +
>>> +mov eax, SEC_DEFAULT_CR0
>>> +mov cr0, eax
>>> +
>>> +jmp LINEAR_CODE_SEL:ADDR_OF(.jmpToNewCodeSeg)
>>> +.jmpToNewCodeSeg:
>>> +
>>> +mov eax, SEC_DEFAULT_CR4
>>> +mov cr4, eax
>>> +
>>> +mov ax, LINEAR_SEL
>>> +mov ds, ax
>>> +mov es, ax
>>> +mov fs, ax
>>> +mov gs, ax
>>> +mov ss, ax
>>> +
>>> +; return to the Main16
>>> +OneTimeCallRet TransitionFromReal16To32BitFlat
>> Is there any description of what OneTimeCallRet is, and why a simple jmp
>> wont do?
> That's is used to return to where the `OneTimeCall' macro has been
> used. In this assembly, they don't use `call' and `ret', instead there
> is a set of two macros:
>
> %macro  OneTimeCall 1
> jmp %1
> %1 %+ OneTimerCallReturn:
> %endmacro
>
> %macro  OneTimeCallRet 1
> jmp %1 %+ OneTimerCallReturn
> %endmacro

I found these, but the complete lack of any documentation makes them
entirely opaque, especially as they are not actually call/ret instructions.

Even if they are suppose to be used in matching pairs, OneTimeCall
hasn't been used so far in the PVH code, so where is this going to
actually go to?

~Andrew

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

Re: [edk2-devel] [Xen-devel] [PATCH v2 06/31] OvmfPkg/XenResetVector: Add new entry point for Xen PVH

2019-04-15 Thread Anthony PERARD 
On Thu, Apr 11, 2019 at 01:52:27PM +0100, Andrew Cooper wrote:
> On 09/04/2019 12:08, Anthony PERARD wrote:
> > diff --git a/OvmfPkg/XenResetVector/Ia32/XenPVHMain.asm 
> > b/OvmfPkg/XenResetVector/Ia32/XenPVHMain.asm
> > new file mode 100644
> > index 00..c4802bf4d1
> > --- /dev/null
> > +++ b/OvmfPkg/XenResetVector/Ia32/XenPVHMain.asm
> > @@ -0,0 +1,47 @@
> > +;--
> > +; @file
> > +; An entry point use by Xen when a guest is started in PVH mode.
> > +;
> > +; Copyright (c) 2019, Citrix Systems, Inc.
> > +;
> > +; This program and the accompanying materials are licensed and made 
> > available
> > +; under the terms and conditions of the BSD License which accompanies this
> > +; distribution.  The full text of the license may be found at
> > +; http://opensource.org/licenses/bsd-license.php
> > +;
> > +; THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, 
> > WITHOUT
> > +; WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> > +;
> > +;--
> > +
> > +BITS32
> > +
> > +xenPVHMain:
> > +mov di, 'BP'
> > +
> > +; ESP -  Initial value of the EAX register (BIST: Built-in Self Test)
> > +mov esp, eax
> 
> Where is the ABI described?  Xen has no BIST, so this will always have
> the value 0. Stashing it in the stack pointer seems like a weird choice,
> and a recipe for subtle bugs.

I've just copied over what was done in the HVM case. I'll change that
and ignore the value in `eax'.

> > +
> > +cli
> 
> Interrupts are guaranteed to be off at this point.

OK, I'll remove the instruction.

> > +
> > +mov ebx, ADDR_OF(gdtr)
> > +lgdt[ebx]
> 
> lgdt ADDR_OF(gdtr), presumably?
> 
> This is 32bit code - there is no need for any indirection through
> registers for memory operands.

I'll change this, if it works.

> > +
> > +mov eax, SEC_DEFAULT_CR0
> > +mov cr0, eax
> > +
> > +jmp LINEAR_CODE_SEL:ADDR_OF(.jmpToNewCodeSeg)
> > +.jmpToNewCodeSeg:
> > +
> > +mov eax, SEC_DEFAULT_CR4
> > +mov cr4, eax
> > +
> > +mov ax, LINEAR_SEL
> > +mov ds, ax
> > +mov es, ax
> > +mov fs, ax
> > +mov gs, ax
> > +mov ss, ax
> > +
> > +; return to the Main16
> > +OneTimeCallRet TransitionFromReal16To32BitFlat
> 
> Is there any description of what OneTimeCallRet is, and why a simple jmp
> wont do?

That's is used to return to where the `OneTimeCall' macro has been
used. In this assembly, they don't use `call' and `ret', instead there
is a set of two macros:

%macro  OneTimeCall 1
jmp %1
%1 %+ OneTimerCallReturn:
%endmacro

%macro  OneTimeCallRet 1
jmp %1 %+ OneTimerCallReturn
%endmacro


> Irrespective of that, you're moving to a function whose name suggests it
> is in 16bit mode, while you are currently in 32bit flat mode. 
> (SEC_DEFAULT_CR0 has PE set, and LINEAR_SEL is 32bit flat.  This clearly
> isn't correct, but surely we want to skip all the 16bit setup, as well.

I think I need to change the comment. Main16 is just a label name for
something that makes several "calls" and transition from 16bits to 32 or
64 before calling the next module in the firmware.

That return simply jmp to the part of the main where we are supposed to
be in 32bit flat mode.

Thanks for the review,

-- 
Anthony PERARD


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#39075): https://edk2.groups.io/g/devel/message/39075
Mute This Topic: https://groups.io/mt/31030915/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

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

2019-04-11 Thread Andrew Cooper 
On 09/04/2019 12:08, Anthony PERARD wrote:
> diff --git a/OvmfPkg/XenResetVector/Ia32/XenPVHMain.asm 
> b/OvmfPkg/XenResetVector/Ia32/XenPVHMain.asm
> new file mode 100644
> index 00..c4802bf4d1
> --- /dev/null
> +++ b/OvmfPkg/XenResetVector/Ia32/XenPVHMain.asm
> @@ -0,0 +1,47 @@
> +;--
> +; @file
> +; An entry point use by Xen when a guest is started in PVH mode.
> +;
> +; Copyright (c) 2019, Citrix Systems, Inc.
> +;
> +; This program and the accompanying materials are licensed and made available
> +; under the terms and conditions of the BSD License which accompanies this
> +; distribution.  The full text of the license may be found at
> +; http://opensource.org/licenses/bsd-license.php
> +;
> +; THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, 
> WITHOUT
> +; WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> +;
> +;--
> +
> +BITS32
> +
> +xenPVHMain:
> +mov di, 'BP'
> +
> +; ESP -  Initial value of the EAX register (BIST: Built-in Self Test)
> +mov esp, eax

Where is the ABI described?  Xen has no BIST, so this will always have
the value 0. Stashing it in the stack pointer seems like a weird choice,
and a recipe for subtle bugs.

> +
> +cli

Interrupts are guaranteed to be off at this point.

> +
> +mov ebx, ADDR_OF(gdtr)
> +lgdt[ebx]

lgdt ADDR_OF(gdtr), presumably?

This is 32bit code - there is no need for any indirection through
registers for memory operands.

> +
> +mov eax, SEC_DEFAULT_CR0
> +mov cr0, eax
> +
> +jmp LINEAR_CODE_SEL:ADDR_OF(.jmpToNewCodeSeg)
> +.jmpToNewCodeSeg:
> +
> +mov eax, SEC_DEFAULT_CR4
> +mov cr4, eax
> +
> +mov ax, LINEAR_SEL
> +mov ds, ax
> +mov es, ax
> +mov fs, ax
> +mov gs, ax
> +mov ss, ax
> +
> +; return to the Main16
> +OneTimeCallRet TransitionFromReal16To32BitFlat

Is there any description of what OneTimeCallRet is, and why a simple jmp
wont do?

Irrespective of that, you're moving to a function whose name suggests it
is in 16bit mode, while you are currently in 32bit flat mode. 
(SEC_DEFAULT_CR0 has PE set, and LINEAR_SEL is 32bit flat.  This clearly
isn't correct, but surely we want to skip all the 16bit setup, as well.

~Andrew

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

[Xen-devel] [PATCH v2 06/31] OvmfPkg/XenResetVector: Add new entry point for Xen PVH

2019-04-08 Thread Anthony PERARD 
This one 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

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Anthony PERARD 
---
 {UefiCpuPkg/ResetVector/Vtf0 => 
OvmfPkg/XenResetVector}/Ia16/ResetVectorVtf0.asm | 18 +++-
 OvmfPkg/XenResetVector/Ia32/XenPVHMain.asm 
  | 47 
 OvmfPkg/XenResetVector/XenResetVector.nasmb
  |  1 +
 3 files changed, 65 insertions(+), 1 deletion(-)

diff --git a/UefiCpuPkg/ResetVector/Vtf0/Ia16/ResetVectorVtf0.asm 
b/OvmfPkg/XenResetVector/Ia16/ResetVectorVtf0.asm
similarity index 76%
copy from UefiCpuPkg/ResetVector/Vtf0/Ia16/ResetVectorVtf0.asm
copy to OvmfPkg/XenResetVector/Ia16/ResetVectorVtf0.asm
index 142d9f3212..46eec66859 100644
--- a/UefiCpuPkg/ResetVector/Vtf0/Ia16/ResetVectorVtf0.asm
+++ b/OvmfPkg/XenResetVector/Ia16/ResetVectorVtf0.asm
@@ -3,6 +3,8 @@
 ; First code executed by processor after resetting.
 ;
 ; Copyright (c) 2008 - 2014, Intel Corporation. All rights reserved.
+; Copyright (c) 2019, Citrix Systems, Inc.
+;
 ; This program and the accompanying materials
 ; are licensed and made available under the terms and conditions of the BSD 
License
 ; which accompanies this distribution.  The full text of the license may be 
found at
@@ -27,9 +29,23 @@ ALIGN   16
 ; located just below 0x1 (4GB) in the firmware device.
 ;
 %ifdef ALIGN_TOP_TO_4K_FOR_PAGING
-TIMES (0x1000 - ($ - EndOfPageTables) - 0x20) DB 0
+TIMES (0x1000 - ($ - EndOfPageTables) - (fourGigabytes - 
xenPVHEntryPoint)) DB 0
 %endif
 
+BITS32
+xenPVHEntryPoint:
+;
+; Entry point to use when running as a Xen PVH guest. (0xffd0)
+;
+; Description of 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
+;
+jmp xenPVHMain
+
+BITS16
+ALIGN   16
+
 applicationProcessorEntryPoint:
 ;
 ; Application Processors entry point
diff --git a/OvmfPkg/XenResetVector/Ia32/XenPVHMain.asm 
b/OvmfPkg/XenResetVector/Ia32/XenPVHMain.asm
new file mode 100644
index 00..c4802bf4d1
--- /dev/null
+++ b/OvmfPkg/XenResetVector/Ia32/XenPVHMain.asm
@@ -0,0 +1,47 @@
+;--
+; @file
+; An entry point use by Xen when a guest is started in PVH mode.
+;
+; Copyright (c) 2019, Citrix Systems, Inc.
+;
+; This program and the accompanying materials are licensed and made available
+; under the terms and conditions of the BSD License which accompanies this
+; distribution.  The full text of the license may be found at
+; http://opensource.org/licenses/bsd-license.php
+;
+; THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, WITHOUT
+; WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+;
+;--
+
+BITS32
+
+xenPVHMain:
+mov di, 'BP'
+
+; ESP -  Initial value of the EAX register (BIST: Built-in Self Test)
+mov esp, eax
+
+cli
+
+mov ebx, ADDR_OF(gdtr)
+lgdt[ebx]
+
+mov eax, SEC_DEFAULT_CR0
+mov cr0, eax
+
+jmp LINEAR_CODE_SEL:ADDR_OF(.jmpToNewCodeSeg)
+.jmpToNewCodeSeg:
+
+mov eax, SEC_DEFAULT_CR4
+mov cr4, eax
+
+mov ax, LINEAR_SEL
+mov ds, ax
+mov es, ax
+mov fs, ax
+mov gs, ax
+mov ss, ax
+
+; return to the Main16
+OneTimeCallRet TransitionFromReal16To32BitFlat
diff --git a/OvmfPkg/XenResetVector/XenResetVector.nasmb 
b/OvmfPkg/XenResetVector/XenResetVector.nasmb
index 49f2bab001..d5a791c139 100644
--- a/OvmfPkg/XenResetVector/XenResetVector.nasmb
+++ b/OvmfPkg/XenResetVector/XenResetVector.nasmb
@@ -70,6 +70,7 @@
 %include "Ia16/Init16.asm"
 
 %include "Main.asm"
+%include "Ia32/XenPVHMain.asm"
 
 %include "Ia16/ResetVectorVtf0.asm"
 
-- 
Anthony PERARD


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