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. > On Jul 25, 2019, at 11:55 PM, Kinney, Michael D <michael.d.kin...@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.kin...@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...@intel.com>; devel@edk2.groups.io; >> Kinney, Michael >>> D <michael.d.kin...@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.kin...@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...@intel.com>; >>>> Kinney, Michael >>>>> D <michael.d.kin...@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 >>>>>> >>>>>> >>>>>> > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#44419): https://edk2.groups.io/g/devel/message/44419 Mute This Topic: https://groups.io/mt/32582433/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
