Re: [PATCH v3 1/2] x86: restore pv_rtc_handler() invocation

[Jan Beulich <jbeulich@xxxxxxxx>]

On 15.07.2020 14:13, Roger Pau Monné wrote:
> On Wed, Jul 15, 2020 at 01:56:47PM +0200, Jan Beulich wrote:
>> @@ -1160,6 +1162,14 @@ void rtc_guest_write(unsigned int port,
>>      case RTC_PORT(1):
>>          if ( !ioports_access_permitted(currd, RTC_PORT(0), RTC_PORT(1)) )
>>              break;
>> +
>> +        spin_lock_irqsave(&rtc_lock, flags);
>> +        hook = pv_rtc_handler;
>> +        spin_unlock_irqrestore(&rtc_lock, flags);
> 
> Given that clearing the pv_rtc_handler variable in handle_rtc_once is
> not done while holding the rtc_lock, I'm not sure there's much point
> in holding the lock here, ie: just doing something like:
> 
> hook = pv_rtc_handler;
> if ( hook )
>     hook(currd->arch.cmos_idx & 0x7f, data);
> 
> Should be as safe as what you do.

No, the compiler is free to eliminate the local variable and read
the global one twice (and it may change contents in between) then.
I could use ACCESS_ONCE() or read_atomic() here, but then it would
become quite clear that at the same time ...

> We also assume that setting pv_rtc_handler to NULL is an atomic
> operation.

... this (which isn't different from what we do elsewhere, and we
really can't fix everything at the same time) ought to also become
ACCESS_ONCE() (or write_atomic()).

A compromise might be to use barrier() in place of the locking for
now ...

Jan