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

[Ni, Ray]

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.kinney at 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 at intel.com>;
>> devel at 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 at intel.com>; devel at edk2.groups.io;
>> Kinney, Michael
>>> D <michael.d.kinney at 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 at intel.com>;
>>>> devel at 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 at edk2.groups.io; Ni, Ray
>> <ray.ni at intel.com>;
>>>> Kinney, Michael
>>>>> D <michael.d.kinney at 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 at edk2.groups.io
>>>>>> [mailto:devel at edk2.groups.io] On Behalf Of Ni,
>> Ray
>>>>>> Sent: Wednesday, July 24, 2019 3:00 AM
>>>>>> To: devel at 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/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-