Re: [Qemu-devel] [PATCH v2] nvme: add Get/Set Feature Timestamp support

[Heitke, Kenneth]

On 6/3/2019 5:14 AM, Kevin Wolf wrote:
> Am 28.05.2019 um 08:18 hat Klaus Birkelund geschrieben:
> > On Mon, May 20, 2019 at 11:40:30AM -0600, Kenneth Heitke wrote:
> > > Signed-off-by: Kenneth Heitke <address@hidden>
>
> > > diff --git a/hw/block/nvme.h b/hw/block/nvme.h
> > > index 56c9d4b4b1..d7277e72b7 100644
> > > --- a/hw/block/nvme.h
> > > +++ b/hw/block/nvme.h
> > > @@ -69,6 +69,7 @@ typedef struct NvmeCtrl {
> > >       uint16_t    max_prp_ents;
> > >       uint16_t    cqe_size;
> > >       uint16_t    sqe_size;
> > > +    uint16_t    oncs;
> >
> > Looks like this unused member snuck its way into the patch. But I see no
> > harm in it being there.
>
> Good catch. I'll just remove it again from my branch.
>
> > > +static inline void nvme_set_timestamp(NvmeCtrl *n, uint64_t ts)
> > > +{
> > > +    trace_nvme_setfeat_timestamp(ts);
> > > +
> > > +    n->host_timestamp = le64_to_cpu(ts);
> > > +    n->timestamp_set_qemu_clock_ms = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
> > > +}
> > > +
> > > +static inline uint64_t nvme_get_timestamp(const NvmeCtrl *n)
> > > +{
> > > +    uint64_t current_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
>
> Here I wonder why we use QEMU_CLOCK_REALTIME in a device emulation.
> Wouldn't QEMU_CLOCK_VIRTUAL make more sense?

QEMU_CLOCK_VIRTUAL probably would make more sense. When I was reading 
through the differences I wasn't really sure what to pick. iven that 
this is the time within the device's context, the virtual time seems 
more correct.

> Kevin