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

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

Ray,

The .h files represent information from specifications and
components that include the include files do so because
their source code depends on definitions associated with 
those specifications.  It does not represent what CPUs
are present in the platform.  It may represent what
CPUs the module/lib supports.

Mike

> -----Original Message-----
> From: Ni, Ray
> Sent: Thursday, July 25, 2019 4:42 PM
> To: Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: devel@edk2.groups.io
> Subject: Re: [edk2-devel] [PATCH V2 0/6] Support 5-
> level paging in DXE long mode
> 
> Having the vendor name for cpu may cause confusion to
> customers.
> AMD customer may found even their code is running in
> AMD processors Intel/Cpuid.h is still included.
> It may also be possible that Intel platform code has to
> include AMD/Cpuid.h.
> 
> The CPU is different from other hardwares. It is ok
> that two PCIE cards from different vendors exist in the
> same platform. But not Ok for CPU.
> Not sure if I am over concerned.
> 
> Sent from my iPhone
> 
> > On Jul 25, 2019, at 11:55 PM, Kinney, Michael D
> <michael.d.kinney@intel.com> wrote:
> >
> > 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
> >>>>>>
> >>>>>>
> >>>>>> 
> >