| Message ID | 20240830021117.538954-2-wangyuquan1236@phytium.com.cn |
|---|---|
| State | New |
| Headers | show |
| Series | MdePkg/IndustryStandard: add definitions for ACPI 6.4 CEDT | expand |
For this MdePkg change to add an ACPI table type, do you mind opening a PR? There are some minor code style issues that need to be addressed. Structure type names and define names should be all upper case. __CXL_Early_Discovery_TABLE_H__ -> __CXL_EARLY_DISCOVERY_TABLE_H__ File names should be camel case. CXLEarlyDiscoveryTable.h -> CxlEarlyDiscoveryTable.h Also, please provide links to the supporting public specifications in the include file headers. Thanks, Mike > -----Original Message----- > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Jonathan > Cameron via groups.io > Sent: Friday, August 30, 2024 4:17 AM > To: Yuquan Wang <wangyuquan1236@phytium.com.cn> > Cc: marcin.juszkiewicz@linaro.org; gaoliming@byosoft.com.cn; Kinney, > Michael D <michael.d.kinney@intel.com>; ardb+tianocore@kernel.org; > chenbaozi@phytium.com.cn; wangyinfeng@phytium.com.cn; > shuyiqi@phytium.com.cn; qemu-devel@nongnu.org; devel@edk2.groups.io > Subject: Re: [edk2-devel] [RFC PATCH 1/1] MdePkg/IndustryStandard: add > definitions for ACPI 6.4 CEDT > > On Fri, 30 Aug 2024 10:11:17 +0800 > Yuquan Wang <wangyuquan1236@phytium.com.cn> wrote: > One request - when cross posting to multiple lists, it is useful to > add something to the patch title to make it clear what is EDK2, what > is QEMU etc. > > [RFC EDK2 PATCH 1/1] > > It might irritate the EDK2 folk but it is useful for everyone else > to figure out what they are looking at. > > > This adds #defines and struct typedefs for the various structure > > types in the ACPI 6.4 CXL Early Discovery Table (CEDT). > > It's in the CXL spec, not ACPI spec so call out in this > patch description that all that was added in ACPI 6.4 was > the reservation of the ID. The rest needs to refer to appropriate > CXL specifications. > > For naming I have no idea on the EDK2 Convention for > structures in specifications other than ACPI that are for > ACPI structures. The current one is definitely missleading > however. > > > > > > Signed-off-by: Yuquan Wang <wangyuquan1236@phytium.com.cn> > > --- > > MdePkg/Include/IndustryStandard/Acpi64.h | 5 ++ > > .../IndustryStandard/CXLEarlyDiscoveryTable.h | 69 > +++++++++++++++++++ > > 2 files changed, 74 insertions(+) > > create mode 100644 > MdePkg/Include/IndustryStandard/CXLEarlyDiscoveryTable.h > > > > > diff --git a/MdePkg/Include/IndustryStandard/CXLEarlyDiscoveryTable.h > b/MdePkg/Include/IndustryStandard/CXLEarlyDiscoveryTable.h > > new file mode 100644 > > index 0000000000..84f88dc737 > > --- /dev/null > > +++ b/MdePkg/Include/IndustryStandard/CXLEarlyDiscoveryTable.h > > @@ -0,0 +1,69 @@ > > +/** @file > > + ACPI CXL Early Discovery Table (CEDT) definitions. > > + > > + Copyright (c) 2024, Phytium Technology Co Ltd. All rights reserved. > > + > > +**/ > > + > > +#ifndef __CXL_Early_Discovery_TABLE_H__ > > +#define __CXL_Early_Discovery_TABLE_H__ > > + > > +#include <IndustryStandard/Acpi.h> > > +#include <IndustryStandard/Acpi64.h> > > + > > +#define EFI_ACPI_CXL_Early_Discovery_TABLE_REVISION_01 0x1 > //CXL2.0 > > +#define EFI_ACPI_CXL_Early_Discovery_TABLE_REVISION_02 0x2 > //CXL3.1 > > + > > +#define EFI_ACPI_CEDT_TYPE_CHBS 0x0 > > +#define EFI_ACPI_CEDT_TYPE_CFMWS 0x1 > > Sensible to add all defines from the start? > So CXIMS, RDPAS and CSDS > (only that last one was added in 3.1 / revision 2.0) > > > > +} EFI_ACPI_6_4_CEDT_Structure; > > + > > > +/// > > +/// Definition for CXL Host Bridge Structure > > +/// > > +typedef struct { > > + EFI_ACPI_6_4_CEDT_Structure header; > > + UINT32 UID; > > + UINT32 CXLVersion; > > + UINT32 Reserved; > > + UINT64 Base; > > + UINT64 Length; > > +} EFI_ACPI_6_4_CXL_Host_Bridge_Structure; > Should this naming reflect where it's actually defined? > EFI_ACPI_CXL_3_1_CXL_Host_Bridge_Structure etc > > > + > > +/// > > +/// Definition for CXL Fixed Memory Window Structure > > +/// > > +typedef struct { > > + EFI_ACPI_6_4_CEDT_Structure header; > > + UINT32 Reserved; > > + UINT64 BaseHPA; > > + UINT64 WindowSize; > > + UINT8 InterleaveMembers; > > + UINT8 InterleaveArithmetic; > > + UINT16 Reserved1; > > + UINT32 Granularity; > > + UINT16 Restrictions; > > + UINT16 QtgId; > > + UINT32 FirstTarget; > > Is this common for an EDK2 definition? If it were kernel we'd > be using a [] to indicate this has variable number of elements. > I'm too lazy to check for EDK2 equivalents ;) > > > +} EFI_ACPI_6_4_CXL_Fixed_Memory_Window_Structure; > > + > > +#pragma pack() > > + > > +#endif > > > > -=-=-=-=-=-=-=-=-=-=-=- > Groups.io Links: You receive all messages sent to this group. > View/Reply Online (#120447): > https://edk2.groups.io/g/devel/message/120447 > Mute This Topic: https://groups.io/mt/108173030/1643496 > Group Owner: devel+owner@edk2.groups.io > Unsubscribe: https://edk2.groups.io/g/devel/unsub > [michael.d.kinney@intel.com] > -=-=-=-=-=-=-=-=-=-=-=- >
Also, leading underscores are supposed to be reserved for compiler implementations (and there only needs to be a single trailing underscore) so it should really be: __CXL_Early_Discovery_TABLE_H__ -> CXL_EARLY_DISCOVERY_TABLE_H_
diff --git a/MdePkg/Include/IndustryStandard/Acpi64.h b/MdePkg/Include/IndustryStandard/Acpi64.h index bbe6a3c9eb..c988de8ebf 100644 --- a/MdePkg/Include/IndustryStandard/Acpi64.h +++ b/MdePkg/Include/IndustryStandard/Acpi64.h @@ -3169,6 +3169,11 @@ typedef struct { /// #define EFI_ACPI_6_4_XEN_PROJECT_TABLE_SIGNATURE SIGNATURE_32('X', 'E', 'N', 'V') +/// +/// "CEDT" CXL Early Discovery Table +/// +#define EFI_ACPI_6_4_CXL_EARLY_DISCOVERY_TABLE_SIGNATURE SIGNATURE_32 ('C', 'E', 'D', 'T') + #pragma pack() #endif diff --git a/MdePkg/Include/IndustryStandard/CXLEarlyDiscoveryTable.h b/MdePkg/Include/IndustryStandard/CXLEarlyDiscoveryTable.h new file mode 100644 index 0000000000..84f88dc737 --- /dev/null +++ b/MdePkg/Include/IndustryStandard/CXLEarlyDiscoveryTable.h @@ -0,0 +1,69 @@ +/** @file + ACPI CXL Early Discovery Table (CEDT) definitions. + + Copyright (c) 2024, Phytium Technology Co Ltd. All rights reserved. + +**/ + +#ifndef __CXL_Early_Discovery_TABLE_H__ +#define __CXL_Early_Discovery_TABLE_H__ + +#include <IndustryStandard/Acpi.h> +#include <IndustryStandard/Acpi64.h> + +#define EFI_ACPI_CXL_Early_Discovery_TABLE_REVISION_01 0x1 //CXL2.0 +#define EFI_ACPI_CXL_Early_Discovery_TABLE_REVISION_02 0x2 //CXL3.1 + +#define EFI_ACPI_CEDT_TYPE_CHBS 0x0 +#define EFI_ACPI_CEDT_TYPE_CFMWS 0x1 + +#pragma pack(1) + +/// +/// Table header +/// +typedef struct { + EFI_ACPI_DESCRIPTION_HEADER Header; +} EFI_ACPI_6_4_CXL_Early_Discovery_TABLE; + +/// +/// Node header definition shared by all structure types +/// +typedef struct { + UINT8 Type; + UINT8 Reserved; + UINT16 Length; +} EFI_ACPI_6_4_CEDT_Structure; + +/// +/// Definition for CXL Host Bridge Structure +/// +typedef struct { + EFI_ACPI_6_4_CEDT_Structure header; + UINT32 UID; + UINT32 CXLVersion; + UINT32 Reserved; + UINT64 Base; + UINT64 Length; +} EFI_ACPI_6_4_CXL_Host_Bridge_Structure; + +/// +/// Definition for CXL Fixed Memory Window Structure +/// +typedef struct { + EFI_ACPI_6_4_CEDT_Structure header; + UINT32 Reserved; + UINT64 BaseHPA; + UINT64 WindowSize; + UINT8 InterleaveMembers; + UINT8 InterleaveArithmetic; + UINT16 Reserved1; + UINT32 Granularity; + UINT16 Restrictions; + UINT16 QtgId; + UINT32 FirstTarget; +} EFI_ACPI_6_4_CXL_Fixed_Memory_Window_Structure; + +#pragma pack() + +#endif
This adds #defines and struct typedefs for the various structure types in the ACPI 6.4 CXL Early Discovery Table (CEDT). Signed-off-by: Yuquan Wang <wangyuquan1236@phytium.com.cn> --- MdePkg/Include/IndustryStandard/Acpi64.h | 5 ++ .../IndustryStandard/CXLEarlyDiscoveryTable.h | 69 +++++++++++++++++++ 2 files changed, 74 insertions(+) create mode 100644 MdePkg/Include/IndustryStandard/CXLEarlyDiscoveryTable.h