Leif, Liming, My comments inline below. Thanks Ashraf > -----Original Message----- > From: Ni, Ray <ray...@intel.com> > Sent: Tuesday, July 28, 2020 8:06 AM > To: devel@edk2.groups.io; Gao, Liming <liming....@intel.com>; Leif Lindholm > <l...@nuviainc.com>; Javeed, Ashraf <ashraf.jav...@intel.com>; Laszlo Ersek > <ler...@redhat.com>; Sean Brogan <sean.bro...@microsoft.com> > Cc: Kinney, Michael D <michael.d.kin...@intel.com>; Gough, Robert > <robert.go...@intel.com> > Subject: RE: [edk2-devel] [PATCH V4 1/2] MdePkg/Include/IndustryStandard: > CXL 1.1 Registers > > I am not sure if Robert (Cced) has the interest to be the reviewer of the > MdePkg/Include/IndustryStandard directory. > > > -----Original Message----- > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Liming > > Gao > > Sent: Tuesday, July 28, 2020 10:07 AM > > To: Leif Lindholm <l...@nuviainc.com>; Javeed, Ashraf > > <ashraf.jav...@intel.com>; Laszlo Ersek <ler...@redhat.com>; Sean > > Brogan <sean.bro...@microsoft.com> > > Cc: devel@edk2.groups.io; Kinney, Michael D > > <michael.d.kin...@intel.com> > > Subject: Re: [edk2-devel] [PATCH V4 1/2] > > MdePkg/Include/IndustryStandard: CXL 1.1 Registers > > > > Leif: > > > > -----Original Message----- > > From: Leif Lindholm <l...@nuviainc.com> > > Sent: 2020年7月28日 0:14 > > To: Gao, Liming <liming....@intel.com> > > Cc: devel@edk2.groups.io; Javeed, Ashraf <ashraf.jav...@intel.com>; > > Kinney, Michael D <michael.d.kin...@intel.com> > > Subject: Re: [edk2-devel] [PATCH V4 1/2] > > MdePkg/Include/IndustryStandard: CXL 1.1 Registers > > > > On Mon, Jul 27, 2020 at 14:55:03 +0000, Gao, Liming wrote: > > > > > diff --git a/MdePkg/Include/IndustryStandard/Cxl11.h > > > > > b/MdePkg/Include/IndustryStandard/Cxl11.h > > > > > new file mode 100644 > > > > > index 0000000000..933c1ab817 > > > > > --- /dev/null > > > > > +++ b/MdePkg/Include/IndustryStandard/Cxl11.h > > > > > @@ -0,0 +1,569 @@ > > > > > +/** @file > > > > > + CXL 1.1 Register definitions > > > > > + > > > > > + This file contains the register definitions based on the > > > > > + Compute Express Link > > > > > + (CXL) Specification Revision 1.1. > > > > > + > > > > > +Copyright (c) 2020, Intel Corporation. All rights reserved.<BR> > > > > > +SPDX-License-Identifier: BSD-2-Clause-Patent > > > > > + > > > > > +**/ > > > > > + > > > > > +#ifndef _CXL11_H_ > > > > > +#define _CXL11_H_ > > > > > > > > We should not be adding macros with a leading _ - these are > > > > intended for toolchain use. > > > > > > This style is align to other header file. This is the file header > > > macro to make sure the header file be included more than once. > > > > Yes. The other headers should also be changed, but in the meantime it would > be good to stop adding more incorrect ones. > > https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/5 > > _source_files/53_include_files#5-3-5-all-include-file- > > contents-must-be-protected-by-a-include-guard > > > > [Liming] Thank for your point. I miss this one, too. Now, most cases > > don't follow this rule. So, there is no good example for the reference. > > I agree the rule to apply the strict check for new adding file. I will check > whether ECC has this check point. > > Can make the change to remove the _ in the beginning of the macro guards when I am adding for CXL 2.0 specification
> > > > > + > > > > > +// > > > > > +// CXL Flex Bus Device default device and function number // > > > > > +Compute Express Link Specification Revision: 1.1 - Chapter > > > > > +7.1.1 // > > > > > +#define CXL_DEV_DEV > > > > > 0 > > > > > +#define CXL_DEV_FUNC > > > > > 0 > > > > > + > > > > > +// > > > > > +// Ensure proper structure formats // #pragma pack(1) > > > > > > > > And this pragma has no function whatsoever with regards to any of > > > > the register definition structs below. It would be much better if > > > > the structs requiring packing (_DEVICE, _PORT, ...) were grouped > > > > together and only those were given this treatment. > > > > > > > > #pragma pack(1) is *not* a safe default. > > > > > > > > > > I know pack(1) is for the compact structure layout. > > > > Yes. And it should be used when structs need to be compacted. > > All of the bitfield structs are single-variable structs, so the > > packing has no effect on them, other than setting the struct alignment > requirements to 1 byte, which will not be correct (or efficient) on all > architectures. > > > > [Liming] Yes. There is no effect for bitfield structure. This header > > file still includes some structure, such as CXL_1_1_DVSEC_FLEX_BUS_DEVICE. > They may have the compact alignment requirement. > > @Javeed, Ashraf, can you conform it? > > Yes, all the grouped CXL registers that are listed below are packed to fixed offsets, hence I thought it is better to have pack(1) in the beginning and pack() in the end to avoid any effect of global compiler settings... -CXL_1_1_DVSEC_FLEX_BUS_DEVICE, -CXL_1_1_DVSEC_FLEX_BUS_PORT, -CXL_1_1_RAS_CAPABILITY_STRUCTURE, -CXL_1_1_LINK_CAPABILITY_STRUCTURE Is the preference is to add the packing instruction to each group independently? > > > > Now, one final comment - and this is more of a project feature > > > > suggestion: > > > > Industry standard headers is something fairly special, even in > > > > comparison with the rest of MdePkg. *I* would certainly like to > > > > ensure I don't miss changes or additions to them. > > > > Could we set up a dedicated group of reviewers for this folder only? > > > > This need not affect the actual maintainership of MdePkg, just > > > > ensure more eyeballs (or screen readers, braille terminals, ...) > > > > hit updates here? > > > > > > > > i.e. something like the below to Maintainers.txt: > > > > > > > > F: MdePkg/Include/IndustryStandard/ > > > > R: Leif ... > > > > R: ... > > > > R: ... > > > > > > > > > > This is a good suggestion. IndustryStandard needs more feedback. > > > Can you send the patch to update Maintainers.txt? > > > > Sure, I can do that. Any thoughts on others to add than me? > > > > [Liming] Thanks. Laszlo or Sean may be added if they are also interested in > IndustryStandard header file. > > > > Thanks > > Liming > > / > > Leif > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#63386): https://edk2.groups.io/g/devel/message/63386 Mute This Topic: https://groups.io/mt/75823529/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
