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

[Klaus Birkelund <klaus@birkelund.eu>]

On Tue, Jun 04, 2019 at 10:46:45AM +0200, Kevin Wolf wrote:
> Am 04.06.2019 um 10:28 hat Klaus Birkelund geschrieben:
> > 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 <kenneth.heitke@intel.com>
> > > > 
> > > > > > 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.
> 
> Which two things would go out of sync with VIRTUAL?
> 
> Not an expert on clocks myself, but I think the main question is what
> happens to the clock while the VM is stopped. REALTIME continues running
> where as VIRTUAL is stopped. If we expose REALTIME measurements to the
> guest, the time passed may look a lot longer than what the guest's clock
> actually says. So this is the thing I am worried would go out of sync
> with REALTIME.
> 

OK, fair point.

Thinking about this some more, I agree that VIRTUAL is more correct. An
application should never track elapsed time using real wall clock time,
but some monotonic clock that is oblivious to say NTP adjustments.

Klaus