From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wr1-f67.google.com (mail-wr1-f67.google.com [209.85.221.67]) by mx.groups.io with SMTP id smtpd.web11.59253.1595866469571759641 for ; Mon, 27 Jul 2020 09:14:30 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@nuviainc-com.20150623.gappssmtp.com header.s=20150623 header.b=Id7t1dEx; spf=pass (domain: nuviainc.com, ip: 209.85.221.67, mailfrom: leif@nuviainc.com) Received: by mail-wr1-f67.google.com with SMTP id a5so5495491wrm.6 for ; Mon, 27 Jul 2020 09:14:29 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=nuviainc-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=BjalSeBRPk2zvsrqu+6KAsfPR+bYP4q5wBpUSF7q2V4=; b=Id7t1dExYSiWD3tQqQN7ZwvthW+VDKLqtEFZ3AmbUTrTfc7CQBKaRAEszTzF+zmLyw rCNetrbwsSyrt/w0cT2XjLI4MjvHjcb5R2MRdhSw1GUNMfw+h4Zxw0H8MeERKIIYQHqg 0PEjsDcY4Uz6ulFrd3e6QMdu0FpUFYBvJN+qq+9ZjJO/46pdOa8Fc4Q8qvUMPYiFvMF6 pqgVwTFZ0Cd2CviM6Xgu5RBUiq9viaXswbgoLYU1TbmRkubkSP9EnBMZODgmu3hwSwNt 47+NDfw24D4defACedER8anxI2dXuY1zTGP12xarzwKzpiNuc0Ji18+iWUcK5tkTslZn NIOA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=BjalSeBRPk2zvsrqu+6KAsfPR+bYP4q5wBpUSF7q2V4=; b=jcEotUBPZLcv0Wijl81EexW3zL1hJ1k/8DS3QBy65Weg2JqtEGdUlNUYBNcIkl0srN u0T7rUx/2uNt4jJiv2vC4+xH/fIzZT5B5yFMr3w/5Fv2yVCHovQzaGbw/Iwkco+rYqtt YaWMn1NBr8LkhpNm3NeddePs9P4sLqYJdReT2vuVLbLUYPjSFfJbxpx/VNQESclaoj2L iwn7K6u2T5UipPyv4hiFk76YK1AMWPHsmDklmUYRlqTE1V0t+Jh/B40wp4X0Db4/CAvf 1ZJifUbVc9G0Wurkis8qxqcrxIJyA5Y31FICNOjHtSmjC1xR7EXdFPz+oPzjYcoSp+PQ Lqrg== X-Gm-Message-State: AOAM530XmtMEZ2kDwh6riCGwoeA2FYpSBQ7lCaPzazBpTJMTvKTRQWAQ 3DZjtEUyXGM/w/PCYWT2DOXTBg== X-Google-Smtp-Source: ABdhPJyn/mmwE17UHyx0PW0FdSJU/7y3/7Hhh3WMUW7MZSk+Ka1bjUZibZfCpA4RKcgW/mJa9WiWbQ== X-Received: by 2002:adf:f4ca:: with SMTP id h10mr20509590wrp.355.1595866467994; Mon, 27 Jul 2020 09:14:27 -0700 (PDT) Return-Path: Received: from vanye ([2001:470:1f09:12f0:b26e:bfff:fea9:f1b8]) by smtp.gmail.com with ESMTPSA id c136sm114078wmd.10.2020.07.27.09.14.26 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 27 Jul 2020 09:14:27 -0700 (PDT) Date: Mon, 27 Jul 2020 17:14:25 +0100 From: "Leif Lindholm" To: "Gao, Liming" Cc: "devel@edk2.groups.io" , "Javeed, Ashraf" , "Kinney, Michael D" Subject: Re: [edk2-devel] [PATCH V4 1/2] MdePkg/Include/IndustryStandard: CXL 1.1 Registers Message-ID: <20200727161425.GB1337@vanye> References: <20200724182613.9344-1-ashraf.javeed@intel.com> <20200724182613.9344-2-ashraf.javeed@intel.com> <20200727142442.GA1337@vanye> MIME-Version: 1.0 In-Reply-To: User-Agent: Mutt/1.10.1 (2018-07-13) Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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. 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