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

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

On Wed, Jul 15, 2020 at 01:56:47PM +0200, Jan Beulich wrote:
> This was lost when making the logic accessible to PVH Dom0.
> 
> While doing so make the access to the global function pointer safe
> against races (as noticed by Roger): The only current user wants to be
> invoked just once (but can tolerate to be invoked multiple times),
> zapping the pointer at that point.
> 
> Fixes: 835d8d69d96a ("x86/rtc: provide mediated access to RTC for PVH dom0")
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

Thanks, sorry I have one comment below.

> ---
> v3: Latch pointer under lock.
> v2: New.
> 
> --- a/xen/arch/x86/time.c
> +++ b/xen/arch/x86/time.c
> @@ -1148,6 +1148,8 @@ void rtc_guest_write(unsigned int port,
>  
>      switch ( port )
>      {
> +        typeof(pv_rtc_handler) hook;

Nit: FWIW, given the current structure of the function I would just have placed
it together with the rest of the top-level local variables.

> +
>      case RTC_PORT(0):
>          /*
>           * All PV domains (and PVH dom0) are allowed to write to the latched
> @@ -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.

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

Roger.