| Message ID | 20200605075006.51238-1-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [for-4.14] x86/rtc: provide mediated access to RTC for PVH dom0 | expand |
> -----Original Message----- > From: Roger Pau Monne <roger.pau@citrix.com> > Sent: 05 June 2020 08:50 > To: xen-devel@lists.xenproject.org > Cc: paul@xen.org; Roger Pau Monne <roger.pau@citrix.com>; Jan Beulich <jbeulich@suse.com>; Andrew > Cooper <andrew.cooper3@citrix.com>; Wei Liu <wl@xen.org> > Subject: [PATCH for-4.14] x86/rtc: provide mediated access to RTC for PVH dom0 > > 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. 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. > > 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. > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > --- > for-4.14 reasoning: the fix is completely isolated to PVH dom0, and as > such the risk is very low of causing issues to other guests types, but > without this fix one vCPU when using a Linux dom0 will be constantly > looping over rtc_timer_do_work with 100% CPU usage, at least when > using Linux 4.19 or newer. > --- I can't say I'm a big fan of the code duplication with emul-priv-op.c, but it's not much code and it does keep this patch small. If the maintainers are happy to ack then consider this... Release-acked-by: Paul Durrant <paul@xen.org> > xen/arch/x86/hvm/rtc.c | 69 ++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 69 insertions(+) > > diff --git a/xen/arch/x86/hvm/rtc.c b/xen/arch/x86/hvm/rtc.c > index 5bbbdc0e0f..5d637cf018 100644 > --- 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); > + break; > + } > + > + return data; > +} > + > +static void hw_write(unsigned int port, unsigned int data) > +{ > + struct domain *currd = current->domain; > + unsigned long flags; > + > + switch ( port ) > + { > + case RTC_PORT(0): > + currd->arch.cmos_idx = data; > + break; > + > + case RTC_PORT(1): > + 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); > + break; > + } > +} > + > +static int hw_rtc_io(int dir, unsigned int port, unsigned int size, > + uint32_t *val) > +{ > + if ( size != 1 ) > + { > + gdprintk(XENLOG_WARNING, "bad RTC access size (%u)\n", size); > + *val = ~0; > + return X86EMUL_OKAY; > + } > + > + if ( dir == IOREQ_WRITE ) > + hw_write(port, *val); > + else > + *val = hw_read(port); > + > + return X86EMUL_OKAY; > +} > + > void rtc_init(struct domain *d) > { > RTCState *s = domain_vrtc(d); > > + if ( is_hardware_domain(d) ) > + { > + /* Hardware domain gets mediated access to the physical RTC. */ > + register_portio_handler(d, RTC_PORT(0), 2, hw_rtc_io); > + return; > + } > + > if ( !has_vrtc(d) ) > return; > > -- > 2.26.2
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
On Fri, Jun 05, 2020 at 09:03:11AM +0100, Paul Durrant wrote: > > -----Original Message----- > > From: Roger Pau Monne <roger.pau@citrix.com> > > Sent: 05 June 2020 08:50 > > To: xen-devel@lists.xenproject.org > > Cc: paul@xen.org; Roger Pau Monne <roger.pau@citrix.com>; Jan Beulich <jbeulich@suse.com>; Andrew > > Cooper <andrew.cooper3@citrix.com>; Wei Liu <wl@xen.org> > > Subject: [PATCH for-4.14] x86/rtc: provide mediated access to RTC for PVH dom0 > > > > 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. 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. > > > > 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. > > > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > > --- > > for-4.14 reasoning: the fix is completely isolated to PVH dom0, and as > > such the risk is very low of causing issues to other guests types, but > > without this fix one vCPU when using a Linux dom0 will be constantly > > looping over rtc_timer_do_work with 100% CPU usage, at least when > > using Linux 4.19 or newer. > > --- > > I can't say I'm a big fan of the code duplication with emul-priv-op.c, but it's not much code and it does keep this patch small. If the maintainers are happy to ack then consider this... Calling guest_io_{write/read} straight away is no acceptable IMO (and it would have to be moved out of emul-priv-op.c), and splitting the RTC accessors into separate helpers seemed like a lot of churn for the actual code that such helpers would contain. > Release-acked-by: Paul Durrant <paul@xen.org> Thanks.
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.
On 05.06.2020 11:20, Roger Pau Monné wrote: > 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. Looks like it was never hooked up for PVHv2, and removed together with a lot of other PVHv1 code by your 33e5c32559e1. >>> 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 the reason PV Dom0 has it applies to PVH Dom0 as well: At least back at the time there were video BIOSes needing this. As it now turns out to have been a mistake to not enable the RTC handling for v2, I would still think it would be better to enable the PIT logic as well there, just to be on the safe side. > 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)? Ah, likely. >>> --- 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. FAOD: I didn't mean to suggest I believe this transition would be working right now. But if someone wanted to make it work, they shouldn't be put at risk to unknowingly leave the original dom0 with more permissions than it's supposed to have. Jan
On Fri, Jun 05, 2020 at 12:05:12PM +0200, Jan Beulich wrote: > On 05.06.2020 11:20, Roger Pau Monné wrote: > > On Fri, Jun 05, 2020 at 10:48:48AM +0200, Jan Beulich wrote: > >> On 05.06.2020 09:50, Roger Pau Monne wrote: > >>> 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 the reason PV Dom0 has it applies to PVH Dom0 as well: > At least back at the time there were video BIOSes needing this. > As it now turns out to have been a mistake to not enable the > RTC handling for v2, I would still think it would be better to > enable the PIT logic as well there, just to be on the safe side. I have to admit I haven't used video output very much with PVH, I've had reports of people having success with it, but I have no idea how many failures there might be. Enabling the PIT for PVH dom0 is mostly a matter of adding XEN_X86_EMU_PIT to the emulation flags, like it's currently done for PV dom0. There's going to be a slight issue though, which is that the PIT will try to inject the interrupts using the PIC IRQ0, and thus would either need to also enable the PIC, or to instead set the timer source to PTSRC_ioapic instead of PTSRC_isa and use GSI 0. I haven't actually tried whether this would work, but seems better than enabling the PIC. Roger.
On 05.06.2020 16:16, Roger Pau Monné wrote: > On Fri, Jun 05, 2020 at 12:05:12PM +0200, Jan Beulich wrote: >> On 05.06.2020 11:20, Roger Pau Monné wrote: >>> On Fri, Jun 05, 2020 at 10:48:48AM +0200, Jan Beulich wrote: >>>> On 05.06.2020 09:50, Roger Pau Monne wrote: >>>>> 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 the reason PV Dom0 has it applies to PVH Dom0 as well: >> At least back at the time there were video BIOSes needing this. >> As it now turns out to have been a mistake to not enable the >> RTC handling for v2, I would still think it would be better to >> enable the PIT logic as well there, just to be on the safe side. > > I have to admit I haven't used video output very much with PVH, I've > had reports of people having success with it, but I have no idea how > many failures there might be. > > Enabling the PIT for PVH dom0 is mostly a matter of adding > XEN_X86_EMU_PIT to the emulation flags, like it's currently done for > PV dom0. > > There's going to be a slight issue though, which is that the PIT will > try to inject the interrupts using the PIC IRQ0, and thus would either > need to also enable the PIC, or to instead set the timer source to > PTSRC_ioapic instead of PTSRC_isa and use GSI 0. I haven't actually > tried whether this would work, but seems better than enabling the PIC. But what we do for PV Dom0 doesn't go as far as injecting IRQs (let alone IRQ0). It's just the few port accesses that we allow them to make (successfully, i.e. seeing the relevant bare hardware bits). Jan
On Fri, Jun 05, 2020 at 04:20:31PM +0200, Jan Beulich wrote: > On 05.06.2020 16:16, Roger Pau Monné wrote: > > On Fri, Jun 05, 2020 at 12:05:12PM +0200, Jan Beulich wrote: > >> On 05.06.2020 11:20, Roger Pau Monné wrote: > >>> On Fri, Jun 05, 2020 at 10:48:48AM +0200, Jan Beulich wrote: > >>>> On 05.06.2020 09:50, Roger Pau Monne wrote: > >>>>> 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 the reason PV Dom0 has it applies to PVH Dom0 as well: > >> At least back at the time there were video BIOSes needing this. > >> As it now turns out to have been a mistake to not enable the > >> RTC handling for v2, I would still think it would be better to > >> enable the PIT logic as well there, just to be on the safe side. > > > > I have to admit I haven't used video output very much with PVH, I've > > had reports of people having success with it, but I have no idea how > > many failures there might be. > > > > Enabling the PIT for PVH dom0 is mostly a matter of adding > > XEN_X86_EMU_PIT to the emulation flags, like it's currently done for > > PV dom0. > > > > There's going to be a slight issue though, which is that the PIT will > > try to inject the interrupts using the PIC IRQ0, and thus would either > > need to also enable the PIC, or to instead set the timer source to > > PTSRC_ioapic instead of PTSRC_isa and use GSI 0. I haven't actually > > tried whether this would work, but seems better than enabling the PIC. > > But what we do for PV Dom0 doesn't go as far as injecting IRQs (let > alone IRQ0). It's just the few port accesses that we allow them to > make (successfully, i.e. seeing the relevant bare hardware bits). It seems cleaner to me to just provide the whole thing for PVH, rather than what we do for classic PV, in which we allow some accesses to the real hardware. I understand there's no need to give dom0 access to the real PIC, and that using the emulated one should work just fine. Roger.
On Fri, Jun 05, 2020 at 04:45:28PM +0200, Roger Pau Monné wrote: > On Fri, Jun 05, 2020 at 04:20:31PM +0200, Jan Beulich wrote: > > On 05.06.2020 16:16, Roger Pau Monné wrote: > > > On Fri, Jun 05, 2020 at 12:05:12PM +0200, Jan Beulich wrote: > > >> On 05.06.2020 11:20, Roger Pau Monné wrote: > > >>> On Fri, Jun 05, 2020 at 10:48:48AM +0200, Jan Beulich wrote: > > >>>> On 05.06.2020 09:50, Roger Pau Monne wrote: > > >>>>> 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 the reason PV Dom0 has it applies to PVH Dom0 as well: > > >> At least back at the time there were video BIOSes needing this. > > >> As it now turns out to have been a mistake to not enable the > > >> RTC handling for v2, I would still think it would be better to > > >> enable the PIT logic as well there, just to be on the safe side. > > > > > > I have to admit I haven't used video output very much with PVH, I've > > > had reports of people having success with it, but I have no idea how > > > many failures there might be. > > > > > > Enabling the PIT for PVH dom0 is mostly a matter of adding > > > XEN_X86_EMU_PIT to the emulation flags, like it's currently done for > > > PV dom0. > > > > > > There's going to be a slight issue though, which is that the PIT will > > > try to inject the interrupts using the PIC IRQ0, and thus would either > > > need to also enable the PIC, or to instead set the timer source to > > > PTSRC_ioapic instead of PTSRC_isa and use GSI 0. I haven't actually > > > tried whether this would work, but seems better than enabling the PIC. > > > > But what we do for PV Dom0 doesn't go as far as injecting IRQs (let > > alone IRQ0). It's just the few port accesses that we allow them to > > make (successfully, i.e. seeing the relevant bare hardware bits). > > It seems cleaner to me to just provide the whole thing for PVH, rather > than what we do for classic PV, in which we allow some accesses to the > real hardware. > > I understand there's no need to give dom0 access to the real PIC, and ^ PIT. > that using the emulated one should work just fine. > > Roger. >
diff --git a/xen/arch/x86/hvm/rtc.c b/xen/arch/x86/hvm/rtc.c index 5bbbdc0e0f..5d637cf018 100644 --- 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); + break; + } + + return data; +} + +static void hw_write(unsigned int port, unsigned int data) +{ + struct domain *currd = current->domain; + unsigned long flags; + + switch ( port ) + { + case RTC_PORT(0): + currd->arch.cmos_idx = data; + break; + + case RTC_PORT(1): + 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); + break; + } +} + +static int hw_rtc_io(int dir, unsigned int port, unsigned int size, + uint32_t *val) +{ + if ( size != 1 ) + { + gdprintk(XENLOG_WARNING, "bad RTC access size (%u)\n", size); + *val = ~0; + return X86EMUL_OKAY; + } + + if ( dir == IOREQ_WRITE ) + hw_write(port, *val); + else + *val = hw_read(port); + + return X86EMUL_OKAY; +} + void rtc_init(struct domain *d) { RTCState *s = domain_vrtc(d); + if ( is_hardware_domain(d) ) + { + /* Hardware domain gets mediated access to the physical RTC. */ + register_portio_handler(d, RTC_PORT(0), 2, hw_rtc_io); + return; + } + if ( !has_vrtc(d) ) return;
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. 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. 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. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- for-4.14 reasoning: the fix is completely isolated to PVH dom0, and as such the risk is very low of causing issues to other guests types, but without this fix one vCPU when using a Linux dom0 will be constantly looping over rtc_timer_do_work with 100% CPU usage, at least when using Linux 4.19 or newer. --- xen/arch/x86/hvm/rtc.c | 69 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 69 insertions(+)