Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7;
Date:   Tue, 12 Dec 2023 12:50:41 -0800
From:   Ira Weiny <ira.weiny@intel.com>
To:     Dan Williams <dan.j.williams@intel.com>,
        Ira Weiny <ira.weiny@intel.com>,
        Jonathan Cameron <jonathan.cameron@huawei.com>,
        Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com>,
        Shiju Jose <shiju.jose@huawei.com>
CC:     Yazen Ghannam <yazen.ghannam@amd.com>,
        Davidlohr Bueso <dave@stgolabs.net>,
        Dave Jiang <dave.jiang@intel.com>,
        Alison Schofield <alison.schofield@intel.com>,
        Vishal Verma <vishal.l.verma@intel.com>,
        "Ard Biesheuvel" <ardb@kernel.org>, <linux-efi@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, <linux-cxl@vger.kernel.org>,
        Ira Weiny <ira.weiny@intel.com>
Subject: Re: [PATCH v2 6/7] firmware/efi: Process CXL Component Events
Message-ID: <6578c7a1707c1_2253952944@iweiny-mobl.notmuch>
References: <20231211-cxl-cper-v2-0-c116900ba658@intel.com>
 <20231211-cxl-cper-v2-6-c116900ba658@intel.com>
 <657891bb7ec69_269bd29421@dwillia2-mobl3.amr.corp.intel.com.notmuch>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <657891bb7ec69_269bd29421@dwillia2-mobl3.amr.corp.intel.com.notmuch>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?gfVt99aPFpBlqrANFgwQaS9sVZEpQptxQrlHKBSumVY3hG6gA9XnD9HJQ9uU?=
 =?us-ascii?Q?kyAai6/aM4jQvbhOzqQFbOo/9p+raCkjLwA1u2HDbIauKNJSkvjwWNESw7EK?=
 =?us-ascii?Q?HyMOPGKZtxOQ1NN/P9hlLiVfNnXxInTy79Gk/nE0Qh7JxE0xKhxipaHW4oeo?=
 =?us-ascii?Q?Ojhx+D9P7NYBrXg7QCVu70ia/Lx+CIoyqNsYBZoEApzF/x5hjpWf62jrsNKi?=
 =?us-ascii?Q?siUbTfcJl9U86Bf3jA8O4P0E54WJzvutcDtTjEc9H7RnkbDpKi4Os197rhss?=
 =?us-ascii?Q?H6HvnGW+MTpHHxgZbNCeIJGVkIF+V2WJxaJLQfd1SgGETYkRWxoLsLwMrkHS?=
 =?us-ascii?Q?7eSrW1mBYq2ptfYAcnaHLS573kgvzeS6wj0wSqeslT46LdpHwR/qn/Tl2HB+?=
 =?us-ascii?Q?xUJDx6Rv04VdXWnAAsLTMSNcbnMkRAvkmwz0r1GpDxEiCrw2nTH7uYkaUdWf?=
 =?us-ascii?Q?3BapGQDuhOw9uD0q25GO6oRiWukpdtMmW8axlhOuicMq84QGZDs6Bhb3S2fn?=
 =?us-ascii?Q?t2YHeJJwC1r97rrvhFp8/Srjhin+xBHMD2NLjL3cz8WQ44CHGhn7QF/Yy7K9?=
 =?us-ascii?Q?ZpoOp5/uBDvjS8e6jsrPL17BOROtR75swDZRwITBZ26Hya+O44gRFsYt7Yom?=
 =?us-ascii?Q?YxiACbCXpCCb3rAFqiKPPZ6sK4kGGzx76R0nBQwZhguratLrU4JtR0c4EIkU?=
 =?us-ascii?Q?ekaEb3EFbmRvr5D6E5AvpiesGAtk1VxViB49ANdBg8Tan+jlt+OWh64Zvobd?=
 =?us-ascii?Q?602Ihp2zS33ay9jDe8F76F3J+qdC8YD8ifTmrUZao3+z/CohMDKmmiuIZzeK?=
 =?us-ascii?Q?1kWcl1GGPTd5TesU+Jei77xdb9J1fWU1CQRT477TXt/ESJOXizyK2tTr2P4E?=
 =?us-ascii?Q?MfM0BULNRtIlWOvgJrzyHBM78qxeyjpW87b98xf9Tiv28KN+qDtd1sIreuFr?=
 =?us-ascii?Q?wOZsbW5j089nxsZoDS+pIlILLSQlrA+HI3EJekyMMFmgMMfsk6G37s6V7rAf?=
 =?us-ascii?Q?me2ftT1TeoSAbtJp4dBrq+Qt/lEA5JsXFepwUR0xEzxAtZrZzyMzrulaU8MZ?=
 =?us-ascii?Q?8098uD+IeTG194qRsVg5zpob+KwGQQETW/L7u4xRjibdIUvw0kdO2IZ3zq6J?=
 =?us-ascii?Q?t01UBQMxZ6ncBZhnzvLHctztvUG3ZvPGMmZY065NdnYYHscmzxT/I+6/0j9G?=
 =?us-ascii?Q?j3jBOjoBGwCnKGCA001ZArT94Hzfd/g1b/bm69a4U0SYxUFnVQAE1bwhrNP4?=
 =?us-ascii?Q?dhbK5E3Wl2csElkLotvnwsl8KzZNVFAQBcj4wgJoX3CTj4lNKFESmQBJdIc5?=
 =?us-ascii?Q?l/t11v4P7+QK7Rr1wQFVIYlXOGpLqqNjFXSkKrJWuSNzNW2NU7qxjkqIQ+jb?=
 =?us-ascii?Q?4b3RRWklxbLSg48/dtehZFSNpziSauA2sHTTjdgHjEEXesbCOC03c2KGqgtc?=
 =?us-ascii?Q?Fs7Ow63Dq1R4hMAkB9Nvg8mG4/Oc5EtpO7NiC3uzOR7incwXQjePT/nx/ELJ?=
 =?us-ascii?Q?/1Ori304MDpgSOP+JOuS55WMOe1Wi/ZnTL6mnSEj8n5GwyAWzQC6NrkBjgKH?=
 =?us-ascii?Q?c8ic5WhnRSshOzXmLV6EbkdoSMhZipGx2y++sF37?=
X-MS-Exchange-CrossTenant-Network-Message-Id: fef0c282-9dd7-45cd-626a-08dbfb540392
X-MS-Exchange-CrossTenant-AuthSource: SA1PR11MB6733.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 12 Dec 2023 20:50:46.1089
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: WwyOu3HfqjOaUvaTf/nd1O4ilZwQP6Ryq+mPdcVHS4618bAMVl6z2KnbzMJU0vTPM3IxJkXihamPWlxt9HHWAA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR11MB5205
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,
        RCVD_IN_DNSWL_BLOCKED,SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (snail.vger.email [0.0.0.0]); Tue, 12 Dec 2023 12:52:54 -0800 (PST)

Dan Williams wrote:
> Ira Weiny wrote:
> > BIOS can configure memory devices as firmware first.  This will send CXL
> > events to the firmware instead of the OS.  The firmware can then send
> > these events to the OS via UEFI.
> > 
> > UEFI v2.10 section N.2.14 defines a Common Platform Error Record (CPER)
> > format for CXL Component Events.  The format is mostly the same as the
> > CXL Common Event Record Format.  The difference is a GUID is used in
> > the Section Type to identify the event type.
> > 
> > Add EFI support to detect CXL CPER records and call a notifier chain
> > with the record data blobs to be processed by the CXL code.
> 
> It is no longer a notifier chain in this version. I wouldn't even call
> it a notifier, it's just a registered callback.

Ah yea I missed that in my rework sorry.

[snip]

> > +DECLARE_RWSEM(cxl_cper_rw_sem);
> > +static cxl_cper_notifier cper_notifier;
> > +
> > +void cxl_cper_post_event(const char *pfx, guid_t *sec_type,
> > +			 struct cper_cxl_event_rec *rec)
> > +{
> > +	struct cxl_cper_event_data data = {
> > +		.rec = rec,
> > +	};
> > +
> > +	if (!(rec->hdr.validation_bits & CPER_CXL_COMP_EVENT_LOG_VALID)) {
> > +		pr_err(FW_WARN "cxl event no Component Event Log present\n");
> > +		return;
> > +	}
> > +
> > +	if (guid_equal(sec_type, &CPER_SEC_CXL_GEN_MEDIA_GUID))
> > +		data.event_type = CXL_CPER_EVENT_GEN_MEDIA;
> > +	else if (guid_equal(sec_type, &CPER_SEC_CXL_DRAM_GUID))
> > +		data.event_type = CXL_CPER_EVENT_DRAM;
> > +	else if (guid_equal(sec_type, &CPER_SEC_CXL_MEM_MODULE_GUID))
> > +		data.event_type = CXL_CPER_EVENT_MEM_MODULE;
> > +
> > +	down_read(&cxl_cper_rw_sem);
> 
>    guard(rwsem_read)(&cxl_cper_rw_sem);

Much better.  Done throughout.

> 
> > +	if (cper_notifier)
> > +		cper_notifier(&data);
> > +	up_read(&cxl_cper_rw_sem);
> > +}
> > +
> > +void cxl_cper_register_notifier(cxl_cper_notifier notifier)
> > +{
> > +	down_write(&cxl_cper_rw_sem);
> 
>    guard(rwsem_write)(&cxl_cper_rw_sem);
> 
> > +	cper_notifier = notifier;
> 
> I would enforce that there is only one registrant and explicitly fail
> attempts to assign more than one.

Done.

> 
> > +	up_write(&cxl_cper_rw_sem);
> > +}
> > +EXPORT_SYMBOL_NS_GPL(cxl_cper_register_notifier, CXL);
> > +
> > +void cxl_cper_unregister_notifier(void)
> > +{
> > +	down_write(&cxl_cper_rw_sem);
> 
>    guard(rwsem_write)(&cxl_cper_rw_sem);
> 
> > +	cper_notifier = NULL;
> 
> This could enforce that the same callback specified at registration time
> must also be passed at unregistration to disallow anonymous
> unregistration.
> 
> Makes the code self documenting that the registrant is a singleton, and
> that unregistration must precede the next registration.

But what do we do if it does not match?  Returning an error will be
ignored by the cxl_pci_driver_exit() and if we enforce the singleton in
the registration I don't see a lot of room for error here.


[snip]

> > diff --git a/include/linux/cxl-event.h b/include/linux/cxl-event.h
> > index 18dab4d90dc8..c764ff877a6d 100644
> > --- a/include/linux/cxl-event.h
> > +++ b/include/linux/cxl-event.h
> > @@ -108,4 +108,55 @@ struct cxl_event_record_raw {
> >  	union cxl_event event;
> >  } __packed;
> >  
> > +enum cxl_event_type {
> > +	CXL_CPER_EVENT_GEN_MEDIA,
> > +	CXL_CPER_EVENT_DRAM,
> > +	CXL_CPER_EVENT_MEM_MODULE,
> > +};
> > +
> > +#pragma pack(1)
> 
> Looks like there is usage of __packed a few lines up, just pick
> one-style. Prefer __packed vs #pragma when only a small handful of
> structures need annotation as that is easier to check for correctness in
> patch form.

Ok I'll change it.  Smita requested the use of pragma but keeping the
__packed is redundant right now.  And I'll go with your preference of
__packed.

Ira