Since RTC/CMOS accesses aren't atomic, there are possible races between code paths setting the index register and subsequently reading/writing the data register. This is supposed to be dealt with by acquiring rtc_lock, but two places up to now lacked respective synchronization: Accesses to the EFI time functions and smpboot_{setup,restore}_warm_reset_vector(). This in turn requires no longer directly passing through guest writes to the index register, but instead using a machanism similar to that for PCI config space method 1 accesses. Signed-off-by: Jan Beulich --- a/xen/arch/x86/efi/runtime.c +++ b/xen/arch/x86/efi/runtime.c @@ -2,8 +2,9 @@ #include #include #include -#include #include +#include +#include DEFINE_XEN_GUEST_HANDLE(CHAR16); @@ -81,9 +82,11 @@ unsigned long efi_get_time(void) { EFI_TIME time; EFI_STATUS status; - unsigned long cr3 = efi_rs_enter(); + unsigned long cr3 = efi_rs_enter(), flags; + spin_lock_irqsave(&rtc_lock, flags); status = efi_rs->GetTime(&time, NULL); + spin_unlock_irqrestore(&rtc_lock, flags); efi_rs_leave(cr3); if ( EFI_ERROR(status) ) @@ -224,7 +227,7 @@ static inline EFI_GUID *cast_guid(struct int efi_runtime_call(struct xenpf_efi_runtime_call *op) { - unsigned long cr3; + unsigned long cr3, flags; EFI_STATUS status = EFI_NOT_STARTED; int rc = 0; @@ -238,7 +241,9 @@ int efi_runtime_call(struct xenpf_efi_ru return -EINVAL; cr3 = efi_rs_enter(); + spin_lock_irqsave(&rtc_lock, flags); status = efi_rs->GetTime(cast_time(&op->u.get_time.time), &caps); + spin_unlock_irqrestore(&rtc_lock, flags); efi_rs_leave(cr3); if ( !EFI_ERROR(status) ) @@ -256,7 +261,9 @@ int efi_runtime_call(struct xenpf_efi_ru return -EINVAL; cr3 = efi_rs_enter(); + spin_lock_irqsave(&rtc_lock, flags); status = efi_rs->SetTime(cast_time(&op->u.set_time)); + spin_unlock_irqrestore(&rtc_lock, flags); efi_rs_leave(cr3); break; @@ -268,8 +275,10 @@ int efi_runtime_call(struct xenpf_efi_ru return -EINVAL; cr3 = efi_rs_enter(); + spin_lock_irqsave(&rtc_lock, flags); status = efi_rs->GetWakeupTime(&enabled, &pending, cast_time(&op->u.get_wakeup_time)); + spin_unlock_irqrestore(&rtc_lock, flags); efi_rs_leave(cr3); if ( !EFI_ERROR(status) ) @@ -288,12 +297,14 @@ int efi_runtime_call(struct xenpf_efi_ru return -EINVAL; cr3 = efi_rs_enter(); + spin_lock_irqsave(&rtc_lock, flags); status = efi_rs->SetWakeupTime(!!(op->misc & XEN_EFI_SET_WAKEUP_TIME_ENABLE), (op->misc & XEN_EFI_SET_WAKEUP_TIME_ENABLE_ONLY) ? NULL : cast_time(&op->u.set_wakeup_time)); + spin_unlock_irqrestore(&rtc_lock, flags); efi_rs_leave(cr3); op->misc = 0; --- a/xen/arch/x86/hpet.c +++ b/xen/arch/x86/hpet.c @@ -502,18 +502,10 @@ static void hpet_detach_channel(unsigned #include -void (*__read_mostly pv_rtc_handler)(unsigned int port, uint8_t value); +void (*__read_mostly pv_rtc_handler)(uint8_t index, uint8_t value); -static void handle_rtc_once(unsigned int port, uint8_t value) +static void handle_rtc_once(uint8_t index, uint8_t value) { - static int index; - - if ( port == 0x70 ) - { - index = value; - return; - } - if ( index != RTC_REG_B ) return; --- a/xen/arch/x86/traps.c +++ b/xen/arch/x86/traps.c @@ -69,6 +69,7 @@ #include #include #include +#include #include #include @@ -1640,6 +1641,10 @@ static int admin_io_okay( if ( (port == 0xcf8) && (bytes == 4) ) return 0; + /* We also never permit direct access to the RTC/CMOS registers. */ + if ( ((port & ~1) == RTC_PORT(0)) ) + return 0; + return ioports_access_permitted(v->domain, port, port + bytes - 1); } @@ -1669,6 +1674,21 @@ static uint32_t guest_io_read( { sub_data = pv_pit_handler(port, 0, 0); } + else if ( (port == RTC_PORT(0)) ) + { + sub_data = v->domain->arch.cmos_idx; + } + else if ( (port == RTC_PORT(1)) && + ioports_access_permitted(v->domain, RTC_PORT(0), + RTC_PORT(1)) ) + { + unsigned long flags; + + spin_lock_irqsave(&rtc_lock, flags); + outb(v->domain->arch.cmos_idx & 0x7f, RTC_PORT(0)); + sub_data = inb(RTC_PORT(1)); + spin_unlock_irqrestore(&rtc_lock, flags); + } else if ( (port == 0xcf8) && (bytes == 4) ) { size = 4; @@ -1702,8 +1722,6 @@ static void guest_io_write( { switch ( bytes ) { case 1: - if ( ((port == 0x70) || (port == 0x71)) && pv_rtc_handler ) - pv_rtc_handler(port, (uint8_t)data); outb((uint8_t)data, port); if ( pv_post_outb_hook ) pv_post_outb_hook(port, (uint8_t)data); @@ -1726,6 +1744,23 @@ static void guest_io_write( { pv_pit_handler(port, (uint8_t)data, 1); } + else if ( (port == RTC_PORT(0)) ) + { + v->domain->arch.cmos_idx = data; + } + else if ( (port == RTC_PORT(1)) && + ioports_access_permitted(v->domain, RTC_PORT(0), + RTC_PORT(1)) ) + { + unsigned long flags; + + if ( pv_rtc_handler ) + pv_rtc_handler(v->domain->arch.cmos_idx & 0x7f, data); + spin_lock_irqsave(&rtc_lock, flags); + outb(v->domain->arch.cmos_idx & 0x7f, RTC_PORT(0)); + outb(data, RTC_PORT(1)); + spin_unlock_irqrestore(&rtc_lock, flags); + } else if ( (port == 0xcf8) && (bytes == 4) ) { size = 4; @@ -2091,10 +2126,6 @@ static int emulate_privileged_op(struct goto fail; if ( admin_io_okay(port, op_bytes, v, regs) ) { - if ( (op_bytes == 1) && - ((port == 0x71) || (port == 0x70)) && - pv_rtc_handler ) - pv_rtc_handler(port, regs->eax); io_emul(regs); if ( (op_bytes == 1) && pv_post_outb_hook ) pv_post_outb_hook(port, regs->eax); --- a/xen/include/asm-x86/domain.h +++ b/xen/include/asm-x86/domain.h @@ -258,6 +258,7 @@ struct arch_domain /* I/O-port admin-specified access capabilities. */ struct rangeset *ioport_caps; uint32_t pci_cf8; + uint8_t cmos_idx; struct list_head pdev_list; --- a/xen/include/asm-x86/hpet.h +++ b/xen/include/asm-x86/hpet.h @@ -74,6 +74,6 @@ void hpet_broadcast_exit(void); int hpet_broadcast_is_available(void); void hpet_disable_legacy_broadcast(void); -extern void (*pv_rtc_handler)(unsigned int port, uint8_t value); +extern void (*pv_rtc_handler)(uint8_t reg, uint8_t value); #endif /* __X86_HPET_H__ */ --- a/xen/include/asm-x86/mach-default/smpboot_hooks.h +++ b/xen/include/asm-x86/mach-default/smpboot_hooks.h @@ -3,7 +3,11 @@ static inline void smpboot_setup_warm_reset_vector(unsigned long start_eip) { + unsigned long flags; + + spin_lock_irqsave(&rtc_lock, flags); CMOS_WRITE(0xa, 0xf); + spin_unlock_irqrestore(&rtc_lock, flags); flush_tlb_local(); Dprintk("1.\n"); *((volatile unsigned short *) TRAMPOLINE_HIGH) = start_eip >> 4; @@ -14,6 +18,8 @@ static inline void smpboot_setup_warm_re static inline void smpboot_restore_warm_reset_vector(void) { + unsigned long flags; + /* * Install writable page 0 entry to set BIOS data area. */ @@ -23,7 +29,9 @@ static inline void smpboot_restore_warm_ * Paranoid: Set warm reset code and vector here back * to default values. */ + spin_lock_irqsave(&rtc_lock, flags); CMOS_WRITE(0, 0xf); + spin_unlock_irqrestore(&rtc_lock, flags); *((volatile int *) maddr_to_virt(0x467)) = 0; }