Re: [PATCH v7 3/7] hw/cxl/events: Wire up get/clear event mailbox commands

[Ira Weiny]

Fan Ni wrote:
> The 05/22/2023 16:09, Jonathan Cameron wrote:
> > From: Ira Weiny <ira.weiny@intel.com>
> > 
> > CXL testing is benefited from an artificial event log injection
> > mechanism.
> > 
> > Add an event log infrastructure to insert, get, and clear events from
> > the various logs available on a device.
> > 
> > Replace the stubbed out CXL Get/Clear Event mailbox commands with
> > commands that operate on the new infrastructure.
> > 
> > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > ---
> 
> Reviewed-by: Fan Ni <fan.ni@samsung.com>
> 
> See comments below in cxl_event_insert.
> 

[snip]

> > +
> > +/*
> > + * return true if an interrupt should be generated as a result
> > + * of inserting this event.
> > + */
> > +bool cxl_event_insert(CXLDeviceState *cxlds, CXLEventLogType log_type,
> > +                      CXLEventRecordRaw *event)
> > +{
> > +    uint64_t time;
> > +    CXLEventLog *log;
> > +    CXLEvent *entry;
> > +
> > +    if (log_type >= CXL_EVENT_TYPE_MAX) {
> > +        return false;
> > +    }
> > +
> > +    time = cxl_device_get_timestamp(cxlds);
> > +
> > +    log = &cxlds->event_logs[log_type];
> > +
> > +    QEMU_LOCK_GUARD(&log->lock);
> > +
> > +    if (cxl_event_count(log) >= CXL_TEST_EVENT_OVERFLOW) {
> > +        if (log->overflow_err_count == 0) {
> > +            log->first_overflow_timestamp = time;
> > +        }
> > +        log->overflow_err_count++;
> > +        log->last_overflow_timestamp = time;
> > +        return false;
> > +    }
> > +
> > +    entry = g_new0(CXLEvent, 1);
> > +
> > +    memcpy(&entry->data, event, sizeof(*event));
> > +
> > +    entry->data.hdr.handle = cpu_to_le16(log->next_handle);
> > +    log->next_handle++;
> > +    /* 0 handle is never valid */
> > +    if (log->next_handle == 0) {
> 
> next_handle is uint16_t, how can it be 0 after next_handle++?
> 

If enough events are added and it overflows back to 0.

> > +        log->next_handle++;

This statement then rolls it to 1 to keep it from ever being 0.

Thanks for the review!

Ira