From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by mx.groups.io with SMTP id smtpd.web10.4148.1595903772379000953 for ; Mon, 27 Jul 2020 19:36:12 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 192.55.52.115, mailfrom: ray.ni@intel.com) IronPort-SDR: XcflWRXZWgfUzaI36kfSVswIPUcQHeei1zpX9IqffGIAHtYDFAtH9sRcW3bt3bBJuEhsX7oOo0 93sr2a2E8BlQ== X-IronPort-AV: E=McAfee;i="6000,8403,9695"; a="150311728" X-IronPort-AV: E=Sophos;i="5.75,404,1589266800"; d="scan'208";a="150311728" X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga004.jf.intel.com ([10.7.209.38]) by fmsmga103.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 27 Jul 2020 19:36:11 -0700 IronPort-SDR: 8QdcPfVKS1haTOo6oabXUFy/p67QkcF265P8rWn2zURkpF94B7w3RdIpALg3c6cSh6C8Ard36t IYulJ4BWKPvw== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.75,404,1589266800"; d="scan'208";a="434157605" Received: from fmsmsx105.amr.corp.intel.com ([10.18.124.203]) by orsmga004.jf.intel.com with ESMTP; 27 Jul 2020 19:36:10 -0700 Received: from fmsmsx104.amr.corp.intel.com (10.18.124.202) by FMSMSX105.amr.corp.intel.com (10.18.124.203) with Microsoft SMTP Server (TLS) id 14.3.439.0; Mon, 27 Jul 2020 19:36:10 -0700 Received: from shsmsx152.ccr.corp.intel.com (10.239.6.52) by fmsmsx104.amr.corp.intel.com (10.18.124.202) with Microsoft SMTP Server (TLS) id 14.3.408.0; Mon, 27 Jul 2020 19:36:09 -0700 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.135]) by SHSMSX152.ccr.corp.intel.com ([169.254.6.250]) with mapi id 14.03.0439.000; Tue, 28 Jul 2020 10:36:05 +0800 From: "Ni, Ray" To: "devel@edk2.groups.io" , "Gao, Liming" , Leif Lindholm , "Javeed, Ashraf" , Laszlo Ersek , Sean Brogan CC: "Kinney, Michael D" , "Gough, Robert" Subject: Re: [edk2-devel] [PATCH V4 1/2] MdePkg/Include/IndustryStandard: CXL 1.1 Registers Thread-Topic: [edk2-devel] [PATCH V4 1/2] MdePkg/Include/IndustryStandard: CXL 1.1 Registers Thread-Index: AQHWYef0u12W1ncd0E6gf4/p4MKELaka+WkAgAAIe4CAABYtgIAApZYAgACNwEA= Date: Tue, 28 Jul 2020 02:36:04 +0000 Message-ID: <734D49CCEBEEF84792F5B80ED585239D5C62D33D@SHSMSX104.ccr.corp.intel.com> References: <20200724182613.9344-1-ashraf.javeed@intel.com> <20200724182613.9344-2-ashraf.javeed@intel.com> <20200727142442.GA1337@vanye> <20200727161425.GB1337@vanye> In-Reply-To: Accept-Language: en-US, zh-CN X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-version: 11.2.0.6 dlp-reaction: no-action x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Return-Path: ray.ni@intel.com Content-Language: en-US Content-Type: text/plain; charset="iso-2022-jp" Content-Transfer-Encoding: quoted-printable I am not sure if Robert (Cced) has the interest to be the reviewer of the M= dePkg/Include/IndustryStandard directory. > -----Original Message----- > From: devel@edk2.groups.io On Behalf Of Liming Ga= o > Sent: Tuesday, July 28, 2020 10:07 AM > To: Leif Lindholm ; Javeed, Ashraf ; Laszlo Ersek ; > Sean Brogan > Cc: devel@edk2.groups.io; Kinney, Michael D > Subject: Re: [edk2-devel] [PATCH V4 1/2] MdePkg/Include/IndustryStandard= : CXL 1.1 Registers >=20 > Leif: >=20 > -----Original Message----- > From: Leif Lindholm > Sent: 2020=1B$BG/=1B(B7=1B$B7n=1B(B28=1B$BF|=1B(B 0:14 > To: Gao, Liming > Cc: devel@edk2.groups.io; Javeed, Ashraf ; Kinn= ey, Michael D > Subject: Re: [edk2-devel] [PATCH V4 1/2] MdePkg/Include/IndustryStandard= : CXL 1.1 Registers >=20 > 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.
> > > > +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. >=20 > Yes. The other headers should also be changed, but in the meantime it wo= uld be good to stop adding more incorrect ones. > https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/5_s= ource_files/53_include_files#5-3-5-all-include-file- > contents-must-be-protected-by-a-include-guard >=20 > [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 c= heck whether ECC has this check point. >=20 > > > > + > > > > +// > > > > +// 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. >=20 > 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 efficien= t) on all architectures. >=20 > [Liming] Yes. There is no effect for bitfield structure. This header fil= e still includes some structure, such as > CXL_1_1_DVSEC_FLEX_BUS_DEVICE. They may have the compact alignment requi= rement. > @Javeed, Ashraf, can you conform it? >=20 > > > 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? >=20 > Sure, I can do that. Any thoughts on others to add than me? >=20 > [Liming] Thanks. Laszlo or Sean may be added if they are also interested= in IndustryStandard header file. >=20 > Thanks > Liming > / > Leif >=20 >=20