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

[Heitke, Kenneth]

On 6/4/2019 3:13 AM, Klaus Birkelund wrote:
> 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 <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.
> >
> > 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

Kevin, would you like me to update the patch to reflect this change or 
will you make the change directly?

thanks,
Ken