[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH RFC 4/7] hw/apci: fix pcihp io initialization
From: |
Igor Mammedov |
Subject: |
Re: [Qemu-devel] [PATCH RFC 4/7] hw/apci: fix pcihp io initialization |
Date: |
Tue, 21 Jun 2016 13:19:18 +0200 |
On Tue, 21 Jun 2016 11:57:29 +0300
Marcel Apfelbaum <address@hidden> wrote:
> On 06/17/2016 12:04 PM, Igor Mammedov wrote:
> > On Tue, 31 May 2016 20:48:35 +0300
> > Marcel Apfelbaum <address@hidden> 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 <address@hidden>
> >> ---
> >> 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.
pm->pcihp_io_len is left uninitialized in q35 case
and then following build_dsdt() would cause jump on uninitialized value
/* reserve PCIHP resources */
if (pm->pcihp_io_len) {
>
> >> 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.
Maybe squash it into the next patch?
>
>
> 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,
> >
>
>