Re: [edk2-devel] [PATCH V2 0/6] Support 5-level paging in DXE long mode

["Michael D Kinney" <michael.d.kinney@intel.com>]

Ray,

I prefer to add a Vendor to the path based
on the vendor who published the specification.
Adding the vendor to the path will allow 
include files to be added in the future with
clear names.

The UefiCpuPkg is an example of a poor choice
for the original Cpuid.h file.  It should have
been in a vendor specific directory from the
beginning.  The AMD one is correct in that
package.

I would also like to consider moving more of
The ARM and AARCH64 content into MdePkg to help
with package dependencies.

If we really want only a single directory,
then another option is to add the vendor
name to the include filename.

Mike

> -----Original Message-----
> From: Ni, Ray
> Sent: Wednesday, July 24, 2019 10:40 PM
> To: Kinney, Michael D <michael.d.kinney@intel.com>;
> devel@edk2.groups.io
> Subject: RE: [edk2-devel] [PATCH V2 0/6] Support 5-
> level paging in DXE long mode
> 
> Mike,
> All the CPUID definitions also applies to AMD
> processors.
> There are two Cpuid.h in UefiCpuPkg today.
> 1. UefiCpuPkg/Include/Register/Cpuid.h
> 2. UefiCpuPkg/Include/Register/Amd/Cpuid.h
> 
> The second one contains additional structures needed by
> AMD processor.
> But first one also applies to AMD processor.
> 
> Can we just put Cpuid.h under MdePkg/Include/Register/
> because they are common to both Intel and AMD?
> 
> Thanks,
> Ray
> 
> > -----Original Message-----
> > From: Kinney, Michael D
> > Sent: Thursday, July 25, 2019 8:52 AM
> > To: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io;
> Kinney, Michael
> > D <michael.d.kinney@intel.com>
> > Subject: RE: [edk2-devel] [PATCH V2 0/6] Support 5-
> level paging in DXE
> > long mode
> >
> > Ray,
> >
> > I think the use of Include/Register is good for this
> type of content.
> > But we may need a Vendor directory.
> >
> > The general form would be:
> >
> >   Include/Register/<Vendor>/<RegisterFile>.h
> >
> > Since the definitions being discussed here are from
> an Intel public
> > document, perhaps the path should be:
> >
> >   Include/Register/Intel/Cpuid.h
> >
> > Thanks,
> >
> > Mike
> >
> > > -----Original Message-----
> > > From: Ni, Ray
> > > Sent: Wednesday, July 24, 2019 5:46 PM
> > > To: Kinney, Michael D <michael.d.kinney@intel.com>;
> > > devel@edk2.groups.io
> > > Subject: RE: [edk2-devel] [PATCH V2 0/6] Support 5-
> level paging in
> > > DXE long mode
> > >
> > > Mike,
> > > Are you suggesting that
> > > 1. Copy Cpuid.h in MdePkg/Include/IndustryStandard/
> 2.
> > > Change UefiCpuPkg/Include/Register/Cpuid.h to just
> include
> > > <IndustryStandard/Cpuid.h>
> > >
> > > It looks like a potential issue that there are two
> "Cpuid.h" public
> > > header file in different folders.
> > > But given the include pattern used:
> > > "<Register/Cpuid.h>" VS
> "<IndustryStandard/Cpuid.h>", the risk
> > > people may include wrong file or compilers don't
> know which file to
> > > use is zero.
> > >
> > > Is that what you think?
> > >
> > > Thanks,
> > > Ray
> > >
> > > > -----Original Message-----
> > > > From: Kinney, Michael D
> > > > Sent: Thursday, July 25, 2019 1:23 AM
> > > > To: devel@edk2.groups.io; Ni, Ray
> <ray.ni@intel.com>;
> > > Kinney, Michael
> > > > D <michael.d.kinney@intel.com>
> > > > Subject: RE: [edk2-devel] [PATCH V2 0/6] Support
> 5-
> > > level paging in DXE
> > > > long mode
> > > >
> > > > Hi Ray,
> > > >
> > > > Given that there may be register definitions for
> > > other CPUs or devices
> > > > added to MdePkg in the future, should an extra
> > > directory level be
> > > > added?  Doing that would break source
> compatibility
> > > for existing
> > > > components that use #include <Register/Cpuid.h>
> from
> > > UefiCpuPkg.  We
> > > > could keep Cpuid.h in UefiCpuPkg, and it could be
> a
> > > #include of the
> > > > new Cpuid.h file in the MdePkg in the extended
> path.
> > > >
> > > > Also, should CpuId.h be included from BaseLib.h
> > > inside:
> > > >
> > > >   #if defined (MDE_CPU_IA32) || defined
> (MDE_CPU_X64)
> > > >
> > > > This would make all CPUID related register
> > > definitions available to
> > > > components the needs BaseLib to call
> > > > AsmCpuId() or AsmCpuIdEx()?
> > > >
> > > > We could also move the CRx,
> FLAGS/EFLAGS/descriptor
> > > structures out of
> > > > BaseLib.h into include files that are peers to
> > > Cpuid.h and could be
> > > > updated based on public spec updates.
> > > >
> > > > Thanks,
> > > >
> > > > Mike
> > > >
> > > > > -----Original Message-----
> > > > > From: devel@edk2.groups.io
> > > > > [mailto:devel@edk2.groups.io] On Behalf Of Ni,
> Ray
> > > > > Sent: Wednesday, July 24, 2019 3:00 AM
> > > > > To: devel@edk2.groups.io
> > > > > Subject: [edk2-devel] [PATCH V2 0/6] Support 5-
> > > level paging in DXE
> > > > > long mode
> > > > >
> > > > > v2:
> > > > >    Refined the patch according to reviewers'
> all
> > > comments except:
> > > > >       0A0h cannot be changed to A0h or build
> fails.
> > > > >    A big change in this patch is Cpuid.h is
> moved
> > > from UefiCpuPkg to
> > > > > MdePkg.
> > > > >    The move is based on real requirement when
> > > certain modules that
> > > > > cannot
> > > > >    depend on UefiCpuPkg but needs to reference
> > > structures defined in
> > > > > SDM.
> > > > >
> > > > > Ray Ni (6):
> > > > >   UefiCpuPkg/MpInitLib: Enable 5-level paging
> for
> > > AP when BSP's
> > > > > enabled
> > > > >   UefiCpuPkg/CpuDxe: Remove unnecessary macros
> > > > >   UefiCpuPkg/CpuDxe: Support parsing 5-level
> page
> > > table
> > > > >   MdeModulePkg/DxeIpl: Introduce PCD
> > > PcdUse5LevelPageTable
> > > > >   MdePkg/Cpuid.h: Move Cpuid.h from UefiCpuPkg
> to
> > > MdePkg
> > > > >   MdeModulePkg/DxeIpl: Create 5-level page
> table
> > > for long mode
> > > > >
> > > > >  MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> |
> > > 1 +
> > > > >  .../Core/DxeIplPeim/X64/VirtualMemory.c
> |
> > > 229
> > > > > ++++++++++++------
> > > > >  MdeModulePkg/MdeModulePkg.dec
> |
> > > 7 +
> > > > >  MdeModulePkg/MdeModulePkg.uni
> |
> > > 8 +
> > > > >  .../Include/Register/Cpuid.h
> |
> > > 0
> > > > >  UefiCpuPkg/CpuDxe/CpuPageTable.c
> |
> > > 59
> > > > > +++--
> > > > >  UefiCpuPkg/CpuDxe/CpuPageTable.h
> |
> > > 3 +-
> > > > >  UefiCpuPkg/Library/MpInitLib/MpLib.c
> |
> > > 13 +
> > > > >  UefiCpuPkg/Library/MpInitLib/MpLib.h
> |
> > > 6 +-
> > > > >  UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc
> |
> > > 3 +-
> > > > >  UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
> |
> > > 14 +-
> > > > >  11 files changed, 243 insertions(+), 100
> > > deletions(-) rename
> > > > > {UefiCpuPkg => MdePkg}/Include/Register/Cpuid.h
> > > > > (100%)
> > > > >
> > > > > --
> > > > > 2.21.0.windows.1
> > > > >
> > > > >
> > > > >