On 7/17/20 5:20 PM, Jessica Clarke wrote:
> The specification says:
>
> 0x00 TIME_LOW R: Get current time, then return low-order 32-bits.
> 0x04 TIME_HIGH R: Return high 32-bits from previous TIME_LOW read.
>
> ...
>
> To read the value, the kernel must perform an IO_READ(TIME_LOW),
> which returns an unsigned 32-bit value, before an IO_READ(TIME_HIGH),
> which returns a signed 32-bit value, corresponding to the higher half
> of the full value.
>
> However, we were just returning the current time for both. If the guest
> is unlucky enough to read TIME_LOW and TIME_HIGH either side of an
> overflow of the lower half, it will see time be in the future, before
> jumping backwards on the next read, and Linux currently relies on the
> atomicity guaranteed by the spec so is affected by this. Fix this
> violation of the spec by caching the correct value for TIME_HIGH
> whenever TIME_LOW is read, and returning that value for any TIME_HIGH
> read.
>
> Signed-off-by: Jessica Clarke <jrt...@jrtc27.com>
> ---
> hw/rtc/goldfish_rtc.c | 14 ++++++++++++--
> include/hw/rtc/goldfish_rtc.h | 1 +
> 2 files changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/hw/rtc/goldfish_rtc.c b/hw/rtc/goldfish_rtc.c
> index 01e9d2b083..9b577bf159 100644
> --- a/hw/rtc/goldfish_rtc.c
> +++ b/hw/rtc/goldfish_rtc.c
> @@ -94,12 +94,22 @@ static uint64_t goldfish_rtc_read(void *opaque, hwaddr
> offset,
> GoldfishRTCState *s = opaque;
> uint64_t r = 0;
>
> + /*
> + * From the documentation linked at the top of the file:
> + *
> + * To read the value, the kernel must perform an IO_READ(TIME_LOW),
> which
> + * returns an unsigned 32-bit value, before an IO_READ(TIME_HIGH),
> which
> + * returns a signed 32-bit value, corresponding to the higher half of
> the
> + * full value.
> + */
> switch (offset) {
> case RTC_TIME_LOW:
> - r = goldfish_rtc_get_count(s) & 0xffffffff;
> + r = goldfish_rtc_get_count(s);
> + s->time_high = r >> 32;
> + r &= 0xffffffff;
> break;
> case RTC_TIME_HIGH:
> - r = goldfish_rtc_get_count(s) >> 32;
> + r = s->time_high;
> break;
> case RTC_ALARM_LOW:
> r = s->alarm_next & 0xffffffff;
> diff --git a/include/hw/rtc/goldfish_rtc.h b/include/hw/rtc/goldfish_rtc.h
> index 16f9f9e29d..9bd8924f5f 100644
> --- a/include/hw/rtc/goldfish_rtc.h
> +++ b/include/hw/rtc/goldfish_rtc.h
> @@ -41,6 +41,7 @@ typedef struct GoldfishRTCState {
> uint32_t alarm_running;
> uint32_t irq_pending;
> uint32_t irq_enabled;
> + uint32_t time_high;
> } GoldfishRTCState;
You need to add the new field to goldfish_rtc_vmstate, and increment the
version.
r~
