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

[Kevin Wolf]

Am 04.06.2019 um 19:06 hat Heitke, Kenneth geschrieben:
> 
> 
> 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?

I already made it directly.

Kevin