Re: [PATCH v2 5/8] hw/cxl/events: Wire up get/clear event mailbox commands

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Wed, 21 Dec 2022 20:24:35 -0800
Ira Weiny <ira.weiny@intel.com> wrote:

> 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>
Hi Ira,

Before I forget.

In similar fashion to discussion on poison overflow timestamps,
the timestamping in here should take into account the
set timestamp command results so should look like the code
in cmd_timestamp_get()

https://elixir.bootlin.com/qemu/latest/source/hw/cxl/cxl-mailbox-utils.c#L165

One other trivial comment inline.

Not sure if you are going to get back to these.  I'm happy to just hack these
changes in if that is easier for you.

Thanks,

Jonathan

> 
> ---
> Change from RFC:
> 	Process multiple records per Get/Set per the spec
> 	Rework all the calls to be within events.c
> 	Add locking around the event logs to ensure that the log
> 		integrity is maintained
> ---

>
> +/*
> + * return if an interrupt should be generated as a result of inserting this
> + * event.
> + */
> +bool cxl_event_insert(CXLDeviceState *cxlds,
> +                      enum cxl_event_log_type log_type,
> +                      struct cxl_event_record_raw *event)
> +{
> +    uint64_t time = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
As noted, this needs to be more complex to take into account that host
and device timestamp base can be different (or it might not
be set at all for the device yet).

> +    struct cxl_event_log *log;
> +    CXLEvent *entry;
> +
> +    if (log_type >= CXL_EVENT_TYPE_MAX) {
> +        return false;
> +    }
> +
> +    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);
> +    if (!entry) {

No need to check. g_new0 failure results in application termination.
https://libsoup.org/glib/glib-Memory-Allocation.html

(this got pointed out to me the other day in an internal code review!)

> +        error_report("Failed to allocate memory for event log entry");
> +        return false;
> +    }