Re: [PATCH v4 09/42] hw/cxl/device: Timestamp implementation (8.2.9.3)

[Jonathan Cameron]

On Thu, 27 Jan 2022 11:50:08 +0000
Alex Bennée <alex.bennee@linaro.org> wrote:

> Jonathan Cameron <Jonathan.Cameron@huawei.com> writes:
> 
> > From: Ben Widawsky <ben.widawsky@intel.com>
> >
> > Per spec, timestamp appears to be a free-running counter from a value
> > set by the host via the Set Timestamp command (0301h). There are
> > references to the epoch, which seem like a red herring. Therefore, the
> > implementation implements the timestamp as freerunning counter from the
> > last value that was issued by the Set Timestamp command.
> >
> > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > ---
> >  hw/cxl/cxl-mailbox-utils.c  | 53 +++++++++++++++++++++++++++++++++++++
> >  include/hw/cxl/cxl_device.h |  6 +++++
> >  2 files changed, 59 insertions(+)
> >
> > diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> > index 1a87846356..cea4b2a59c 100644
> > --- a/hw/cxl/cxl-mailbox-utils.c
> > +++ b/hw/cxl/cxl-mailbox-utils.c
> > @@ -43,6 +43,9 @@ enum {
> >          #define CLEAR_RECORDS   0x1
> >          #define GET_INTERRUPT_POLICY   0x2
> >          #define SET_INTERRUPT_POLICY   0x3
> > +    TIMESTAMP   = 0x03,
> > +        #define GET           0x0
> > +        #define SET           0x1
> >  };
> >  
> >  /* 8.2.8.4.5.1 Command Return Codes */
> > @@ -117,8 +120,11 @@ define_mailbox_handler_zeroed(EVENTS_GET_RECORDS, 
> > 0x20);
> >  define_mailbox_handler_nop(EVENTS_CLEAR_RECORDS);
> >  define_mailbox_handler_zeroed(EVENTS_GET_INTERRUPT_POLICY, 4);
> >  define_mailbox_handler_nop(EVENTS_SET_INTERRUPT_POLICY);
> > +declare_mailbox_handler(TIMESTAMP_GET);
> > +declare_mailbox_handler(TIMESTAMP_SET);
> >  
> >  #define IMMEDIATE_CONFIG_CHANGE (1 << 1)
> > +#define IMMEDIATE_POLICY_CHANGE (1 << 3)
> >  #define IMMEDIATE_LOG_CHANGE (1 << 4)
> >  
> >  #define CXL_CMD(s, c, in, cel_effect) \
> > @@ -129,10 +135,57 @@ static struct cxl_cmd cxl_cmd_set[256][256] = {
> >      CXL_CMD(EVENTS, CLEAR_RECORDS, ~0, IMMEDIATE_LOG_CHANGE),
> >      CXL_CMD(EVENTS, GET_INTERRUPT_POLICY, 0, 0),
> >      CXL_CMD(EVENTS, SET_INTERRUPT_POLICY, 4, IMMEDIATE_CONFIG_CHANGE),
> > +    CXL_CMD(TIMESTAMP, GET, 0, 0),
> > +    CXL_CMD(TIMESTAMP, SET, 8, IMMEDIATE_POLICY_CHANGE),
> >  };
> >  
> >  #undef CXL_CMD
> >  
> > +/*
> > + * 8.2.9.3.1
> > + */
> > +define_mailbox_handler(TIMESTAMP_GET)
> > +{
> > +    struct timespec ts;
> > +    uint64_t delta;
> > +
> > +    if (!cxl_dstate->timestamp.set) {
> > +        *(uint64_t *)cmd->payload = 0;
> > +        goto done;
> > +    }
> > +
> > +    /* First find the delta from the last time the host set the time. */
> > +    clock_gettime(CLOCK_REALTIME, &ts);  
> 
> Could you consider using qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL)?
> Otherwise by introducing a dependency on real time you'll loose the
> ability to get deterministic execution via icount.

I don't have a strong opinion either way, so Ill go with 
qemu_clock_get_ns() and see if any comments on it in v5.

I've also updated the commit message as the catch all errata
which was F4 included a clarification that the timestamp
was as implemented here.

Thanks,

Jonathan