Re: [PATCH 2/2] hw/cxl/cxl_event: Fix interrupt triggering for dynamic capacity events grouped via More flag

[Ira Weiny]

Jonathan Cameron wrote:
> On Tue, 27 Aug 2024 09:40:05 -0700
> nifan.cxl@gmail.com wrote:
> 
> > From: Fan Ni <fan.ni@samsung.com>
> > 
> > When inserting multiple dynamic capacity event records grouped via More 
> > flag,
> > we should only trigger interrupt after the last record is inserted into the
> > event log. Achieving the goal by letting cxl_event_insert return true only
> > for the insertion of the last dynamic capacity event record in the sequence.
> 
> I'm not sure this one is accurate.  We might well have a slow
> system provisioning capacity one extent at time (and interrupting).
> 
> The event buffer might also not be large enough to hold all records so
> the device might 'wait' before figuring out the next extent for there
> to be somewhere to put the record.
> 
> Overall I think we can interrupt on each one and it should 'work'
> as should interrupt only once there are lots of them or
> every (n).

Indeed I think it should work.  But you won't see any extents as they will
be pending in the memdev.

Did this fail in some way?  I'm sorry I did not try and use qemu to test
the more bit.  Rather I used cxl_test for that.

Ira

> 
> Interrupt only fires on a 0 to >= 1 transition anyway, not
> on repeats after that unless the log has been cleared.
> It's up to OS to keep clearing records until it at least
> momentarily hits 0 if it wants to get any more interrupts.
> 
> Jonathan
> 
> 
> > 
> > Signed-off-by: Fan Ni <fan.ni@samsung.com>
> > ---
> >  hw/cxl/cxl-events.c         | 8 ++++++++
> >  include/hw/cxl/cxl_events.h | 1 +
> >  2 files changed, 9 insertions(+)
> > 
> > diff --git a/hw/cxl/cxl-events.c b/hw/cxl/cxl-events.c
> > index 12dee2e467..90536c0e68 100644
> > --- a/hw/cxl/cxl-events.c
> > +++ b/hw/cxl/cxl-events.c
> > @@ -135,6 +135,14 @@ bool cxl_event_insert(CXLDeviceState *cxlds, 
> > CXLEventLogType log_type,
> >      QSIMPLEQ_INSERT_TAIL(&log->events, entry, node);
> >      cxl_event_set_status(cxlds, log_type, true);
> >  
> > +    /*
> > +     * For dynamic capacity event records grouped via More flag,
> > +     * Only raise interrupt after inserting the last record in the log.
> > +     */
> > +    if (log_type == CXL_EVENT_TYPE_DYNAMIC_CAP) {
> > +        CXLEventDynamicCapacity *dCap = (CXLEventDynamicCapacity *)event;
> > +        return (dCap->flags & MORE_FLAG) ? false : true;
> > +    }
> >      /* Count went from 0 to 1 */
> >      return cxl_event_count(log) == 1;
> 
> If there are multiple this will fail I think as cxl_event_count(log) will go 
> from 0
> to X not 1.
> 
> >  }
> > diff --git a/include/hw/cxl/cxl_events.h b/include/hw/cxl/cxl_events.h
> > index 38cadaa0f3..b0e5cc89c0 100644
> > --- a/include/hw/cxl/cxl_events.h
> > +++ b/include/hw/cxl/cxl_events.h
> > @@ -170,6 +170,7 @@ typedef struct CXLEventMemoryModule {
> >   * CXL r3.1 section Table 8-50: Dynamic Capacity Event Record
> >   * All fields little endian.
> >   */
> > +#define MORE_FLAG BIT_MASK(0)
> >  typedef struct CXLEventDynamicCapacity {
> >      CXLEventRecordHdr hdr;
> >      uint8_t type;
>