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

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

[Jessica Clarke]

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 <jrtc27@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;
 
 #endif
-- 
2.20.1

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

[Jessica Clarke]

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 <jrtc27@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;
 
 #endif
-- 
2.20.1

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

[Richard Henderson]

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 <jrtc27@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~

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

[Richard Henderson]

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 <jrtc27@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~

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

[Jessica Clarke]

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 <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;

 #endif
--
2.20.1

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

[Jessica Clarke]

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 <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;

 #endif
--
2.20.1

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;

---

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;

---

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

[Jessica Clarke]

On 18 Jul 2020, at 08:42, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> 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...

It actually makes a lot of sense. You know software always needs both
halves, and needs them to be atomic, so this is an easy way to provide
atomicity across two seemingly-independent reads.

>> 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):

We could, but why waste space storing an extra 32 bits you never need?
I don't think saving all 64 bits makes it any easier to read, I'd
personally even argue it makes it slightly less obvious what's going on.

Jess

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

[Jessica Clarke]

On 18 Jul 2020, at 08:42, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> 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...

It actually makes a lot of sense. You know software always needs both
halves, and needs them to be atomic, so this is an easy way to provide
atomicity across two seemingly-independent reads.

>> 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):

We could, but why waste space storing an extra 32 bits you never need?
I don't think saving all 64 bits makes it any easier to read, I'd
personally even argue it makes it slightly less obvious what's going on.

Jess

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

[Peter Maydell]

On Sat, 18 Jul 2020 at 15:45, Jessica Clarke <jrtc27@jrtc27.com> wrote:
> On 18 Jul 2020, at 08:42, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> > Maybe easier to cache the whole u64, this matches RTC_ALARM_LOW /
> > RTC_ALARM_HIGH pattern (goldfish_rtc_vmstate change not included):
>
> We could, but why waste space storing an extra 32 bits you never need?
> I don't think saving all 64 bits makes it any easier to read, I'd
> personally even argue it makes it slightly less obvious what's going on.

You could go either way. (The original Google-written version
of this device model went for store-the-whole-u64:
https://android.googlesource.com/platform/external/qemu/+/refs/heads/emu-2.0-release/hw/android/goldfish/timer.c
but we don't need to follow their implementation.)
Since "save the high half" is the version you've written
and tested, I vote we go with that :-)

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

NB: this is a migration compatibility break for the risc-v
'virt' board : up to Alistair whether that's OK or if the
more awkward compat-maintaining vmstate subsection is worth
the effort.

thanks
-- PMM

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

[Peter Maydell]

On Sat, 18 Jul 2020 at 15:45, Jessica Clarke <jrtc27@jrtc27.com> wrote:
> On 18 Jul 2020, at 08:42, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> > Maybe easier to cache the whole u64, this matches RTC_ALARM_LOW /
> > RTC_ALARM_HIGH pattern (goldfish_rtc_vmstate change not included):
>
> We could, but why waste space storing an extra 32 bits you never need?
> I don't think saving all 64 bits makes it any easier to read, I'd
> personally even argue it makes it slightly less obvious what's going on.

You could go either way. (The original Google-written version
of this device model went for store-the-whole-u64:
https://android.googlesource.com/platform/external/qemu/+/refs/heads/emu-2.0-release/hw/android/goldfish/timer.c
but we don't need to follow their implementation.)
Since "save the high half" is the version you've written
and tested, I vote we go with that :-)

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

NB: this is a migration compatibility break for the risc-v
'virt' board : up to Alistair whether that's OK or if the
more awkward compat-maintaining vmstate subsection is worth
the effort.

thanks
-- PMM

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

[Richard Henderson]

On 7/17/20 5:49 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 <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(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

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

[Richard Henderson]

On 7/17/20 5:49 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 <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(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

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

[Alistair Francis]

On Sat, Jul 18, 2020 at 11:09 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Sat, 18 Jul 2020 at 15:45, Jessica Clarke <jrtc27@jrtc27.com> wrote:
> > On 18 Jul 2020, at 08:42, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> > > Maybe easier to cache the whole u64, this matches RTC_ALARM_LOW /
> > > RTC_ALARM_HIGH pattern (goldfish_rtc_vmstate change not included):
> >
> > We could, but why waste space storing an extra 32 bits you never need?
> > I don't think saving all 64 bits makes it any easier to read, I'd
> > personally even argue it makes it slightly less obvious what's going on.
>
> You could go either way. (The original Google-written version
> of this device model went for store-the-whole-u64:
> https://android.googlesource.com/platform/external/qemu/+/refs/heads/emu-2.0-release/hw/android/goldfish/timer.c
> but we don't need to follow their implementation.)
> Since "save the high half" is the version you've written
> and tested, I vote we go with that :-)
>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

Applied to the RISC-V tree, this looks like a bug worth fixing for 5.1
so I'll send a PR soon.

>
> NB: this is a migration compatibility break for the risc-v
> 'virt' board : up to Alistair whether that's OK or if the
> more awkward compat-maintaining vmstate subsection is worth
> the effort.

I think it's ok

Alistair

>
> thanks
> -- PMM
>

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

[Alistair Francis]

On Sat, Jul 18, 2020 at 11:09 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Sat, 18 Jul 2020 at 15:45, Jessica Clarke <jrtc27@jrtc27.com> wrote:
> > On 18 Jul 2020, at 08:42, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> > > Maybe easier to cache the whole u64, this matches RTC_ALARM_LOW /
> > > RTC_ALARM_HIGH pattern (goldfish_rtc_vmstate change not included):
> >
> > We could, but why waste space storing an extra 32 bits you never need?
> > I don't think saving all 64 bits makes it any easier to read, I'd
> > personally even argue it makes it slightly less obvious what's going on.
>
> You could go either way. (The original Google-written version
> of this device model went for store-the-whole-u64:
> https://android.googlesource.com/platform/external/qemu/+/refs/heads/emu-2.0-release/hw/android/goldfish/timer.c
> but we don't need to follow their implementation.)
> Since "save the high half" is the version you've written
> and tested, I vote we go with that :-)
>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

Applied to the RISC-V tree, this looks like a bug worth fixing for 5.1
so I'll send a PR soon.

>
> NB: this is a migration compatibility break for the risc-v
> 'virt' board : up to Alistair whether that's OK or if the
> more awkward compat-maintaining vmstate subsection is worth
> the effort.

I think it's ok

Alistair

>
> thanks
> -- PMM
>