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