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

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 19/07/2019 15:33, Laszlo Ersek wrote:
> On 07/19/19 12:20, Anthony PERARD wrote:
>> On Fri, Jul 05, 2019 at 02:57:06PM +0100, Andrew Cooper wrote:
>>> On 04/07/2019 15:42, Anthony PERARD wrote:
>>>> diff --git a/OvmfPkg/XenResetVector/Ia16/ResetVectorVtf0.asm b/OvmfPkg/XenResetVector/Ia16/ResetVectorVtf0.asm
>>>> new file mode 100644
>>>> index 0000000000..958195bc5e
>>>> --- /dev/null
>>>> +++ b/OvmfPkg/XenResetVector/Ia16/ResetVectorVtf0.asm
>>>> +vtfSignature:
>>>> +    DB      'V', 'T', 'F', 0
>>>> +
>>>> +ALIGN   16
>>>> +
>>>> +resetVector:
>>>> +;
>>>> +; Reset Vector
>>>> +;
>>>> +; This is where the processor will begin execution
>>>> +;
>>>> +    nop
>>>> +    nop
>>> Why two nops?
>> I don't know, this is existing code that I duplicated to allow adding a
>> new entry point. (I wanted to use --find-copies-harder when sending the
>> patch, but forgot this time.
> Not a big problem; while reviewing v3, I did such comparisons myself, in
> my local clone. Feel free to skip "--find-copies-harder" when posting v4
> too; I think there isn't much churn going on in parallel right now.
>
> However, a new request for v4: please make sure that the new modules /
> paths introduced by this patch set are covered in Maintainers.txt.
>
>> This part of the chunk would not be there.)
> Regarding the NOPs: all I can tell you is that they originate from
> commit 8332983e2e33 ("UefiCpuPkg: Replace the un-necessary WBINVD
> instruction at the reset vector with two NOPs in VTF0.", 2011-08-04).
>
> Whether that change made sense back then, let alone if it makes sense
> now: no clue.

Dropping wbinvd makes sense, because when virtualised, the caches (and
paging for that matter) are always up and running correctly.  Its an
unnecessary vmexit for something which the hypervisor will nop out anyway.

Leaving two nops behind makes no sense at all.

~Andrew