Re: [Qemu-devel] [PATCH] rtc: placing RTC memory region outside BQL

[Gonglei (Arei)]

> -----Original Message-----
> From: Paolo Bonzini [mailto:address@hidden
> 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 <address@hidden> 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