On 18 Jul 2020, at 08:42, Philippe Mathieu-Daudé <f4...@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 <jrt...@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
