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

["Ni, Ray" <ray.ni@intel.com>]

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 <leif@nuviainc.com>; Javeed, Ashraf <ashraf.javeed@intel.com>; Laszlo Ersek <lersek@redhat.com>;
> Sean Brogan <sean.brogan@microsoft.com>
> Cc: devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>
> Subject: Re: [edk2-devel] [PATCH V4 1/2] MdePkg/Include/IndustryStandard: CXL 1.1 Registers
> 
> 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
> 
>