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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 05.06.2020 09:50, Roger Pau Monne wrote:
> At some point (maybe PVHv1?) mediated access to the RTC was provided
> for PVH dom0 using the PV code paths (guest_io_{write/read}). At some
> point this code has been made PV specific and unhooked from the
> current PVH IO path.

I don't suppose you've identified the commit which did? This would
help deciding whether / how far to backport such a change.

> This patch provides such mediated access to the
> RTC for PVH dom0, just like it's provided for a classic PV dom0.
> 
> Instead of re-using the PV paths implement such handler together with
> the vRTC code for HVM, so that calling rtc_init will setup the
> appropriate handlers for all HVM based guests.

Hooking this into rtc_init() makes sense as long as it's really
just the RTC. Looking at the PV code you're cloning from, I
wonder though whether pv_pit_handler() should also regain callers
for PVH. As it stands the function is called for PV only, yet was
deliberately put/kept outside of pv/.

So along the lines of Paul's reply I think it would be better if
code was shared between PV and PVH Dom0, perhaps by breaking out
respective pieces from guest_io_{read,write}().

> 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.

Will it? I'm pretty sure Linux may (have?) ignore(d) this flag.

> --- a/xen/arch/x86/hvm/rtc.c
> +++ b/xen/arch/x86/hvm/rtc.c
> @@ -808,10 +808,79 @@ void rtc_reset(struct domain *d)
>      s->pt.source = PTSRC_isa;
>  }
>  
> +/* RTC mediator for HVM hardware domain. */
> +static unsigned int hw_read(unsigned int port)
> +{
> +    const struct domain *currd = current->domain;
> +    unsigned long flags;
> +    unsigned int data = 0;
> +
> +    switch ( port )
> +    {
> +    case RTC_PORT(0):
> +          data = currd->arch.cmos_idx;
> +          break;
> +
> +    case RTC_PORT(1):
> +          spin_lock_irqsave(&rtc_lock, flags);
> +          outb(currd->arch.cmos_idx & 0x7f, RTC_PORT(0));
> +          data = inb(RTC_PORT(1));
> +          spin_unlock_irqrestore(&rtc_lock, flags);

Avoiding to clone the original code would also avoid omissions
like the ioports_access_permitted() calls you've dropped from
here as well as the write counterpart. This may seem redundant
as we never deny access right now, but should imo be there in
case we decide to do (e.g. on NO_CMOS_RTC systems).

Actually, "never" wasn't quite right I think: Late-hwdom
creation will revoke access from dom0 and instead grant it to
the new hwdom. Without the check, dom0 would continue to be
able to access the RTC.

Jan