Re: [PATCH v2] goldfish_rtc: Fix non-atomic read behaviour of TIME_LOW/TIME_HIGH

[Philippe Mathieu-Daudé]

On 7/18/20 2:49 AM, 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.

What a odd design choice...

> 
> 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 <jrtc27@jrtc27.com>
> ---
> Changes since v1:
> 
>  * Add time_high to goldfish_rtc_vmstate and increment version.
> 
>  hw/rtc/goldfish_rtc.c         | 17 ++++++++++++++---
>  include/hw/rtc/goldfish_rtc.h |  1 +
>  2 files changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/rtc/goldfish_rtc.c b/hw/rtc/goldfish_rtc.c
> index 01e9d2b083..6ddd45cce0 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;
> @@ -216,7 +226,7 @@ static const MemoryRegionOps goldfish_rtc_ops = {
> 
>  static const VMStateDescription goldfish_rtc_vmstate = {
>      .name = TYPE_GOLDFISH_RTC,
> -    .version_id = 1,
> +    .version_id = 2,
>      .pre_save = goldfish_rtc_pre_save,
>      .post_load = goldfish_rtc_post_load,
>      .fields = (VMStateField[]) {
> @@ -225,6 +235,7 @@ static const VMStateDescription goldfish_rtc_vmstate = {
>          VMSTATE_UINT32(alarm_running, GoldfishRTCState),
>          VMSTATE_UINT32(irq_pending, GoldfishRTCState),
>          VMSTATE_UINT32(irq_enabled, GoldfishRTCState),
> +        VMSTATE_UINT32(time_high, GoldfishRTCState),
>          VMSTATE_END_OF_LIST()
>      }
>  };
> 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;

Maybe easier to cache the whole u64, this matches RTC_ALARM_LOW /
RTC_ALARM_HIGH pattern (goldfish_rtc_vmstate change not included):

-- >8 --
--- a/include/hw/rtc/goldfish_rtc.h
+++ b/include/hw/rtc/goldfish_rtc.h
@@ -37,6 +37,7 @@ typedef struct GoldfishRTCState {

     uint64_t tick_offset;
     uint64_t tick_offset_vmstate;
+    uint64_t rtc_time; /* Updated when RTC_TIME_LOW is read */
     uint64_t alarm_next;
     uint32_t alarm_running;
     uint32_t irq_pending;

--- a/hw/rtc/goldfish_rtc.c
+++ b/hw/rtc/goldfish_rtc.c
@@ -96,10 +96,11 @@ static uint64_t goldfish_rtc_read(void *opaque,
hwaddr offset,

     switch (offset) {
     case RTC_TIME_LOW:
-        r = goldfish_rtc_get_count(s) & 0xffffffff;
+        s->rtc_time = goldfish_rtc_get_count(s);
+        r = s->rtc_time & 0xffffffff;
         break;
     case RTC_TIME_HIGH:
-        r = goldfish_rtc_get_count(s) >> 32;
+        r = s->rtc_time >> 32;
         break;
     case RTC_ALARM_LOW:
         r = s->alarm_next & 0xffffffff;

---