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....@citrix.com>
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
