Shiyang Ruan wrote: > > > 在 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. By definition the DPA in all these payloads can't use the lower 6 bits. We are defining a mask to get the DPA. This has nothing to do with what may be stored in the other 6 bits. Defining a common DPA mask is correct per both sections of the spec. > > > > > 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. I appreciate your not calling out my code as a bug. :-D However, bits [5:2] are also Reserved. So it is incorrect to mask off only the lower 2 bits. Even though the reserved bits must be 0 per the spec, it is still better to properly mask all reserved bits because they are by definition not part of the DPA. Could you create a common macro for the next version of the patch? Thanks, Ira > > > That was short sighted of me. > > > > Yes we should consolidate these. > > Ira > > -- > Thanks, > Ruan. >
