From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47198) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Utvni-00083T-G5 for qemu-devel@nongnu.org; Tue, 02 Jul 2013 04:19:03 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Utvnh-0004cR-60 for qemu-devel@nongnu.org; Tue, 02 Jul 2013 04:19:02 -0400 Received: from mail-bk0-x232.google.com ([2a00:1450:4008:c01::232]:50891) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Utvng-0004cL-Sn for qemu-devel@nongnu.org; Tue, 02 Jul 2013 04:19:01 -0400 Received: by mail-bk0-f50.google.com with SMTP id ik8so2156772bkc.9 for ; Tue, 02 Jul 2013 01:19:00 -0700 (PDT) Sender: Paolo Bonzini Message-ID: <51D28CF0.4090806@redhat.com> Date: Tue, 02 Jul 2013 10:18:56 +0200 From: Paolo Bonzini MIME-Version: 1.0 References: <1372438702-20491-1-git-send-email-pbonzini@redhat.com> <1372438702-20491-7-git-send-email-pbonzini@redhat.com> <51D1CB70.9050308@siemens.com> <51D27638.4030804@redhat.com> <51D27D38.2090309@siemens.com> In-Reply-To: <51D27D38.2090309@siemens.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 06/11] memory: add ref/unref calls List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jan Kiszka Cc: "peter.maydell@linaro.org" , "qemu-devel@nongnu.org" Il 02/07/2013 09:11, Jan Kiszka ha scritto: > On 2013-07-02 08:42, Paolo Bonzini wrote: >> Il 01/07/2013 20:33, Jan Kiszka ha scritto: >>> On 2013-06-28 18:58, Paolo Bonzini wrote: >>>> Add ref/unref calls at the following places: >>>> >>>> - places where memory regions are stashed by a listener and >>>> used outside the BQL (including in Xen or KVM). >>>> >>>> - memory_region_find callsites >>> >>> You missed some recently added ones. Check hw/acpi/piix4.c and >>> hw/isa/lpc_ich9.c (both will require some refactorings for this). >> >> Here are the required changes: >> >> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c >> index 3b95c69..0310053 100644 >> --- a/hw/acpi/piix4.c >> +++ b/hw/acpi/piix4.c >> @@ -380,6 +380,16 @@ static void piix4_pm_powerdown_req(Notifier *n, void *opaque) >> acpi_pm1_evt_power_down(&s->ar); >> } >> >> +static bool memory_region_present(MemoryRegion *io, hwaddr port) >> +{ >> + MemoryRegion *mr = memory_region_find(io, port, 1); >> + if (!mr) { >> + return false; >> + } >> + memory_region_unref(mr); >> + return true; >> +} >> + >> static void piix4_pm_machine_ready(Notifier *n, void *opaque) >> { >> PIIX4PMState *s = container_of(n, PIIX4PMState, machine_ready); >> @@ -388,10 +398,10 @@ static void piix4_pm_machine_ready(Notifier *n, void *opaque) >> >> pci_conf = s->dev.config; >> pci_conf[0x5f] = 0x10 | >> - (memory_region_find(io_as, 0x378, 1).mr ? 0x80 : 0); >> + (memory_region_present(io_as, 0x378, 1) ? 0x80 : 0); >> pci_conf[0x63] = 0x60; >> - pci_conf[0x67] = (memory_region_find(io_as, 0x3f8, 1).mr ? 0x08 : 0) | >> - (memory_region_find(io_as, 0x2f8, 1).mr ? 0x90 : 0); >> + pci_conf[0x67] = (memory_region_present(io_as, 0x3f8, 1) ? 0x08 : 0) | >> + (memory_region_present(io_as, 0x2f8, 1) ? 0x90 : 0); >> } >> >> static int piix4_pm_initfn(PCIDevice *dev) >> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c >> index 82f8ea6..205ac26 100644 >> --- a/hw/isa/lpc_ich9.c >> +++ b/hw/isa/lpc_ich9.c >> @@ -478,24 +478,33 @@ static void ich9_lpc_machine_ready(Notifier *n, void *opaque) >> { >> ICH9LPCState *s = container_of(n, ICH9LPCState, machine_ready); >> MemoryRegion *io_as = pci_address_space_io(&s->d); >> + MemoryRegion *mr; >> uint8_t *pci_conf; >> >> pci_conf = s->d.config; >> - if (memory_region_find(io_as, 0x3f8, 1).mr) { >> + mr = memory_region_find(io_as, 0x3f8, 1).mr; >> + if (mr) { >> /* com1 */ >> pci_conf[0x82] |= 0x01; >> + memory_region_unref(mr); >> } >> - if (memory_region_find(io_as, 0x2f8, 1).mr) { >> + mr = memory_region_find(io_as, 0x2f8, 1).mr; >> + if (mr) { >> /* com2 */ >> pci_conf[0x82] |= 0x02; >> + memory_region_unref(mr); >> } >> - if (memory_region_find(io_as, 0x378, 1).mr) { >> + mr = memory_region_find(io_as, 0x378, 1).mr; >> + if (mr) { >> /* lpt */ >> pci_conf[0x82] |= 0x04; >> + memory_region_unref(mr); >> } >> - if (memory_region_find(io_as, 0x3f0, 1).mr) { >> + mr = memory_region_find(io_as, 0x3f0, 1).mr; >> + if (mr) { >> /* floppy */ >> pci_conf[0x82] |= 0x08; >> + memory_region_unref(mr); >> } >> } >> >> >> > > Hmm, two solutions for one problem - can we improve this in the next round? Sure, I can adapt the hw/acpi/piix4.c to use ifs in the same style as hw/isa/lpc_ich9.c (I find the code easier to read). Paolo