| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <4881660.OV4Wx5bFTl@fdefranc-mobl3>
Date: Thu, 06 Jun 2024 12:42:44 +0200
From: "Fabio M. De Francesco" <fabio.m.de.francesco@...ux.intel.com>
To: Jonathan Cameron <Jonathan.Cameron@...wei.com>
Cc: Davidlohr Bueso <dave@...olabs.net>, Dave Jiang <dave.jiang@...el.com>,
Alison Schofield <alison.schofield@...el.com>,
Vishal Verma <vishal.l.verma@...el.com>, Ira Weiny <ira.weiny@...el.com>,
Dan Williams <dan.j.williams@...el.com>, linux-cxl@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject:
Re: [PATCH v6] cxl/events: Use a common struct for DRAM and General Media
events
On Wednesday, June 5, 2024 1:30:00 PM GMT+2 Jonathan Cameron wrote:
> On Sun, 2 Jun 2024 21:12:25 +0200
> "Fabio M. De Francesco" <fabio.m.de.francesco@...ux.intel.com> wrote:
>
> > cxl_event_common was an unfortunate naming choice and caused confusion
with
> > the existing Common Event Record. Furthermore, its fields didn't map all
> > the common information between DRAM and General Media Events.
> >
> > Remove cxl_event_common and introduce cxl_event_media_hdr to record common
> > information between DRAM and General Media events.
> >
> > cxl_event_media_hdr, which is embedded in both cxl_event_gen_media and
> > cxl_event_dram, leverages the commonalities between the two events to
> > simplify their respective handling.
> >
> > Suggested-by: Dan Williams <dan.j.williams@...el.com>
> > Reviewed-by: Alison Schofield <alison.schofield@...el.com>
> > Reviewed-by: Dan Williams <dan.j.williams@...el.com>
> > Reviewed-by: Ira Weiny <ira.weiny@...el.com>
> > Signed-off-by: Fabio M. De Francesco <fabio.m.de.francesco@...ux.intel.com>
> > ---
> >
> > - Changes for v6 -
> >
> > - Add "Reviewed-by" tags
> Hi Fabio,
Hi Jonathan,
> Adding tags doesn't need a new version. b4 or however Dave is picking these
> up will gather them up from the v5 posting.
I just read what you replied in the v5 thread and know that now it makes more
sense.
> My one nervousness about this is the impression it perhaps gives that the
contents
> of validity flags has the same meaning in both records.
>
> Maybe a comment to that effect next to it's definition makes sense?
Yes, sure.
I'll add it.
> I think the alternative of not including that or the remaining elements in
> your common structure would be worse.
>
> Jonathan
>
Thank you,
Fabio
>
> >
> > - Changes for v5 -
> >
> > - Rebase on v6.10-rc1
> >
> > - Changes for v4 -
> >
> > - Initialise cxl_test_dram and cxl_test_gen_media without
> > unnecessary extra de-references (Dan)
> > - Add a comment for media_hdr in union cxl_event (Alison)
> >
> > - Changes for v3 -
> >
> > - Rework the layout of cxl_event_dram and cxl_event_gen_media to
> > make a simpler change (Dan)
> > - Remove a "Fixes" tag (Dan)
> > - Don't use unnecessary struct_group[_tagged] (Jonathan, Ira)
> > - Rewrite end extend the commit message
> >
> > - Link to v4 -
> >
> > https://lore.kernel.org/linux-cxl/20240521140750.26035-1-fabio.m.de.francesco@linux.intel.com/
> >
> > drivers/cxl/core/mbox.c | 2 +-
> > drivers/cxl/core/trace.h | 32 ++++++++++-----------
> > include/linux/cxl-event.h | 41 ++++++++++-----------------
> > tools/testing/cxl/test/mem.c | 54 +++++++++++++++++++-----------------
> > 4 files changed, 61 insertions(+), 68 deletions(-)
>
>
>
> > diff --git a/include/linux/cxl-event.h b/include/linux/cxl-event.h
> > index 60b25020281f..1119d0bbb091 100644
> > --- a/include/linux/cxl-event.h
> > +++ b/include/linux/cxl-event.h
> > @@ -21,6 +21,17 @@ struct cxl_event_record_hdr {
> > u8 reserved[15];
> > } __packed;
> >
> > +struct cxl_event_media_hdr {
> > + struct cxl_event_record_hdr hdr;
> > + __le64 phys_addr;
> > + u8 descriptor;
> > + u8 type;
> > + u8 transaction_type;
> > + u8 validity_flags[2];
>
> Perhaps a comment to say that validity_flags meaning after bit 2
> varies across the different records?
>
> > + u8 channel;
> > + u8 rank;
> > +} __packed;
> > +
> > #define CXL_EVENT_RECORD_DATA_LENGTH 0x50
> > struct cxl_event_generic {
> > struct cxl_event_record_hdr hdr;
> > @@ -33,14 +44,7 @@ struct cxl_event_generic {
> > */
> > #define CXL_EVENT_GEN_MED_COMP_ID_SIZE 0x10
> > struct cxl_event_gen_media {
> > - struct cxl_event_record_hdr hdr;
> > - __le64 phys_addr;
> > - u8 descriptor;
> > - u8 type;
> > - u8 transaction_type;
> > - u8 validity_flags[2];
> > - u8 channel;
> > - u8 rank;
> > + struct cxl_event_media_hdr media_hdr;
> > u8 device[3];
> > u8 component_id[CXL_EVENT_GEN_MED_COMP_ID_SIZE];
> > u8 reserved[46];
> > @@ -52,14 +56,7 @@ struct cxl_event_gen_media {
> > */
> > #define CXL_EVENT_DER_CORRECTION_MASK_SIZE 0x20
> > struct cxl_event_dram {
> > - struct cxl_event_record_hdr hdr;
> > - __le64 phys_addr;
> > - u8 descriptor;
> > - u8 type;
> > - u8 transaction_type;
> > - u8 validity_flags[2];
> > - u8 channel;
> > - u8 rank;
> > + struct cxl_event_media_hdr media_hdr;
> > u8 nibble_mask[3];
> > u8 bank_group;
> > u8 bank;
> > @@ -95,21 +92,13 @@ struct cxl_event_mem_module {
> > u8 reserved[0x3d];
> > } __packed;
> >
> > -/*
> > - * General Media or DRAM Event Common Fields
> > - * - provides common access to phys_addr
> > - */
> > -struct cxl_event_common {
> > - struct cxl_event_record_hdr hdr;
> > - __le64 phys_addr;
> > -} __packed;
> > -
> > union cxl_event {
> > struct cxl_event_generic generic;
> > struct cxl_event_gen_media gen_media;
> > struct cxl_event_dram dram;
> > struct cxl_event_mem_module mem_module;
> > - struct cxl_event_common common;
> > + /* dram & gen_media event header */
> > + struct cxl_event_media_hdr media_hdr;
> > } __packed;
>
>
>
>
>
Powered by blists - more mailing lists