public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Michael D Kinney" <michael.d.kinney@intel.com>
To: "Ni, Ray" <ray.ni@intel.com>,
	"Kinney, Michael D" <michael.d.kinney@intel.com>
Cc: "devel@edk2.groups.io" <devel@edk2.groups.io>
Subject: Re: [edk2-devel] [PATCH V2 0/6] Support 5-level paging in DXE long mode
Date: Fri, 26 Jul 2019 18:17:16 +0000	[thread overview]
Message-ID: <E92EE9817A31E24EB0585FDF735412F5B9D7DF69@ORSMSX113.amr.corp.intel.com> (raw)
In-Reply-To: <FAC6F0F7-A61F-466C-876A-82E1885DE431@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
> >>>>>>
> >>>>>>
> >>>>>> 
> >

  reply	other threads:[~2019-07-26 18:17 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-07-24 10:00 [PATCH V2 0/6] Support 5-level paging in DXE long mode Ni, Ray
2019-07-24 10:00 ` [PATCH V2 1/6] UefiCpuPkg/MpInitLib: Enable 5-level paging for AP when BSP's enabled Ni, Ray
2019-07-24 10:00 ` [PATCH V2 2/6] UefiCpuPkg/CpuDxe: Remove unnecessary macros Ni, Ray
2019-07-24 10:00 ` [PATCH V2 3/6] UefiCpuPkg/CpuDxe: Support parsing 5-level page table Ni, Ray
2019-07-24 10:00 ` [PATCH V2 4/6] MdeModulePkg/DxeIpl: Introduce PCD PcdUse5LevelPageTable Ni, Ray
2019-07-24 10:00 ` [PATCH V2 5/6] MdePkg/Cpuid.h: Move Cpuid.h from UefiCpuPkg to MdePkg Ni, Ray
2019-07-30  3:29   ` Liming Gao
2019-07-30  3:42     ` Michael D Kinney
2019-07-30  3:49       ` Liming Gao
2019-07-30 16:30         ` Michael D Kinney
2019-08-01  2:26           ` Ni, Ray
2019-08-01  8:15             ` Michael D Kinney
2019-07-24 10:00 ` [PATCH V2 6/6] MdeModulePkg/DxeIpl: Create 5-level page table for long mode Ni, Ray
2019-07-24 17:23 ` [edk2-devel] [PATCH V2 0/6] Support 5-level paging in DXE " Michael D Kinney
2019-07-25  0:45   ` Ni, Ray
2019-07-25  0:52     ` Michael D Kinney
2019-07-25  5:39       ` Ni, Ray
2019-07-25 15:55         ` Michael D Kinney
2019-07-25 23:42           ` Ni, Ray
2019-07-26 18:17             ` Michael D Kinney [this message]
2019-07-26 10:14 ` Laszlo Ersek

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=E92EE9817A31E24EB0585FDF735412F5B9D7DF69@ORSMSX113.amr.corp.intel.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox