Re: [RFC V2 PATCH 02/11] cxl/mem: Implement Get Event Records command

[Ira Weiny]

On Thu, Oct 20, 2022 at 04:50:30PM -0500, Smita Koralahalli wrote:
> Hi Ira,
> 
> On 10/10/22 5:41 PM, ira.weiny@xxxxxxxxx wrote:
> > From: Ira Weiny <ira.weiny@xxxxxxxxx>
> > 
> > 

[snip]

> > +
> > +/**
> > + * cxl_mem_get_event_records - Get Event Records from the device
> > + * @cxlds: The device data for the operation
> > + *
> > + * Retrieve all event records available on the device and report them as trace
> > + * events.
> > + *
> > + * See CXL rev 3.0 @8.2.9.2.2 Get Event Records
> > + */
> > +void cxl_mem_get_event_records(struct cxl_dev_state *cxlds)
> > +{
> > +	enum cxl_event_log_type log_type;
> > +
> > +	dev_dbg(cxlds->dev, "Reading event logs\n");
> > +
> > +	for (log_type = CXL_EVENT_TYPE_INFO;
> > +	     log_type < CXL_EVENT_TYPE_MAX; log_type++)
> 
> Why should we loop through each event log here?

The idea was to clear all the event logs.  I think this made more sense before
the addition of the optional dynamic capacity log.

>
> What if the event
> record doesn't exist in the event log?

An empty log does not cause any issue.  The query will simply return 0 records
which is valid.

> 
> I got some Mailbox error messages like this while bootup..
> [  346.387010] cxl_pci 0000:7f:00.0: Sending command
> [  346.387181] cxl_pci 0000:7f:00.0: Doorbell wait took 0ms
> [  346.387197] cxl_pci 0000:7f:00.0: Mailbox operation had an error: cmd
> input was invalid
> [  346.387205] cxl_pci 0000:7f:00.0: Event log 'Warning': Failed to query
> event records : -6
> ..
> 
> Can we just read the "Event Status" field from Event Status Register
> (Device Status Registers Capability Offset + 00h) 8.2.8.3.1 in CXL Spec,
> determine if the records exist and just query those event logs?

Likely the hardware does not have the dynamic capacity log and so the code is
asking for something invalid.  I did not think of that when I added that new
log.  Checking status register looks to be the proper solution.

I'll throw in some testing in QEMU for this.  I'll also have to implement the
status register in QEMU to fully test.

Thanks for the testing!  :-D

Ira

> 
> Thanks,
> Smita
> 
> > +		cxl_mem_get_records_log(cxlds, log_type);
> > +}
> > +EXPORT_SYMBOL_NS_GPL(cxl_mem_get_event_records, CXL);
> > +
> >   /**
> >    * cxl_mem_get_partition_info - Get partition info
> >    * @cxlds: The device data for the operation