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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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>

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

Thanks.