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

["Leif Lindholm" <leif@nuviainc.com>]

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

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

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

/
    Leif