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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 15.07.2020 15:32, Roger Pau Monné wrote:
> On Wed, Jul 15, 2020 at 02:36:49PM +0200, Jan Beulich wrote:
>> 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 ...
> 
> Oh, right. Didn't realize you did it in order to prevent
> optimizations. Using the lock seems also quite weird IMO, so I'm not
> sure it's much better than just using ACCESS_ONCE (or a barrier).
> Anyway, I don't want to delay this any longer, so:
> 
> Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Thanks.

> Feel free to change to ACCESS_ONCE or barrier if you think it's
> clearer.

I did so (also on the writer side), not the least based on guessing
what Andrew would presumably have preferred.

Jan