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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 15.07.2020 12:01, Paul Durrant wrote:
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@xxxxxxxx>
>> Sent: 15 July 2020 10:47
>> To: xen-devel@xxxxxxxxxxxxxxxxxxxx
>> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>; Paul Durrant <paul@xxxxxxx>; 
>> Wei Liu <wl@xxxxxxx>;
>> Roger Pau Monné <roger.pau@xxxxxxxxxx>
>> Subject: [PATCH v2 1/2] x86: restore pv_rtc_handler() invocation
>>
>> This was lost when making the logic accessible to PVH Dom0.
>>
>> Fixes: 835d8d69d96a ("x86/rtc: provide mediated access to RTC for PVH dom0")
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>>
>> --- a/xen/arch/x86/time.c
>> +++ b/xen/arch/x86/time.c
>> @@ -1160,6 +1160,10 @@ void rtc_guest_write(unsigned int port,
>>      case RTC_PORT(1):
>>          if ( !ioports_access_permitted(currd, RTC_PORT(0), RTC_PORT(1)) )
>>              break;
>> +
>> +        if ( pv_rtc_handler )
>> +            pv_rtc_handler(currd->arch.cmos_idx & 0x7f, data);
>> +
> 
> This appears to be semantically slightly different to the old code in that it 
> is only done for a write to RC_PORT(1), whereas it would have been done on a 
> write to either RTC_POR(0) or RTC_PORT(1) before. Is that of any concern?

The old code was (quoting plain 4.13.1)

        else if ( port == RTC_PORT(0) )
        {
            currd->arch.cmos_idx = data;
        }
        else if ( (port == RTC_PORT(1)) &&
                  ioports_access_permitted(currd, RTC_PORT(0), RTC_PORT(1)) )
        {
            unsigned long flags;

            if ( pv_rtc_handler )
                pv_rtc_handler(currd->arch.cmos_idx & 0x7f, data);
            spin_lock_irqsave(&rtc_lock, flags);
            outb(currd->arch.cmos_idx & 0x7f, RTC_PORT(0));
            outb(data, RTC_PORT(1));
            spin_unlock_irqrestore(&rtc_lock, flags);
        }

which I think similarly invoked the hook for RTC_PORT(1) only.

Jan