From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56489) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bFHV4-0002HO-Rb for qemu-devel@nongnu.org; Tue, 21 Jun 2016 04:57:39 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bFHUy-0002oQ-QA for qemu-devel@nongnu.org; Tue, 21 Jun 2016 04:57:37 -0400 Received: from mx1.redhat.com ([209.132.183.28]:43710) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bFHUy-0002oJ-I6 for qemu-devel@nongnu.org; Tue, 21 Jun 2016 04:57:32 -0400 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 1B12B8E008 for ; Tue, 21 Jun 2016 08:57:32 +0000 (UTC) References: <1464716918-29689-1-git-send-email-marcel@redhat.com> <1464716918-29689-5-git-send-email-marcel@redhat.com> <20160617110457.1bd2fa8e@nial.brq.redhat.com> From: Marcel Apfelbaum Message-ID: <57690179.9010009@redhat.com> Date: Tue, 21 Jun 2016 11:57:29 +0300 MIME-Version: 1.0 In-Reply-To: <20160617110457.1bd2fa8e@nial.brq.redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH RFC 4/7] hw/apci: fix pcihp io initialization List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Mammedov Cc: qemu-devel@nongnu.org, mst@redhat.com, pbonzini@redhat.com, ehabkost@redhat.com On 06/17/2016 12:04 PM, Igor Mammedov wrote: > On Tue, 31 May 2016 20:48:35 +0300 > Marcel Apfelbaum wrote: > >> The pm initialization code assigns the pcihp IO base and length to -1 on error, >> but the later code will assume 0 as invalid value. >> >> Fix it initializing the above value to 0 as expected. >> >> Signed-off-by: Marcel Apfelbaum >> --- >> hw/i386/acpi-build.c | 10 ++++++---- >> 1 file changed, 6 insertions(+), 4 deletions(-) >> >> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c >> index 0c329fb..2097c4c 100644 >> --- a/hw/i386/acpi-build.c >> +++ b/hw/i386/acpi-build.c >> @@ -124,17 +124,19 @@ static void acpi_get_pm_info(AcpiPmInfo *pm) >> Object *lpc = ich9_lpc_find(); >> Object *obj = NULL; >> QObject *o; >> + int pcihp_io_len, pcihp_io_base; >> >> pm->cpu_hp_io_base = 0; >> - pm->pcihp_io_base = 0; >> - pm->pcihp_io_len = 0; > this introduces uninitialized memory access in q35 case > How? We are always checking pcihp_io_len/pcihp_io_base. >> if (piix) { >> obj = piix; >> pm->cpu_hp_io_base = PIIX4_CPU_HOTPLUG_IO_BASE; >> - pm->pcihp_io_base = >> + pcihp_io_base = >> object_property_get_int(obj, ACPI_PCIHP_IO_BASE_PROP, NULL); >> - pm->pcihp_io_len = >> + pcihp_io_len = >> object_property_get_int(obj, ACPI_PCIHP_IO_LEN_PROP, NULL); >> + >> + pm->pcihp_io_base = (pcihp_io_base == -1) ? 0 : pcihp_io_base; >> + pm->pcihp_io_len = (pcihp_io_len == -1) ? 0 : pcihp_io_len; >> } >> if (lpc) { >> obj = lpc; > > how about something like that: Please see the next patch. It is only a temporary measure, the next patch initialize it right to ACPI_PCIHP_IO_BASE_PROP/ACPI_PCIHP_IO_LEN_PROP. If the properties are not there, they will be 0, but we are checking this everywhere. Also I'll prefer pm->pcihp_io_len 0 length to mean "unused" because it is used widely. If you still think is a real issue, I'll change this, of course. Thanks, Marcel > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > index 4f9aec6..d753e25 100644 > --- a/hw/i386/acpi-build.c > +++ b/hw/i386/acpi-build.c > @@ -125,7 +125,7 @@ static void acpi_get_pm_info(AcpiPmInfo *pm) > > pm->cpu_hp_io_base = 0; > pm->pcihp_io_base = 0; > - pm->pcihp_io_len = 0; > + pm->pcihp_io_len = UINT16_MAX; > if (piix) { > obj = piix; > pm->cpu_hp_io_base = PIIX4_CPU_HOTPLUG_IO_BASE; > @@ -2053,7 +2053,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, > g_ptr_array_free(mem_ranges, true); > > /* reserve PCIHP resources */ > - if (pm->pcihp_io_len) { > + if (pm->pcihp_io_len != UINT16_MAX) { > dev = aml_device("PHPR"); > aml_append(dev, aml_name_decl("_HID", aml_string("PNP0A06"))); > aml_append(dev, >