* [PATCH v2 0/2] x86: RTC handling adjustments @ 2020-07-15 9:44 Jan Beulich 2020-07-15 9:47 ` [PATCH v2 1/2] x86: restore pv_rtc_handler() invocation Jan Beulich 2020-07-15 9:47 ` [PATCH v2 2/2] x86: detect CMOS aliasing on ports other than 0x70/0x71 Jan Beulich 0 siblings, 2 replies; 7+ messages in thread From: Jan Beulich @ 2020-07-15 9:44 UTC (permalink / raw) To: xen-devel; +Cc: Andrew Cooper, Paul Durrant, Wei Liu, Roger Pau Monné The first patch addresses a recent regression and hence ought to be considered for 4.14, despite it getting late. I noticed the problem while re-basing the 2nd patch here, which I decided to now re-post despite the original discussion on v1 not having lead to any clear result (i.e. the supposed "leave Dom0 handle the RTC" has never materialized over the past almost 5 years). 1: restore pv_rtc_handler() invocation 2: detect CMOS aliasing on ports other than 0x70/0x71 Jan ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2 1/2] x86: restore pv_rtc_handler() invocation 2020-07-15 9:44 [PATCH v2 0/2] x86: RTC handling adjustments Jan Beulich @ 2020-07-15 9:47 ` Jan Beulich 2020-07-15 10:01 ` Paul Durrant 2020-07-15 9:47 ` [PATCH v2 2/2] x86: detect CMOS aliasing on ports other than 0x70/0x71 Jan Beulich 1 sibling, 1 reply; 7+ messages in thread From: Jan Beulich @ 2020-07-15 9:47 UTC (permalink / raw) To: xen-devel; +Cc: Andrew Cooper, Roger Pau Monné, Wei Liu, Paul Durrant This was lost when making the logic accessible to PVH Dom0. Fixes: 835d8d69d96a ("x86/rtc: provide mediated access to RTC for PVH dom0") Signed-off-by: Jan Beulich <jbeulich@suse.com> --- a/xen/arch/x86/time.c +++ b/xen/arch/x86/time.c @@ -1160,6 +1160,10 @@ void rtc_guest_write(unsigned int port, case RTC_PORT(1): if ( !ioports_access_permitted(currd, RTC_PORT(0), RTC_PORT(1)) ) break; + + if ( pv_rtc_handler ) + pv_rtc_handler(currd->arch.cmos_idx & 0x7f, data); + spin_lock_irqsave(&rtc_lock, flags); outb(currd->arch.cmos_idx & 0x7f, RTC_PORT(0)); outb(data, RTC_PORT(1)); ^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH v2 1/2] x86: restore pv_rtc_handler() invocation 2020-07-15 9:47 ` [PATCH v2 1/2] x86: restore pv_rtc_handler() invocation Jan Beulich @ 2020-07-15 10:01 ` Paul Durrant 2020-07-15 10:08 ` Jan Beulich 0 siblings, 1 reply; 7+ messages in thread From: Paul Durrant @ 2020-07-15 10:01 UTC (permalink / raw) To: 'Jan Beulich', xen-devel Cc: 'Andrew Cooper', 'Wei Liu', 'Roger Pau Monné' > -----Original Message----- > From: Jan Beulich <jbeulich@suse.com> > Sent: 15 July 2020 10:47 > To: xen-devel@lists.xenproject.org > Cc: Andrew Cooper <andrew.cooper3@citrix.com>; Paul Durrant <paul@xen.org>; Wei Liu <wl@xen.org>; > Roger Pau Monné <roger.pau@citrix.com> > Subject: [PATCH v2 1/2] x86: restore pv_rtc_handler() invocation > > This was lost when making the logic accessible to PVH Dom0. > > Fixes: 835d8d69d96a ("x86/rtc: provide mediated access to RTC for PVH dom0") > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > --- a/xen/arch/x86/time.c > +++ b/xen/arch/x86/time.c > @@ -1160,6 +1160,10 @@ void rtc_guest_write(unsigned int port, > case RTC_PORT(1): > if ( !ioports_access_permitted(currd, RTC_PORT(0), RTC_PORT(1)) ) > break; > + > + if ( pv_rtc_handler ) > + pv_rtc_handler(currd->arch.cmos_idx & 0x7f, data); > + This appears to be semantically slightly different to the old code in that it is only done for a write to RC_PORT(1), whereas it would have been done on a write to either RTC_POR(0) or RTC_PORT(1) before. Is that of any concern? Paul > spin_lock_irqsave(&rtc_lock, flags); > outb(currd->arch.cmos_idx & 0x7f, RTC_PORT(0)); > outb(data, RTC_PORT(1)); ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/2] x86: restore pv_rtc_handler() invocation 2020-07-15 10:01 ` Paul Durrant @ 2020-07-15 10:08 ` Jan Beulich 0 siblings, 0 replies; 7+ messages in thread From: Jan Beulich @ 2020-07-15 10:08 UTC (permalink / raw) To: paul Cc: xen-devel, 'Roger Pau Monné', 'Wei Liu', 'Andrew Cooper' On 15.07.2020 12:01, Paul Durrant wrote: >> -----Original Message----- >> From: Jan Beulich <jbeulich@suse.com> >> Sent: 15 July 2020 10:47 >> To: xen-devel@lists.xenproject.org >> Cc: Andrew Cooper <andrew.cooper3@citrix.com>; Paul Durrant <paul@xen.org>; Wei Liu <wl@xen.org>; >> Roger Pau Monné <roger.pau@citrix.com> >> Subject: [PATCH v2 1/2] x86: restore pv_rtc_handler() invocation >> >> This was lost when making the logic accessible to PVH Dom0. >> >> Fixes: 835d8d69d96a ("x86/rtc: provide mediated access to RTC for PVH dom0") >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> >> --- a/xen/arch/x86/time.c >> +++ b/xen/arch/x86/time.c >> @@ -1160,6 +1160,10 @@ void rtc_guest_write(unsigned int port, >> case RTC_PORT(1): >> if ( !ioports_access_permitted(currd, RTC_PORT(0), RTC_PORT(1)) ) >> break; >> + >> + if ( pv_rtc_handler ) >> + pv_rtc_handler(currd->arch.cmos_idx & 0x7f, data); >> + > > This appears to be semantically slightly different to the old code in that it is only done for a write to RC_PORT(1), whereas it would have been done on a write to either RTC_POR(0) or RTC_PORT(1) before. Is that of any concern? The old code was (quoting plain 4.13.1) else if ( port == RTC_PORT(0) ) { currd->arch.cmos_idx = data; } else if ( (port == RTC_PORT(1)) && ioports_access_permitted(currd, RTC_PORT(0), RTC_PORT(1)) ) { unsigned long flags; if ( pv_rtc_handler ) pv_rtc_handler(currd->arch.cmos_idx & 0x7f, data); 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); } which I think similarly invoked the hook for RTC_PORT(1) only. Jan ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2 2/2] x86: detect CMOS aliasing on ports other than 0x70/0x71 2020-07-15 9:44 [PATCH v2 0/2] x86: RTC handling adjustments Jan Beulich 2020-07-15 9:47 ` [PATCH v2 1/2] x86: restore pv_rtc_handler() invocation Jan Beulich @ 2020-07-15 9:47 ` Jan Beulich 2020-07-16 14:31 ` Roger Pau Monné 1 sibling, 1 reply; 7+ messages in thread From: Jan Beulich @ 2020-07-15 9:47 UTC (permalink / raw) To: xen-devel; +Cc: Andrew Cooper, Wei Liu, Roger Pau Monné ... in order to also intercept accesses through the alias ports. Also stop intercepting accesses to the CMOS ports if we won't ourselves use the CMOS RTC. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- v2: Re-base. --- a/xen/arch/x86/physdev.c +++ b/xen/arch/x86/physdev.c @@ -670,6 +670,80 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_H return ret; } +#ifndef COMPAT +#include <asm/mc146818rtc.h> + +unsigned int __read_mostly cmos_alias_mask; + +static int __init probe_cmos_alias(void) +{ + unsigned int i, offs; + + if ( acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_CMOS_RTC ) + return 0; + + for ( offs = 2; offs < 8; offs <<= 1 ) + { + 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); + +/* Has the administrator granted sufficient permission for this I/O access? */ +bool admin_io_okay(unsigned int port, unsigned int bytes, + const struct domain *d) +{ + /* + * Port 0xcf8 (CONFIG_ADDRESS) is only visible for DWORD accesses. + * We never permit direct access to that register. + */ + if ( (port == 0xcf8) && (bytes == 4) ) + return false; + + /* + * We also never permit direct access to the RTC/CMOS registers + * if we may be accessing the RTC ones ourselves. + */ + if ( !(acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_CMOS_RTC) && + ((port & ~(cmos_alias_mask | 1)) == RTC_PORT(0)) ) + return false; + + return ioports_access_permitted(d, port, port + bytes - 1); +} +#endif /* COMPAT */ + /* * Local variables: * mode: C --- a/xen/arch/x86/pv/emul-priv-op.c +++ b/xen/arch/x86/pv/emul-priv-op.c @@ -198,24 +198,6 @@ static bool guest_io_okay(unsigned int p return false; } -/* Has the administrator granted sufficient permission for this I/O access? */ -static bool admin_io_okay(unsigned int port, unsigned int bytes, - const struct domain *d) -{ - /* - * Port 0xcf8 (CONFIG_ADDRESS) is only visible for DWORD accesses. - * We never permit direct access to that register. - */ - if ( (port == 0xcf8) && (bytes == 4) ) - return false; - - /* We also never permit direct access to the RTC/CMOS registers. */ - if ( ((port & ~1) == RTC_PORT(0)) ) - return false; - - return ioports_access_permitted(d, port, port + bytes - 1); -} - static bool pci_cfg_ok(struct domain *currd, unsigned int start, unsigned int size, uint32_t *write) { --- a/xen/arch/x86/setup.c +++ b/xen/arch/x86/setup.c @@ -48,7 +48,7 @@ #include <xen/cpu.h> #include <asm/nmi.h> #include <asm/alternative.h> -#include <asm/mc146818rtc.h> +#include <asm/iocap.h> #include <asm/cpuid.h> #include <asm/spec_ctrl.h> #include <asm/guest.h> @@ -2009,37 +2009,33 @@ int __hwdom_init xen_in_range(unsigned l static int __hwdom_init 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); + if ( admin_io_okay(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()), which isn't covered by + * the admin_io_okay() check in io_bitmap_cb(). + */ + __set_bit(0xcf8, d->arch.hvm.io_bitmap); } /* --- a/xen/arch/x86/time.c +++ b/xen/arch/x86/time.c @@ -1092,7 +1092,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; } @@ -1114,7 +1117,7 @@ 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): /* @@ -1126,11 +1129,12 @@ unsigned int rtc_guest_read(unsigned int 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; @@ -1146,8 +1150,10 @@ void rtc_guest_write(unsigned int port, struct domain *currd = current->domain; unsigned long flags; - switch ( port ) + switch ( port & ~cmos_alias_mask ) { + unsigned int idx; + case RTC_PORT(0): /* * All PV domains (and PVH dom0) are allowed to write to the latched @@ -1158,15 +1164,17 @@ void rtc_guest_write(unsigned int port, 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))); + if ( pv_rtc_handler ) - pv_rtc_handler(currd->arch.cmos_idx & 0x7f, data); + pv_rtc_handler(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; --- a/xen/include/asm-x86/iocap.h +++ b/xen/include/asm-x86/iocap.h @@ -18,4 +18,7 @@ (!rangeset_is_empty((d)->iomem_caps) || \ !rangeset_is_empty((d)->arch.ioport_caps)) +bool admin_io_okay(unsigned int port, unsigned int bytes, + const struct domain *d); + #endif /* __X86_IOCAP_H__ */ --- a/xen/include/asm-x86/mc146818rtc.h +++ b/xen/include/asm-x86/mc146818rtc.h @@ -9,6 +9,8 @@ extern spinlock_t rtc_lock; /* serialize CMOS RAM access */ +extern unsigned int cmos_alias_mask; + /********************************************************************** * register summary **********************************************************************/ ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 2/2] x86: detect CMOS aliasing on ports other than 0x70/0x71 2020-07-15 9:47 ` [PATCH v2 2/2] x86: detect CMOS aliasing on ports other than 0x70/0x71 Jan Beulich @ 2020-07-16 14:31 ` Roger Pau Monné 2020-07-17 10:00 ` Jan Beulich 0 siblings, 1 reply; 7+ messages in thread From: Roger Pau Monné @ 2020-07-16 14:31 UTC (permalink / raw) To: Jan Beulich; +Cc: xen-devel, Wei Liu, Andrew Cooper On Wed, Jul 15, 2020 at 11:47:56AM +0200, Jan Beulich wrote: > ... in order to also intercept accesses through the alias ports. > > Also stop intercepting accesses to the CMOS ports if we won't ourselves > use the CMOS RTC. I think you are missing the registration of the aliased ports in rtc_init for a PVH hardware domain, hw_rtc_io will currently only get called by accesses to 0x71-0x71. > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > v2: Re-base. > > --- a/xen/arch/x86/physdev.c > +++ b/xen/arch/x86/physdev.c > @@ -670,6 +670,80 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_H > return ret; > } > > +#ifndef COMPAT > +#include <asm/mc146818rtc.h> > + > +unsigned int __read_mostly cmos_alias_mask; > + > +static int __init probe_cmos_alias(void) > +{ > + unsigned int i, offs; > + > + if ( acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_CMOS_RTC ) > + return 0; > + > + for ( offs = 2; offs < 8; offs <<= 1 ) > + { > + 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; I'm missing something, why do you avoid the century register for comparison reasons? > @@ -2009,37 +2009,33 @@ int __hwdom_init xen_in_range(unsigned l > static int __hwdom_init io_bitmap_cb(unsigned long s, unsigned long e, > void *ctx) > { > - struct domain *d = ctx; > + const struct domain *d = ctx; Urg, it's kind of weird to constify d ... > unsigned int i; > > ASSERT(e <= INT_MAX); > for ( i = s; i <= e; i++ ) > - __clear_bit(i, d->arch.hvm.io_bitmap); > + if ( admin_io_okay(i, 1, d) ) > + __clear_bit(i, d->arch.hvm.io_bitmap); ... when you are modifying the bitmap here. > > 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(); You can directly use BUG_ON, no need for the if. IIRC it's safe to call admin_io_okay (and thus ioports_access_permitted) when already holding the rangeset lock, as both are read-lockers and can safely recurse. > + > + /* > + * We need to trap 4-byte accesses to 0xcf8 (see admin_io_okay(), > + * guest_io_read(), and guest_io_write()), which isn't covered by > + * the admin_io_okay() check in io_bitmap_cb(). > + */ > + __set_bit(0xcf8, d->arch.hvm.io_bitmap); > } > > /* > --- a/xen/arch/x86/time.c > +++ b/xen/arch/x86/time.c > @@ -1092,7 +1092,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; Do you need to set this flag also when using the EFI runtime services in order to get the time in get_cmos_time? In that case the RTC is not use, and hence could be handled to the hardware domain? > + } > break; > } > > @@ -1114,7 +1117,7 @@ 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): > /* > @@ -1126,11 +1129,12 @@ unsigned int rtc_guest_read(unsigned int > 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))), Why do you only mask this for accesses to the non aliased ports? If the RTC is aliased you also want to mask the aliased accesses in the same way? Thanks, Roger. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 2/2] x86: detect CMOS aliasing on ports other than 0x70/0x71 2020-07-16 14:31 ` Roger Pau Monné @ 2020-07-17 10:00 ` Jan Beulich 0 siblings, 0 replies; 7+ messages in thread From: Jan Beulich @ 2020-07-17 10:00 UTC (permalink / raw) To: Roger Pau Monné; +Cc: xen-devel, Wei Liu, Andrew Cooper On 16.07.2020 16:31, Roger Pau Monné wrote: > On Wed, Jul 15, 2020 at 11:47:56AM +0200, Jan Beulich wrote: >> ... in order to also intercept accesses through the alias ports. >> >> Also stop intercepting accesses to the CMOS ports if we won't ourselves >> use the CMOS RTC. > > I think you are missing the registration of the aliased ports in > rtc_init for a PVH hardware domain, hw_rtc_io will currently only get > called by accesses to 0x71-0x71. Oh, right - a re-basing oversight. Thanks for noticing. (It's not just the registration that's missing, but also the avoiding of it in case ACPI_FADT_NO_CMOS_RTC is set.) >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> --- >> v2: Re-base. >> >> --- a/xen/arch/x86/physdev.c >> +++ b/xen/arch/x86/physdev.c >> @@ -670,6 +670,80 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_H >> return ret; >> } >> >> +#ifndef COMPAT >> +#include <asm/mc146818rtc.h> >> + >> +unsigned int __read_mostly cmos_alias_mask; >> + >> +static int __init probe_cmos_alias(void) >> +{ >> + unsigned int i, offs; >> + >> + if ( acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_CMOS_RTC ) >> + return 0; >> + >> + for ( offs = 2; offs < 8; offs <<= 1 ) >> + { >> + 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; > > I'm missing something, why do you avoid the century register for > comparison reasons? Just like the other RTC registers - their contents may change behind our backs, here e.g. over New Year between two centuries. >> @@ -2009,37 +2009,33 @@ int __hwdom_init xen_in_range(unsigned l >> static int __hwdom_init io_bitmap_cb(unsigned long s, unsigned long e, >> void *ctx) >> { >> - struct domain *d = ctx; >> + const struct domain *d = ctx; > > Urg, it's kind of weird to constify d ... > >> unsigned int i; >> >> ASSERT(e <= INT_MAX); >> for ( i = s; i <= e; i++ ) >> - __clear_bit(i, d->arch.hvm.io_bitmap); >> + if ( admin_io_okay(i, 1, d) ) >> + __clear_bit(i, d->arch.hvm.io_bitmap); > > ... when you are modifying the bitmap here. Well - I'm not modifying what d points to. In principle these I/O bitmaps are shared; It's just Dom0 which gets a separate one. So modifying the bitmap really is unrelated to modifying struct domain. >> 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(); > > You can directly use BUG_ON, no need for the if. Long ago we agreed to avoid BUG_ON() with expressions that have required (side) effects. I.e. just like for ASSERT(), where the expression wouldn't get evaluated at all when NDEBUG is defined. > IIRC it's safe to > call admin_io_okay (and thus ioports_access_permitted) when already > holding the rangeset lock, as both are read-lockers and can safely > recurse. I'm afraid I don't see the connection of this remark to the construct in question. >> --- a/xen/arch/x86/time.c >> +++ b/xen/arch/x86/time.c >> @@ -1092,7 +1092,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; > > Do you need to set this flag also when using the EFI runtime services > in order to get the time in get_cmos_time? In that case the RTC is not > use, and hence could be handled to the hardware domain? Whether the EFI runtime services use the RTC is unknown. There are specific precautions towards this in the UEFI spec, iirc. >> @@ -1114,7 +1117,7 @@ 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): >> /* >> @@ -1126,11 +1129,12 @@ unsigned int rtc_guest_read(unsigned int >> 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))), > > Why do you only mask this for accesses to the non aliased ports? If > the RTC is aliased you also want to mask the aliased accesses in the > same way? Bit 7 in port 70 has a different meaning (NMI mask); you can access RTC/CMOS bytes 0-127 only this way. There are chipsets which provide 256-byte CMOS, where the high half can be accessed via the aliases. However, seeing this comment of yours I noticed that there's still a related bug here: When the guest reads/writes the index port, I _also_ need to mask off the high bit when it's the non-aliased port that gets accessed. Otherwise Dom0 writing port 74 but then reading port 71 could lead to bit 7 getting set in port 70. Jan ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-07-17 10:00 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-07-15 9:44 [PATCH v2 0/2] x86: RTC handling adjustments Jan Beulich 2020-07-15 9:47 ` [PATCH v2 1/2] x86: restore pv_rtc_handler() invocation Jan Beulich 2020-07-15 10:01 ` Paul Durrant 2020-07-15 10:08 ` Jan Beulich 2020-07-15 9:47 ` [PATCH v2 2/2] x86: detect CMOS aliasing on ports other than 0x70/0x71 Jan Beulich 2020-07-16 14:31 ` Roger Pau Monné 2020-07-17 10:00 ` Jan Beulich
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.