[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH v2] nvme: add Get/Set Feature Timestamp support
From: |
Klaus Birkelund |
Subject: |
Re: [Qemu-block] [PATCH v2] nvme: add Get/Set Feature Timestamp support |
Date: |
Tue, 4 Jun 2019 11:13:58 +0200 |
User-agent: |
Mutt/1.12.0 (2019-05-25) |
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
- Re: [Qemu-block] [PATCH v2] nvme: add Get/Set Feature Timestamp support, Kevin Wolf, 2019/06/03
- Re: [Qemu-block] [PATCH v2] nvme: add Get/Set Feature Timestamp support, Heitke, Kenneth, 2019/06/03
- Re: [Qemu-block] [PATCH v2] nvme: add Get/Set Feature Timestamp support, Klaus Birkelund, 2019/06/04
- Re: [Qemu-block] [PATCH v2] nvme: add Get/Set Feature Timestamp support, Kevin Wolf, 2019/06/04
- Re: [Qemu-block] [PATCH v2] nvme: add Get/Set Feature Timestamp support,
Klaus Birkelund <=
- Re: [Qemu-block] [PATCH v2] nvme: add Get/Set Feature Timestamp support, Kevin Wolf, 2019/06/04
- Re: [Qemu-block] [PATCH v2] nvme: add Get/Set Feature Timestamp support, Heitke, Kenneth, 2019/06/04
- Re: [Qemu-block] [PATCH v2] nvme: add Get/Set Feature Timestamp support, Kevin Wolf, 2019/06/05