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

[Klaus Birkelund]

On Mon, Jun 03, 2019 at 09:30:53AM -0600, Heitke, Kenneth wrote:
> 
> 
> 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.
> 
 
I thought about this too when I reviewed, but came to the conclusion
that REALTIME was correct. The timestamp is basically a value that the
host stores in the controller. When the host uses Get Features to get
the the current time it would expect it to match the progression for its
own wall clockright? If I understand REALTIME vs VIRTUAL correctly,
using VIRTUAL, it would go way out of sync.

Klaus