Re: [PATCH for-4.14 v3] x86/rtc: provide mediated access to RTC for PVH dom0

[Jan Beulich <jbeulich@xxxxxxxx>]

On 08.06.2020 12:29, Roger Pau Monne wrote:
> Mediated access to the RTC was provided for PVHv1 dom0 using the PV
> code paths (guest_io_{write/read}), but those accesses where never
> implemented for PVHv2 dom0. This patch provides such mediated accesses
> to the RTC for PVH dom0, just like it's provided for a classic PV
> dom0.
> 
> Pull out some of the RTC logic from guest_io_{read/write} into
> specific helpers that can be used by both PV and HVM guests. The
> setup of the handlers for PVH is done in rtc_init, which is already
> used to initialize the fully emulated RTC.
> 
> Without this a Linux PVH dom0 will read garbage when trying to access
> the RTC, and one vCPU will be constantly looping in
> rtc_timer_do_work.
> 
> Note that such issue doesn't happen on domUs because the ACPI
> NO_CMOS_RTC flag is set in FADT, which prevents the OS from accessing
> the RTC. Also the X86_EMU_RTC flag is not set for PVH dom0, as the
> accesses are not emulated but rather forwarded to the physical
> hardware.
> 
> No functional change expected for classic PV dom0.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
preferably with ...

> @@ -1110,6 +1111,67 @@ static unsigned long get_cmos_time(void)
>      return mktime(rtc.year, rtc.mon, rtc.day, rtc.hour, rtc.min, rtc.sec);
>  }
>  
> +/* Helpers for guest accesses to the physical RTC. */
> +unsigned int rtc_guest_read(unsigned int port)
> +{
> +    const struct domain *currd = current->domain;
> +    unsigned long flags;
> +    unsigned int data = ~0;
> +
> +    switch ( port )
> +    {
> +    case RTC_PORT(0):
> +        /*
> +         * All PV domains are allowed to read the latched value of the first
> +         * RTC port. This is useful in order to store data when debugging.
> +         */

... at least the 2nd sentence of this and ...

> +void rtc_guest_write(unsigned int port, unsigned int data)
> +{
> +    struct domain *currd = current->domain;
> +    unsigned long flags;
> +
> +    switch ( port )
> +    {
> +    case RTC_PORT(0):
> +        /*
> +         * All PV domains are allowed to write to the latched value of the
> +         * first RTC port. This is useful in order to store data when
> +         * debugging.
> +         */

... this comment dropped again. This justification of the possible
usefulness is my very private guessing. Just like the original code
was, I think we could leave this uncommented altogether.

Jan