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

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

On Wed, Jul 15, 2020 at 03:51:17PM +0200, Jan Beulich wrote:
> 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.

Thanks! Sorry I might be pedantic, but is the ACCESS_ONCE on the write
side actually required? I'm not sure I see what ACCESS_ONCE protects
against in handle_rtc_once.

Roger.