> -----Original Message----- > From: Paolo Bonzini [mailto:pbonz...@redhat.com] > Sent: Monday, February 05, 2018 10:04 PM > To: Peter Maydell > Cc: Gonglei (Arei); QEMU Developers; Huangweidong (C) > Subject: Re: [Qemu-devel] [PATCH] rtc: placing RTC memory region outside BQL > > On 04/02/2018 19:02, Peter Maydell wrote: > > On 1 February 2018 at 14:23, Paolo Bonzini <pbonz...@redhat.com> wrote: > >> On 01/02/2018 08:47, Gonglei wrote: > >>> diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c > >>> index 35a05a6..d9d99c5 100644 > >>> --- a/hw/timer/mc146818rtc.c > >>> +++ b/hw/timer/mc146818rtc.c > >>> @@ -986,6 +986,7 @@ static void rtc_realizefn(DeviceState *dev, Error > **errp) > >>> qemu_register_suspend_notifier(&s->suspend_notifier); > >>> > >>> memory_region_init_io(&s->io, OBJECT(s), &cmos_ops, s, "rtc", 2); > >>> + memory_region_clear_global_locking(&s->io); > >>> isa_register_ioport(isadev, &s->io, base); > >>> > >>> qdev_set_legacy_instance_id(dev, base, 3); > >>> > >> > >> This is not enough, you need to add a new lock or something like that. > >> Otherwise two vCPUs can access the RTC together and make a mess. > > > > Do you also need to do something to take the global lock before > > raising the outbound IRQ line (since it might be connected to a device > > that does need the global lock), or am I confused ? > > Yes, that's a good point. Most of the time the IRQ line is raised in a > timer, but not always. > So, taking BQL is necessary, and what we can do is trying our best to narrow down the process of locking ? For example, do the following wrapping:
static void rtc_rasie_irq(RTCState *s) { qemu_mutex_lock_iothread(); qemu_irq_raise(s->irq); qemu_mutex_unlock_iothread(); } static void rtc_lower_irq(RTCState *s) { qemu_mutex_lock_iothread(); qemu_irq_lower(s->irq); qemu_mutex_unlock_iothread(); } Thanks, -Gonglei