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

[Kevin Wolf <kwolf@redhat.com>]

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.

Or did I read the code wrong and this isn't actually exposed to the
guest?

Kevin