| Message ID | 8f2fa47d-89b7-b39c-e60f-edee1de5ca82@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v5] x86: detect CMOS aliasing on ports other than 0x70/0x71 | expand |
On Thu, Mar 30, 2023 at 12:40:38PM +0200, Jan Beulich wrote: > ... in order to also intercept Dom0 accesses through the alias ports. > > Also stop intercepting accesses to the CMOS ports if we won't ourselves > use the CMOS RTC, because of there being none. So it's fine for dom0 to switch off NMIs if Xen isn't using the RTC? Seems like a weird side-effect of Xen not using the RTC (seeing as we would otherwise mask bit 8 from dom0 RTC accesses). Also I'm worried that when Xen doesn't intercept RTC ports accesses from dom0 could be interrupted for example by the vCPU being scheduled out, so a vCPU might perform a write to the index port, and be scheduled out, leaving the RTC in an undefined state. I've read claims online that the RTC is not reset by the firmware, and since it has a battery the state is kept across reboots, so interrupting an access like that cold leave the RTC in a broken state across reboots. > Note that rtc_init() deliberately uses 16 as the upper loop bound, > despite probe_cmos_alias() using 8: The higher bound is benign now, but > would save us touching the code (or, worse, missing to touch it) in case > the lower one was doubled. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > v5: Simplify logic in is_cmos_port(). Limit the scope of a local > variable. Adjust a comment that's being moved. > v4: Also conditionally mask top bit for guest index port accesses. Add > missing adjustments to rtc_init(). Re-work to avoid recursive > read_lock(). Also adjust guest_io_{read,write}(). Re-base. > v3: Re-base over change to earlier patch. > v2: Re-base. > > --- a/xen/arch/x86/hvm/rtc.c > +++ b/xen/arch/x86/hvm/rtc.c > @@ -27,7 +27,7 @@ > #include <asm/hvm/vpt.h> > #include <asm/hvm/io.h> > #include <asm/hvm/save.h> > -#include <asm/current.h> > +#include <asm/iocap.h> > #include <xen/trace.h> > #include <public/hvm/params.h> > > @@ -836,10 +836,18 @@ void rtc_init(struct domain *d) > > if ( !has_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; > + unsigned int port; > + > + if ( !is_hardware_domain(d) ) > + return; > + > + /* > + * Hardware domain gets mediated access to the physical RTC/CMOS (of > + * course unless we don't use it ourselves, for there being none). > + */ > + for ( port = RTC_PORT(0); port < RTC_PORT(0) + 0x10; port += 2 ) > + if ( is_cmos_port(port, 2, d) ) > + register_portio_handler(d, port, 2, hw_rtc_io); You seem to have dropped a return from here, as for PVH dom0 the initialization below shouldn't be done. > } > > spin_lock_init(&s->lock); > --- a/xen/arch/x86/include/asm/mc146818rtc.h > +++ b/xen/arch/x86/include/asm/mc146818rtc.h > @@ -9,6 +9,10 @@ > > extern spinlock_t rtc_lock; /* serialize CMOS RAM access */ > > +struct domain; > +bool is_cmos_port(unsigned int port, unsigned int bytes, > + const struct domain *d); > + > /********************************************************************** > * register summary > **********************************************************************/ > --- a/xen/arch/x86/pv/emul-priv-op.c > +++ b/xen/arch/x86/pv/emul-priv-op.c > @@ -220,7 +220,7 @@ static bool admin_io_okay(unsigned int p > return false; > > /* We also never permit direct access to the RTC/CMOS registers. */ > - if ( port <= RTC_PORT(1) && port + bytes > RTC_PORT(0) ) > + if ( is_cmos_port(port, bytes, d) ) > return false; > > return ioports_access_permitted(d, port, port + bytes - 1); > @@ -290,7 +290,7 @@ static uint32_t guest_io_read(unsigned i > { > sub_data = pv_pit_handler(port, 0, 0); > } > - else if ( port == RTC_PORT(0) || port == RTC_PORT(1) ) > + else if ( is_cmos_port(port, 1, currd) ) > { > sub_data = rtc_guest_read(port); > } > @@ -436,7 +436,7 @@ static void guest_io_write(unsigned int > { > pv_pit_handler(port, (uint8_t)data, 1); > } > - else if ( port == RTC_PORT(0) || port == RTC_PORT(1) ) > + else if ( is_cmos_port(port, 1, currd) ) > { > rtc_guest_write(port, data); > } > --- a/xen/arch/x86/setup.c > +++ b/xen/arch/x86/setup.c > @@ -2131,37 +2131,36 @@ int __hwdom_init xen_in_range(unsigned l > static int __hwdom_init cf_check io_bitmap_cb( > unsigned long s, unsigned long e, void *ctx) > { > - struct domain *d = ctx; > + const struct domain *d = ctx; > unsigned int i; > > ASSERT(e <= INT_MAX); > for ( i = s; i <= e; i++ ) > - __clear_bit(i, d->arch.hvm.io_bitmap); > + /* > + * Accesses to RTC ports also need to be trapped in order to keep > + * consistency with hypervisor accesses. > + */ > + if ( !is_cmos_port(i, 1, d) ) > + __clear_bit(i, d->arch.hvm.io_bitmap); > > return 0; > } > > void __hwdom_init setup_io_bitmap(struct domain *d) > { > - int rc; > + if ( !is_hvm_domain(d) ) > + return; > > - if ( is_hvm_domain(d) ) > - { > - bitmap_fill(d->arch.hvm.io_bitmap, 0x10000); > - rc = rangeset_report_ranges(d->arch.ioport_caps, 0, 0x10000, > - io_bitmap_cb, d); > - BUG_ON(rc); > - /* > - * NB: we need to trap accesses to 0xcf8 in order to intercept > - * 4 byte accesses, that need to be handled by Xen in order to > - * keep consistency. > - * Access to 1 byte RTC ports also needs to be trapped in order > - * to keep consistency with PV. > - */ > - __set_bit(0xcf8, d->arch.hvm.io_bitmap); > - __set_bit(RTC_PORT(0), d->arch.hvm.io_bitmap); > - __set_bit(RTC_PORT(1), d->arch.hvm.io_bitmap); > - } > + bitmap_fill(d->arch.hvm.io_bitmap, 0x10000); > + if ( rangeset_report_ranges(d->arch.ioport_caps, 0, 0x10000, > + io_bitmap_cb, d) ) > + BUG(); > + > + /* > + * We need to trap 4-byte accesses to 0xcf8 (see admin_io_okay(), > + * guest_io_read(), and guest_io_write()). > + */ > + __set_bit(0xcf8, d->arch.hvm.io_bitmap); > } > > /* > --- a/xen/arch/x86/time.c > +++ b/xen/arch/x86/time.c > @@ -1234,7 +1234,10 @@ static unsigned long get_cmos_time(void) > if ( seconds < 60 ) > { > if ( rtc.sec != seconds ) > + { > cmos_rtc_probe = false; > + acpi_gbl_FADT.boot_flags &= ~ACPI_FADT_NO_CMOS_RTC; > + } > break; > } > > @@ -1249,6 +1252,77 @@ static unsigned long get_cmos_time(void) > return mktime(rtc.year, rtc.mon, rtc.day, rtc.hour, rtc.min, rtc.sec); > } > > +static unsigned int __ro_after_init cmos_alias_mask; > + > +static int __init cf_check probe_cmos_alias(void) > +{ > + unsigned int offs; > + > + if ( acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_CMOS_RTC ) > + return 0; > + > + for ( offs = 2; offs < 8; offs <<= 1 ) > + { > + unsigned int i; > + bool read = true; > + > + for ( i = RTC_REG_D + 1; i < 0x80; ++i ) > + { > + uint8_t normal, alt; > + unsigned long flags; > + > + if ( i == acpi_gbl_FADT.century ) > + continue; > + > + spin_lock_irqsave(&rtc_lock, flags); > + > + normal = CMOS_READ(i); > + if ( inb(RTC_PORT(offs)) != i ) > + read = false; > + > + alt = inb(RTC_PORT(offs + 1)); > + > + spin_unlock_irqrestore(&rtc_lock, flags); > + > + if ( normal != alt ) > + break; > + > + process_pending_softirqs(); > + } > + if ( i == 0x80 ) > + { > + cmos_alias_mask |= offs; > + printk(XENLOG_INFO "CMOS aliased at %02x, index %s\n", > + RTC_PORT(offs), read ? "r/w" : "w/o"); I would consider making this a DEBUG message, not sure it's that useful for a normal end user, and printing to the console can be slow. > + } > + } > + > + return 0; > +} > +__initcall(probe_cmos_alias); > + > +bool is_cmos_port(unsigned int port, unsigned int bytes, const struct domain *d) > +{ > + unsigned int offs; > + > + if ( !is_hardware_domain(d) || > + !(acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_CMOS_RTC) ) > + return port <= RTC_PORT(1) && port + bytes > RTC_PORT(0); > + > + if ( acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_CMOS_RTC ) > + return false; > + > + for ( offs = 2; offs <= cmos_alias_mask; offs <<= 1 ) > + { > + if ( !(offs & cmos_alias_mask) ) > + continue; > + if ( port <= RTC_PORT(offs | 1) && port + bytes > RTC_PORT(offs) ) > + return true; > + } Maybe I'm confused, but doesn't this loop start at RTC_PORT(2), and hence you need to check for the RTC_PORT(0,1) pair outside of the loop? > + > + return false; > +} > + > /* Helpers for guest accesses to the physical RTC. */ > unsigned int rtc_guest_read(unsigned int port) > { > @@ -1256,23 +1330,25 @@ unsigned int rtc_guest_read(unsigned int > unsigned long flags; > unsigned int data = ~0; > > - switch ( port ) > + switch ( port & ~cmos_alias_mask ) Given that the call is gated with is_cmos_port() it would be clearer to just use RTC_PORT(1) as the mask here IMO. Thanks, Roger.
On 03.04.2023 13:09, Roger Pau Monné wrote: > On Thu, Mar 30, 2023 at 12:40:38PM +0200, Jan Beulich wrote: >> ... in order to also intercept Dom0 accesses through the alias ports. >> >> Also stop intercepting accesses to the CMOS ports if we won't ourselves >> use the CMOS RTC, because of there being none. > > So it's fine for dom0 to switch off NMIs if Xen isn't using the RTC? > Seems like a weird side-effect of Xen not using the RTC (seeing as we > would otherwise mask bit 8 from dom0 RTC accesses). I haven't been able to find documentation on this single bit in the absence of RTC / CMOS. > Also I'm worried that when Xen doesn't intercept RTC ports accesses > from dom0 could be interrupted for example by the vCPU being scheduled > out, so a vCPU might perform a write to the index port, and be > scheduled out, leaving the RTC in an undefined state. I did specifically add "because of there being none" to the sentence to clarify in which case we avoid intercepting. > I've read claims online that the RTC is not reset by the firmware, and > since it has a battery the state is kept across reboots, so > interrupting an access like that cold leave the RTC in a broken state > across reboots. I can easily imagine such firmware exists. >> @@ -836,10 +836,18 @@ void rtc_init(struct domain *d) >> >> if ( !has_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; >> + unsigned int port; >> + >> + if ( !is_hardware_domain(d) ) >> + return; >> + >> + /* >> + * Hardware domain gets mediated access to the physical RTC/CMOS (of >> + * course unless we don't use it ourselves, for there being none). >> + */ >> + for ( port = RTC_PORT(0); port < RTC_PORT(0) + 0x10; port += 2 ) >> + if ( is_cmos_port(port, 2, d) ) >> + register_portio_handler(d, port, 2, hw_rtc_io); > > You seem to have dropped a return from here, as for PVH dom0 the > initialization below shouldn't be done. Oh, indeed, thanks for spotting. (The excess init is benign afaict, but I still shouldn't have dropped that "return".) >> @@ -1249,6 +1252,77 @@ static unsigned long get_cmos_time(void) >> return mktime(rtc.year, rtc.mon, rtc.day, rtc.hour, rtc.min, rtc.sec); >> } >> >> +static unsigned int __ro_after_init cmos_alias_mask; >> + >> +static int __init cf_check probe_cmos_alias(void) >> +{ >> + unsigned int offs; >> + >> + if ( acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_CMOS_RTC ) >> + return 0; >> + >> + for ( offs = 2; offs < 8; offs <<= 1 ) >> + { >> + unsigned int i; >> + bool read = true; >> + >> + for ( i = RTC_REG_D + 1; i < 0x80; ++i ) >> + { >> + uint8_t normal, alt; >> + unsigned long flags; >> + >> + if ( i == acpi_gbl_FADT.century ) >> + continue; >> + >> + spin_lock_irqsave(&rtc_lock, flags); >> + >> + normal = CMOS_READ(i); >> + if ( inb(RTC_PORT(offs)) != i ) >> + read = false; >> + >> + alt = inb(RTC_PORT(offs + 1)); >> + >> + spin_unlock_irqrestore(&rtc_lock, flags); >> + >> + if ( normal != alt ) >> + break; >> + >> + process_pending_softirqs(); >> + } >> + if ( i == 0x80 ) >> + { >> + cmos_alias_mask |= offs; >> + printk(XENLOG_INFO "CMOS aliased at %02x, index %s\n", >> + RTC_PORT(offs), read ? "r/w" : "w/o"); > > I would consider making this a DEBUG message, not sure it's that > useful for a normal end user, and printing to the console can be slow. Can do, sure. >> +bool is_cmos_port(unsigned int port, unsigned int bytes, const struct domain *d) >> +{ >> + unsigned int offs; >> + >> + if ( !is_hardware_domain(d) || >> + !(acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_CMOS_RTC) ) >> + return port <= RTC_PORT(1) && port + bytes > RTC_PORT(0); >> + >> + if ( acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_CMOS_RTC ) >> + return false; >> + >> + for ( offs = 2; offs <= cmos_alias_mask; offs <<= 1 ) >> + { >> + if ( !(offs & cmos_alias_mask) ) >> + continue; >> + if ( port <= RTC_PORT(offs | 1) && port + bytes > RTC_PORT(offs) ) >> + return true; >> + } > > Maybe I'm confused, but doesn't this loop start at RTC_PORT(2), and > hence you need to check for the RTC_PORT(0,1) pair outside of the > loop? The loop starts at offset 2, yes, but see the initial if() in the function. Or at least I thought I got that right, but it looks like I didn't (failed attempt to try to address a specific request of yours, iirc). >> @@ -1256,23 +1330,25 @@ unsigned int rtc_guest_read(unsigned int >> unsigned long flags; >> unsigned int data = ~0; >> >> - switch ( port ) >> + switch ( port & ~cmos_alias_mask ) > > Given that the call is gated with is_cmos_port() it would be clearer > to just use RTC_PORT(1) as the mask here IMO. Hmm, personally I wouldn't consider RTC_PORT(1) to be reasonable to use as a mask (even if technically it would be okay). Jan
On Mon, Apr 03, 2023 at 01:26:41PM +0200, Jan Beulich wrote: > On 03.04.2023 13:09, Roger Pau Monné wrote: > > On Thu, Mar 30, 2023 at 12:40:38PM +0200, Jan Beulich wrote: > >> ... in order to also intercept Dom0 accesses through the alias ports. > >> > >> Also stop intercepting accesses to the CMOS ports if we won't ourselves > >> use the CMOS RTC, because of there being none. > > > > So it's fine for dom0 to switch off NMIs if Xen isn't using the RTC? > > Seems like a weird side-effect of Xen not using the RTC (seeing as we > > would otherwise mask bit 8 from dom0 RTC accesses). > > I haven't been able to find documentation on this single bit in the > absence of RTC / CMOS. > > > Also I'm worried that when Xen doesn't intercept RTC ports accesses > > from dom0 could be interrupted for example by the vCPU being scheduled > > out, so a vCPU might perform a write to the index port, and be > > scheduled out, leaving the RTC in an undefined state. > > I did specifically add "because of there being none" to the sentence > to clarify in which case we avoid intercepting. Oh, right, sorry for the fuzz, I didn't parse that last bit of the sentence. I'm fine with the current wording then. > >> +bool is_cmos_port(unsigned int port, unsigned int bytes, const struct domain *d) > >> +{ > >> + unsigned int offs; > >> + > >> + if ( !is_hardware_domain(d) || > >> + !(acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_CMOS_RTC) ) > >> + return port <= RTC_PORT(1) && port + bytes > RTC_PORT(0); > >> + > >> + if ( acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_CMOS_RTC ) > >> + return false; > >> + > >> + for ( offs = 2; offs <= cmos_alias_mask; offs <<= 1 ) > >> + { > >> + if ( !(offs & cmos_alias_mask) ) > >> + continue; > >> + if ( port <= RTC_PORT(offs | 1) && port + bytes > RTC_PORT(offs) ) > >> + return true; > >> + } > > > > Maybe I'm confused, but doesn't this loop start at RTC_PORT(2), and > > hence you need to check for the RTC_PORT(0,1) pair outside of the > > loop? > > The loop starts at offset 2, yes, but see the initial if() in the > function. Or at least I thought I got that right, but it looks like > I didn't (failed attempt to try to address a specific request of > yours, iirc). Hm, doesn't that first if() cause that on all systems with an RTC only PORTS(0,1) are allowed? > >> @@ -1256,23 +1330,25 @@ unsigned int rtc_guest_read(unsigned int > >> unsigned long flags; > >> unsigned int data = ~0; > >> > >> - switch ( port ) > >> + switch ( port & ~cmos_alias_mask ) > > > > Given that the call is gated with is_cmos_port() it would be clearer > > to just use RTC_PORT(1) as the mask here IMO. > > Hmm, personally I wouldn't consider RTC_PORT(1) to be reasonable to > use as a mask (even if technically it would be okay). OK, never mind then. Thanks, Roger.
On 03.04.2023 13:44, Roger Pau Monné wrote: > On Mon, Apr 03, 2023 at 01:26:41PM +0200, Jan Beulich wrote: >> On 03.04.2023 13:09, Roger Pau Monné wrote: >>> On Thu, Mar 30, 2023 at 12:40:38PM +0200, Jan Beulich wrote: >>>> +bool is_cmos_port(unsigned int port, unsigned int bytes, const struct domain *d) >>>> +{ >>>> + unsigned int offs; >>>> + >>>> + if ( !is_hardware_domain(d) || >>>> + !(acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_CMOS_RTC) ) >>>> + return port <= RTC_PORT(1) && port + bytes > RTC_PORT(0); >>>> + >>>> + if ( acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_CMOS_RTC ) >>>> + return false; >>>> + >>>> + for ( offs = 2; offs <= cmos_alias_mask; offs <<= 1 ) >>>> + { >>>> + if ( !(offs & cmos_alias_mask) ) >>>> + continue; >>>> + if ( port <= RTC_PORT(offs | 1) && port + bytes > RTC_PORT(offs) ) >>>> + return true; >>>> + } >>> >>> Maybe I'm confused, but doesn't this loop start at RTC_PORT(2), and >>> hence you need to check for the RTC_PORT(0,1) pair outside of the >>> loop? >> >> The loop starts at offset 2, yes, but see the initial if() in the >> function. Or at least I thought I got that right, but it looks like >> I didn't (failed attempt to try to address a specific request of >> yours, iirc). > > Hm, doesn't that first if() cause that on all systems with an RTC only > PORTS(0,1) are allowed? Indeed, hence why I said "failed attempt". Looking at it now I really don't know what I was thinking when writing it that way. Jan
--- a/xen/arch/x86/hvm/rtc.c +++ b/xen/arch/x86/hvm/rtc.c @@ -27,7 +27,7 @@ #include <asm/hvm/vpt.h> #include <asm/hvm/io.h> #include <asm/hvm/save.h> -#include <asm/current.h> +#include <asm/iocap.h> #include <xen/trace.h> #include <public/hvm/params.h> @@ -836,10 +836,18 @@ void rtc_init(struct domain *d) if ( !has_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; + unsigned int port; + + if ( !is_hardware_domain(d) ) + return; + + /* + * Hardware domain gets mediated access to the physical RTC/CMOS (of + * course unless we don't use it ourselves, for there being none). + */ + for ( port = RTC_PORT(0); port < RTC_PORT(0) + 0x10; port += 2 ) + if ( is_cmos_port(port, 2, d) ) + register_portio_handler(d, port, 2, hw_rtc_io); } spin_lock_init(&s->lock); --- a/xen/arch/x86/include/asm/mc146818rtc.h +++ b/xen/arch/x86/include/asm/mc146818rtc.h @@ -9,6 +9,10 @@ extern spinlock_t rtc_lock; /* serialize CMOS RAM access */ +struct domain; +bool is_cmos_port(unsigned int port, unsigned int bytes, + const struct domain *d); + /********************************************************************** * register summary **********************************************************************/ --- a/xen/arch/x86/pv/emul-priv-op.c +++ b/xen/arch/x86/pv/emul-priv-op.c @@ -220,7 +220,7 @@ static bool admin_io_okay(unsigned int p return false; /* We also never permit direct access to the RTC/CMOS registers. */ - if ( port <= RTC_PORT(1) && port + bytes > RTC_PORT(0) ) + if ( is_cmos_port(port, bytes, d) ) return false; return ioports_access_permitted(d, port, port + bytes - 1); @@ -290,7 +290,7 @@ static uint32_t guest_io_read(unsigned i { sub_data = pv_pit_handler(port, 0, 0); } - else if ( port == RTC_PORT(0) || port == RTC_PORT(1) ) + else if ( is_cmos_port(port, 1, currd) ) { sub_data = rtc_guest_read(port); } @@ -436,7 +436,7 @@ static void guest_io_write(unsigned int { pv_pit_handler(port, (uint8_t)data, 1); } - else if ( port == RTC_PORT(0) || port == RTC_PORT(1) ) + else if ( is_cmos_port(port, 1, currd) ) { rtc_guest_write(port, data); } --- a/xen/arch/x86/setup.c +++ b/xen/arch/x86/setup.c @@ -2131,37 +2131,36 @@ int __hwdom_init xen_in_range(unsigned l static int __hwdom_init cf_check io_bitmap_cb( unsigned long s, unsigned long e, void *ctx) { - struct domain *d = ctx; + const struct domain *d = ctx; unsigned int i; ASSERT(e <= INT_MAX); for ( i = s; i <= e; i++ ) - __clear_bit(i, d->arch.hvm.io_bitmap); + /* + * Accesses to RTC ports also need to be trapped in order to keep + * consistency with hypervisor accesses. + */ + if ( !is_cmos_port(i, 1, d) ) + __clear_bit(i, d->arch.hvm.io_bitmap); return 0; } void __hwdom_init setup_io_bitmap(struct domain *d) { - int rc; + if ( !is_hvm_domain(d) ) + return; - if ( is_hvm_domain(d) ) - { - bitmap_fill(d->arch.hvm.io_bitmap, 0x10000); - rc = rangeset_report_ranges(d->arch.ioport_caps, 0, 0x10000, - io_bitmap_cb, d); - BUG_ON(rc); - /* - * NB: we need to trap accesses to 0xcf8 in order to intercept - * 4 byte accesses, that need to be handled by Xen in order to - * keep consistency. - * Access to 1 byte RTC ports also needs to be trapped in order - * to keep consistency with PV. - */ - __set_bit(0xcf8, d->arch.hvm.io_bitmap); - __set_bit(RTC_PORT(0), d->arch.hvm.io_bitmap); - __set_bit(RTC_PORT(1), d->arch.hvm.io_bitmap); - } + bitmap_fill(d->arch.hvm.io_bitmap, 0x10000); + if ( rangeset_report_ranges(d->arch.ioport_caps, 0, 0x10000, + io_bitmap_cb, d) ) + BUG(); + + /* + * We need to trap 4-byte accesses to 0xcf8 (see admin_io_okay(), + * guest_io_read(), and guest_io_write()). + */ + __set_bit(0xcf8, d->arch.hvm.io_bitmap); } /* --- a/xen/arch/x86/time.c +++ b/xen/arch/x86/time.c @@ -1234,7 +1234,10 @@ static unsigned long get_cmos_time(void) if ( seconds < 60 ) { if ( rtc.sec != seconds ) + { cmos_rtc_probe = false; + acpi_gbl_FADT.boot_flags &= ~ACPI_FADT_NO_CMOS_RTC; + } break; } @@ -1249,6 +1252,77 @@ static unsigned long get_cmos_time(void) return mktime(rtc.year, rtc.mon, rtc.day, rtc.hour, rtc.min, rtc.sec); } +static unsigned int __ro_after_init cmos_alias_mask; + +static int __init cf_check probe_cmos_alias(void) +{ + unsigned int offs; + + if ( acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_CMOS_RTC ) + return 0; + + for ( offs = 2; offs < 8; offs <<= 1 ) + { + unsigned int i; + bool read = true; + + for ( i = RTC_REG_D + 1; i < 0x80; ++i ) + { + uint8_t normal, alt; + unsigned long flags; + + if ( i == acpi_gbl_FADT.century ) + continue; + + spin_lock_irqsave(&rtc_lock, flags); + + normal = CMOS_READ(i); + if ( inb(RTC_PORT(offs)) != i ) + read = false; + + alt = inb(RTC_PORT(offs + 1)); + + spin_unlock_irqrestore(&rtc_lock, flags); + + if ( normal != alt ) + break; + + process_pending_softirqs(); + } + if ( i == 0x80 ) + { + cmos_alias_mask |= offs; + printk(XENLOG_INFO "CMOS aliased at %02x, index %s\n", + RTC_PORT(offs), read ? "r/w" : "w/o"); + } + } + + return 0; +} +__initcall(probe_cmos_alias); + +bool is_cmos_port(unsigned int port, unsigned int bytes, const struct domain *d) +{ + unsigned int offs; + + if ( !is_hardware_domain(d) || + !(acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_CMOS_RTC) ) + return port <= RTC_PORT(1) && port + bytes > RTC_PORT(0); + + if ( acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_CMOS_RTC ) + return false; + + for ( offs = 2; offs <= cmos_alias_mask; offs <<= 1 ) + { + if ( !(offs & cmos_alias_mask) ) + continue; + if ( port <= RTC_PORT(offs | 1) && port + bytes > RTC_PORT(offs) ) + return true; + } + + return false; +} + /* Helpers for guest accesses to the physical RTC. */ unsigned int rtc_guest_read(unsigned int port) { @@ -1256,23 +1330,25 @@ unsigned int rtc_guest_read(unsigned int unsigned long flags; unsigned int data = ~0; - switch ( port ) + switch ( port & ~cmos_alias_mask ) { case RTC_PORT(0): /* * All PV domains (and PVH dom0) are allowed to read the latched value * of the first RTC port, as there's no access to the physical IO - * ports. + * ports. Note that we return the index value regardless of whether + * underlying hardware would permit doing so. */ - data = currd->arch.cmos_idx; + data = currd->arch.cmos_idx & (0xff >> (port == RTC_PORT(0))); break; case RTC_PORT(1): - if ( !ioports_access_permitted(currd, RTC_PORT(0), RTC_PORT(1)) ) + if ( !ioports_access_permitted(currd, port - 1, port) ) break; spin_lock_irqsave(&rtc_lock, flags); - outb(currd->arch.cmos_idx & 0x7f, RTC_PORT(0)); - data = inb(RTC_PORT(1)); + outb(currd->arch.cmos_idx & (0xff >> (port == RTC_PORT(1))), + port - 1); + data = inb(port); spin_unlock_irqrestore(&rtc_lock, flags); break; @@ -1288,9 +1364,10 @@ void rtc_guest_write(unsigned int port, struct domain *currd = current->domain; unsigned long flags; - switch ( port ) + switch ( port & ~cmos_alias_mask ) { typeof(pv_rtc_handler) hook; + unsigned int idx; case RTC_PORT(0): /* @@ -1298,20 +1375,22 @@ void rtc_guest_write(unsigned int port, * value of the first RTC port, as there's no access to the physical IO * ports. */ - currd->arch.cmos_idx = data; + currd->arch.cmos_idx = data & (0xff >> (port == RTC_PORT(0))); break; case RTC_PORT(1): - if ( !ioports_access_permitted(currd, RTC_PORT(0), RTC_PORT(1)) ) + if ( !ioports_access_permitted(currd, port - 1, port) ) break; + idx = currd->arch.cmos_idx & (0xff >> (port == RTC_PORT(1))); + hook = ACCESS_ONCE(pv_rtc_handler); if ( hook ) - hook(currd->arch.cmos_idx & 0x7f, data); + hook(idx, data); spin_lock_irqsave(&rtc_lock, flags); - outb(currd->arch.cmos_idx & 0x7f, RTC_PORT(0)); - outb(data, RTC_PORT(1)); + outb(idx, port - 1); + outb(data, port); spin_unlock_irqrestore(&rtc_lock, flags); break;
... in order to also intercept Dom0 accesses through the alias ports. Also stop intercepting accesses to the CMOS ports if we won't ourselves use the CMOS RTC, because of there being none. Note that rtc_init() deliberately uses 16 as the upper loop bound, despite probe_cmos_alias() using 8: The higher bound is benign now, but would save us touching the code (or, worse, missing to touch it) in case the lower one was doubled. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- v5: Simplify logic in is_cmos_port(). Limit the scope of a local variable. Adjust a comment that's being moved. v4: Also conditionally mask top bit for guest index port accesses. Add missing adjustments to rtc_init(). Re-work to avoid recursive read_lock(). Also adjust guest_io_{read,write}(). Re-base. v3: Re-base over change to earlier patch. v2: Re-base.