Re: [PATCH v3 1/2] cxl/core: correct length of DPA field masks

[Shiyang Ruan]

在 2024/4/24 5:04, Ira Weiny 写道:
> Alison Schofield wrote:
> > On Wed, Apr 17, 2024 at 03:50:52PM +0800, Shiyang Ruan wrote:
>
> [snip]
>
> > > diff --git a/drivers/cxl/core/trace.h b/drivers/cxl/core/trace.h
> > > index e5f13260fc52..cdfce932d5b1 100644
> > > --- a/drivers/cxl/core/trace.h
> > > +++ b/drivers/cxl/core/trace.h
> > > @@ -253,7 +253,7 @@ TRACE_EVENT(cxl_generic_event,
> > >    * DRAM Event Record
> > >    * CXL rev 3.0 section 8.2.9.2.1.2; Table 8-44
> > >    */
> > > -#define CXL_DPA_FLAGS_MASK                     0x3F
> > > +#define CXL_DPA_FLAGS_MASK                     0x3FULL
> > >   #define CXL_DPA_MASK                          (~CXL_DPA_FLAGS_MASK)
> > >   
> > >   #define CXL_DPA_VOLATILE			BIT(0)
> >
> > This works but I'm thinking this is the time to convene on one
> > CXL_EVENT_DPA_MASK for both all CXL events, rather than having
> > cxl_poison event be different.
> >
> > I prefer how poison defines it:
> >
> > cxlmem.h:#define CXL_POISON_START_MASK          GENMASK_ULL(63, 6)
> >
> > Can we rename that CXL_EVENT_DPA_MASK and use for all events?

cxlmem.h:CXL_POISON_START_MASK is defined for MBOX commands (poison 
record, the lower 3 bits indicate "Error Source"), but CXL_DPA_MASK here 
is for events.  They have different meaning though their values are 
same.  So, I don't think we should consolidate them.

> Ah!  Great catch.  I dont' know why I only masked off the 2 used bits.

Per spec, the lowest 2 bits of CXL event's DPA are defined for "Volatile 
or not" and "not repairable".  So there is no mistake here.

> That was short sighted of me.
>
> Yes we should consolidate these.
> Ira

--
Thanks,
Ruan.