From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53317) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Utw4X-0003Dk-T0 for qemu-devel@nongnu.org; Tue, 02 Jul 2013 04:36:27 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Utw4W-0002ed-K7 for qemu-devel@nongnu.org; Tue, 02 Jul 2013 04:36:25 -0400 Received: from david.siemens.de ([192.35.17.14]:19979) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Utw4W-0002eH-AB for qemu-devel@nongnu.org; Tue, 02 Jul 2013 04:36:24 -0400 Message-ID: <51D29104.6020109@siemens.com> Date: Tue, 02 Jul 2013 10:36:20 +0200 From: Jan Kiszka 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> <51D28CF0.4090806@redhat.com> In-Reply-To: <51D28CF0.4090806@redhat.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: Paolo Bonzini Cc: "peter.maydell@linaro.org" , "qemu-devel@nongnu.org" On 2013-07-02 10:18, Paolo Bonzini wrote: > 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). I was more referring to memory_region_present vs. open-coding. Jan -- Siemens AG, Corporate Technology, CT RTC ITP SES-DE Corporate Competence Center Embedded Linux