Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:6 as permitted sender) client-ip=2620:137:e000::3:6;
Date:   Fri, 8 Dec 2023 10:21:49 -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 1/6] cxl/trace: Pass uuid explicitly to event traces
Message-ID: <65735ebd531b5_1e7d27294c7@iweiny-mobl.notmuch>
References: <20230601-cxl-cper-v1-0-d19f1ac18ab6@intel.com>
 <20230601-cxl-cper-v1-1-d19f1ac18ab6@intel.com>
 <657263c249c4c_45e0129471@dwillia2-xfh.jf.intel.com.notmuch>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <657263c249c4c_45e0129471@dwillia2-xfh.jf.intel.com.notmuch>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?n2PzhorG0bna5swTIVILQKJEVDfFt9VS3r5R43c15pGlbGrd1TvAzLzU7+Je?=
 =?us-ascii?Q?gKy8EDZkzYAiqP3oN03Y+cDTLSQLxQ9Cc3vFFZXNwWB2I4d7kCDBC42TuB74?=
 =?us-ascii?Q?ZL1SSPr+OOMf14V97ZrJdqfEouzAPaspWZB1d7SzB9U3jkoICeAYYBQ/Fg8u?=
 =?us-ascii?Q?CLPqXhrazx1d2EqDqosRyflN3oxezcMqKiqV4catTTllcXfdKrOZRxCJhz2j?=
 =?us-ascii?Q?sKiIieGvM/KD8pvL0dpfZtadAG6LrbKvioNaEfPGY1AMegJd35YoT19KGlOD?=
 =?us-ascii?Q?CR9035ltmsnDtZ9zpFDPjQKo9Q1W8+q4M/YSacpfDR3jsFf5LMAU4W9Edzq2?=
 =?us-ascii?Q?9Cffy6zVYdbnU/Fv2SQW8NXv3gtfRjNGTJakdujmR+KFdXZ/WjjrV75bwrAO?=
 =?us-ascii?Q?rGxhoRj95DL/eXFCQpGZW6FEoZPGkPw0JJ1BaaM0QwolaZdT+tpfuNV55Ig/?=
 =?us-ascii?Q?aRFHtuJUNnGp9TiwB1Ko8UoYCJrPrIoqaM2ht2hbp0ufgE2zONdHzIT2uLvT?=
 =?us-ascii?Q?wAzF08927DQqaC46X8HvuKHQaXfPBIbdhFw+t4djcX8mDaI2TNJ/gKnfcHY0?=
 =?us-ascii?Q?vSMK6aL+spvYIvH4kh7f8iv4u+lkMSUsIjZUF+3NYCzGRMSmfaNDrTWsu9Oq?=
 =?us-ascii?Q?vX+rrEtJGI2su/CGgy6k3gVJOxyL9EuvHAfqTfAtNxAEd2SNWfRJ1AxrIxHz?=
 =?us-ascii?Q?nFZs3BAm+171Q3FuwXaTAnxZWPYudvbj2cUq9jeEsP2DnNyRHl4vKrFbGufj?=
 =?us-ascii?Q?PwZD+cYXXau+s1DVRaoLF2jd0jrkLSKhSAUACL9dK2/VBFvo7X9QOYFchqEN?=
 =?us-ascii?Q?Gwurli+3+GG0cTClV69bhF6JQhNWQTluuqVzCa9p2TrtuaxqsAvtdn0XMNWe?=
 =?us-ascii?Q?X/aO8creFAwaoBgIWTEqX7OA9PVn8WA6CxOalWhJMAQtRNmQqcOGKoGdhODy?=
 =?us-ascii?Q?MEBz9J1hBJrIQP3zfRwsEXDqd9XFmtbJ1SkyqrLXnL++JXzEEnyJuV4LvY/i?=
 =?us-ascii?Q?vaM0KHV3D7eG0yqSLapt7joR3GUaTHkrgcI1qPXKCvDCo0jAK3KtbUJqSJwp?=
 =?us-ascii?Q?8btkotTkwFPqmeZ4JT7LPnbXHXelSet33lQCHwldOhKYA+x1dU7oVKufPp2o?=
 =?us-ascii?Q?dsbPXyoi6nvpieRao+5ECv595GXRoVXUWXeDdU0czSFpPuVP3OtdD8KEMCmN?=
 =?us-ascii?Q?F9luulwZ/TyxG4Ta/DhnUNlISjIeJwUurKx8IRXh0fQYdi62FxVSZp5YhLbx?=
 =?us-ascii?Q?gk8AgFmPJMaiqnVYFpNitlWbM7iQ1MBL88aRdqQ7yjNCMlK1LTBvXbNQBb+N?=
 =?us-ascii?Q?wnLOuxCPE3pHE4Cpv9SEbhqjYrcdXTIx+t2C4fu3C0Ga25L3sgBbwj/ycHje?=
 =?us-ascii?Q?YRY/j1loOuhWeGyP1rLsWz1V9kthKD05CMAsRNqPO4EHLJZwUhZuGMf0isjw?=
 =?us-ascii?Q?njIqgSIh/FuV7DdYprKlGNFcDqj1OiaHCiACrET9X8prCym7PIdIZ6iwfGlq?=
 =?us-ascii?Q?EBrTPq+fjPLFN+gbC7aHIjB9ZI1YPU12+y3ix5j6js4dyElDPAKHR+NTndIp?=
 =?us-ascii?Q?jwhPKroaA70Yj5m16ROQeeh3oTtJ5j+FPHyRyjlu?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 0a57f934-a203-4d9d-a4f9-08dbf81a8dd1
X-MS-Exchange-CrossTenant-AuthSource: SA1PR11MB6733.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 08 Dec 2023 18:21:53.7019
 (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: cq/r6jrDEX2K8jRdr+Y9DNvsuqlYSNGxxWf0+CB3qde3+kxozLnf56o27hVRh14b7C3gcSDTHCa/b7myutM3sQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH8PR11MB6610
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE
	autolearn=unavailable autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on pete.vger.email
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 (pete.vger.email [0.0.0.0]); Fri, 08 Dec 2023 10:22:13 -0800 (PST)

Dan Williams wrote:
> Ira Weiny wrote:
> > CPER CXL events do not have a UUID associated with them.  It is
> > desirable to share event structures between the CPER CXL event and the
> > CXL event log events.
> 
> This parses strange to me, maybe it is trying to capture too many
> changes in too few sentences? Something like this instead?

Yea probably just trying to squeeze too much in.

> 
> "CXL CPER events are identified by the CPER Section Type GUID. The GUID
> correlates with the CXL UUID for the event record. It turns out that a
> CXL CPER record is a strict subset of the CXL event record, only the
> UUID header field is chopped.
> 
> In order to unify handling between native and CPER flavors of CXL
> events, prepare the code for the UUID to be passed in rather than
> inferred from the record itself.
> 
> Later patches update the passed in record to only refer to the common
> data between the formats."

That looks much more detailed.  I'll add it in.


[snip]

> > diff --git a/drivers/cxl/core/trace.h b/drivers/cxl/core/trace.h
> > index a0b5819bc70b..2aef185f4cd0 100644
> > --- a/drivers/cxl/core/trace.h
> > +++ b/drivers/cxl/core/trace.h
> > @@ -189,7 +189,7 @@ TRACE_EVENT(cxl_overflow,
> >  	__string(memdev, dev_name(&cxlmd->dev))			\
> >  	__string(host, dev_name(cxlmd->dev.parent))		\
> >  	__field(int, log)					\
> > -	__field_struct(uuid_t, hdr_uuid)			\
> > +	__field_struct(uuid_t, uuid)				\
> 
> Hmm, is it too late to make this rename? I.e. would a script that is
> looking for "hdr_uuid" in one kernel be broken by the rename of the
> field to "uuid" in the next?

True, probably best to leave the field name alone.  I'll change it back.

> 
> >  	__field(u64, serial)					\
> >  	__field(u32, hdr_flags)					\
> >  	__field(u16, hdr_handle)				\
> > @@ -198,12 +198,12 @@ TRACE_EVENT(cxl_overflow,
> >  	__field(u8, hdr_length)					\
> >  	__field(u8, hdr_maint_op_class)
> >  
> > -#define CXL_EVT_TP_fast_assign(cxlmd, l, hdr)					\
> > +#define CXL_EVT_TP_fast_assign(cxlmd, l, uuid, hdr)				\
> >  	__assign_str(memdev, dev_name(&(cxlmd)->dev));				\
> >  	__assign_str(host, dev_name((cxlmd)->dev.parent));			\
> >  	__entry->log = (l);							\
> >  	__entry->serial = (cxlmd)->cxlds->serial;				\
> > -	memcpy(&__entry->hdr_uuid, &(hdr).id, sizeof(uuid_t));			\
> > +	memcpy(&__entry->uuid, (uuid), sizeof(uuid_t));				\
> 
> Per above this would be:
> 
> 	memcpy(&__entry->hdr_uuid, (uuid), sizeof(uuid_t));				\

yep.

> 
> >  	__entry->hdr_length = (hdr).length;					\
> >  	__entry->hdr_flags = get_unaligned_le24((hdr).flags);			\
> >  	__entry->hdr_handle = le16_to_cpu((hdr).handle);			\
> > @@ -217,7 +217,7 @@ TRACE_EVENT(cxl_overflow,
> >  		"maint_op_class=%u : " fmt,					\
> >  		__get_str(memdev), __get_str(host), __entry->serial,		\
> >  		cxl_event_log_type_str(__entry->log),				\
> > -		__entry->hdr_timestamp, &__entry->hdr_uuid, __entry->hdr_length,\
> > +		__entry->hdr_timestamp, &__entry->uuid, __entry->hdr_length,	\
> 
> ...and this change could be dropped.

Yep.

All done.

> 
> Other than that, this looks good to me.

Thanks,
Ira