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

[Ira Weiny <ira.weiny@intel.com>]

On Thu, Nov 17, 2022 at 10:43:37AM +0000, Jonathan Cameron wrote:
> On Wed, 16 Nov 2022 16:47:20 -0800
> Ira Weiny <ira.weiny@intel.com> wrote:
> 
> 

[snip]

> > 
> > >   
> > > > +			int i;
> > > > +
> > > > +			for (i = 0; i < nr_rec; i++)
> > > > +				trace_cxl_generic_event(dev_name(cxlds->dev),
> > > > +							type,
> > > > +							&payload.record[i]);
> > > > +		}
> > > > +
> > > > +		if (trace_cxl_overflow_enabled() &&
> > > > +		    (payload.flags & CXL_GET_EVENT_FLAG_OVERFLOW))
> > > > +			trace_cxl_overflow(dev_name(cxlds->dev), type, &payload);
> > > > +
> > > > +	} while (pl_nr > CXL_GET_EVENT_NR_RECORDS ||  
> > > 
> > > Isn't pl_nr > CXL_GET_EVENT_NR_RECORDS a hardware bug? It's the number in returned
> > > payload not the total number.  
> > 
> > I don't think so.  The only value passed to the device is the _input_ payload
> > size.  The output payload size is not passed to the device and is not included
> > in the Get Event Records Input Payload.  (Table 8-49)
> > 
> > So my previous code was wrong.  Here is an example I think which is within the
> > spec but would result in the more records flag not being set.
> > 
> > 	Device log depth == 10
> > 	nr log entries == 7
> > 	nr log entries in 1MB ~= (1M - hdr size) / 128 ~= 8000
> > 
> > Device sets Output Payload.Event Record Count == 7 (which is < 8000).  Common
> > mailbox code truncates that to 3.  More Event Records == 0 because it sent all
> > 7 that it had.
> > 
> > This code will clear 3 and read again 2 more times.
> > 
> > Am I reading that wrong?
> 
> I think this is still wrong, but for a different reason. :)

I hope not...  :-/

> If we don't clear the records and more records is set, that means it didn't
> fit in the mailbox payload (potentially 1MB)  then the next read
> will return the next set of records from there.

That is not how I read the Get Event Records command:

From 8.2.9.2.2 Get Event Records

... "Devices shall return event records to the host in the temporal order the
device detected the events in. The event occurring the earliest in time, in the
specific event log, shall be returned first."

If item 3 below is earlier than 4 then it must be returned if we have not
cleared it.  At least that is how I read the above.  :-/

> 
> Taking this patch only, let's say the mailbox takes 4 records.
> Read 1: Records 0, 1, 2, 3 More set.
>    We handle 0, 1, 2
> Read 2: Records 4, 5, 6 More not set.
>    We handle 4, 5, 6
> 
> Record 3 is never handled.
> 
> If we add in clearing as happens later in the series,

I suppose I should squash the patches as this may not work without the
clearing.  :-/

> the current
> assumption is that if we clear some records a subsequent read will
> start again.  I'm not sure that is true. If it is spec reference needed.
> 
> So assumption is
> Read 1: Records 0, 1, 2, 3 More set
>   Clear 0, 1, 2
> Read 2: Records 3, 4, 5, 6
>   Clear 3, 4, 5 More not set, but catch it with the condition above.
> Read 3: 6 only
>   Clear 6
> 
> However, I think a valid implementation could do the following
> (imagine a ring buffer with a pointer to the 'next' record to read out and
>  each record has a 'valid' flag to deal with corner cases around
>  sequences such as read log once, start reading again and some
>  clears occur using handles obtained from first read - not that
>  case isn't ruled out by the spec as far as I can see).

I believe this is a violation because the next pointer can't be advanced until
the record is cleared.  Otherwise the device is not returning items in temporal
order based on what is in the log.

> 
> Read 1: Records 0, 1, 2, 3 More set.  'next' pointer points to record 4.
>   Clear 0, 1, 2
> Read 2: Records 4, 5, 6 More not set. 'next' pointer points to record 7.
>   Clear 4, 5, 6
> 
> Skipping record 3.
> 
> So I think we have to absorb the full mailbox payload each time to guarantee
> we don't skip events or process them out of order (which is what would happen
> if we relied on a retry loop - we aren't allowed to clear them out of
> order anyway 8.2.9.2.3 "Events shall be cleared in temporal order. The device
> shall verify the event record handles specified in the input payload are in
> temporal order. ... "). 
> Obviously that temporal order thing is only relevant if we get my second
> example occurring on real hardware.  I think the spec is vague enough
> to allow that implementation.  Would have been easy to specify this originally
> but it probably won't go in as errata so we need to cope with all the
> flexibility that is present.

:-(  Yea coulda, woulda, shoulda...  ;-)

> 
> What fun and oh for a parameter to control how many records are returned!

Yea.  But I really don't think there is a problem unless someone really take
liberty with the spec.  I think it boils down to how one interprets _when_ a
record is removed from the log.

If the record is removed when it is returned (as in your 'next' pointer
example) then why have a clear at all?  If my interpretation is correct then
the next available entry is the one which has not been cleared.  Therefore in
your example 'next' is not incremented until clear has been called.  I think
that implementation is also supported by the idea that records must be cleared
in temporal order.  Otherwise I think devices would get confused.

FWIW the qemu implementation is based on my interpretation ATM.

Ira

> 
> Jonathan
> 
> 
> > 
> > >   
> > > > +		 payload.flags & CXL_GET_EVENT_FLAG_MORE_RECORDS);
> > > > +}  
> > > 
> 
> > 
>