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

["Anthony PERARD" <anthony.perard@citrix.com>]

On Fri, Jul 19, 2019 at 03:41:52PM +0100, Andrew Cooper wrote:
> 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.

Will do.

> >> 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.

I'll remove the nops.

Thanks,

-- 
Anthony PERARD