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

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

On Fri, Jun 05, 2020 at 10:48:48AM +0200, Jan Beulich wrote:
> 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.

I've attempted to, but failed to find the exact commit. I guess it
might have happened at ecaba067e1e433 when guest_io_{read/write} was
moved into emul-priv-op.c and made PV specific, but that's just a hint.

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

IIRC pv_pit_handler was also used by PVHv1 dom0, but we decided to not
enable it for PVHv2 because no one really knew why the PIT was
actually needed for by dom0.

I think it's in emul-i8524.c (outside of pv/) because it calls into
static functions on that file that are also used by HVM
(handle_pit_io)?

> 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}().

OK, I can try but it will involve more code churn.

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

Seems like it's acknowledged now:

https://elixir.bootlin.com/linux/latest/source/arch/x86/kernel/acpi/boot.c#L969

PVHv2 support is fairly recent anyway, and I wouldn't recommend using
anything below Linux 4.19, which also has this implemented.

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

TBH I'm not sure late-hwdom switching from an initial PVH domain will
work work very well, as we would already have to modify the vmcs/vmcb
io_bitmap in order to convey the changes to the allowed IO port ranges
which we don't do now.

Let me see if I can split the helpers.

Roger.