Re: [edk2-devel] [PATCH V4 1/2] MdePkg/Include/IndustryStandard: CXL 1.1 Registers

["Liming Gao" <liming.gao@intel.com>]

Leif:

-----Original Message-----
From: Leif Lindholm <leif@nuviainc.com> 
Sent: 2020年7月28日 0:14
To: Gao, Liming <liming.gao@intel.com>
Cc: devel@edk2.groups.io; Javeed, Ashraf <ashraf.javeed@intel.com>; Kinney, Michael D <michael.d.kinney@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. 

> > > +
> > > +//
> > > +// 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?

> > 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