Re: [Qemu-devel] [PATCH RFC 4/7] hw/apci: fix pcihp io initialization

[Marcel Apfelbaum <marcel@redhat.com>]

On 06/17/2016 12:04 PM, Igor Mammedov wrote:
> On Tue, 31 May 2016 20:48:35 +0300
> Marcel Apfelbaum <marcel@redhat.com> 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 <marcel@redhat.com>
>> ---
>>   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,
>