Re: [PATCH v3 4/4] acpi: Set proper maximum size for "etc/acpi/rsdp" blob

From: David Hildenbrand <david@redhat.com>
To: Igor Mammedov <imammedo@redhat.com>
Cc: Peter Maydell <peter.maydell@linaro.org>,
	"Michael S. Tsirkin" <mst@redhat.com>,
	Richard Henderson <richard.henderson@linaro.org>,
	qemu-devel@nongnu.org,
	Alistair Francis <alistair.francis@xilinx.com>,
	Shannon Zhao <shannon.zhaosl@gmail.com>,
	qemu-arm@nongnu.org, Paolo Bonzini <pbonzini@redhat.com>,
	Laszlo Ersek <lersek@redhat.com>
Subject: Re: [PATCH v3 4/4] acpi: Set proper maximum size for "etc/acpi/rsdp" blob
Date: Mon, 15 Mar 2021 13:16:01 +0100	[thread overview]
Message-ID: <23d57ebe-32aa-b535-e0cd-c3af6232bca8@redhat.com> (raw)
In-Reply-To: <20210315125403.68ef8925@redhat.com>

On 15.03.21 12:54, Igor Mammedov wrote:
> On Thu,  4 Mar 2021 11:55:54 +0100
> David Hildenbrand <david@redhat.com> wrote:
> 
>> Let's also set a maximum size for "etc/acpi/rsdp", so the maximum
>> size doesn't get implicitly set based on the initial table size. In my
>> experiments, the table size was in the range of 22 bytes, so a single
>> page (== what we used until now) seems to be good enough.
>>
>> Now that we have defined maximum sizes for all currently used table types,
>> let's assert that we catch usage with new tables that need a proper maximum
>> size definition.
>>
>> Also assert that our initial size does not exceed the maximum size; while
>> qemu_ram_alloc_internal() properly asserts that the initial RAMBlock size
>> is <= its maximum size, the result might differ when the host page size
>> is bigger than 4k.
>>
>> Suggested-by: Laszlo Ersek <lersek@redhat.com>
>> Cc: Alistair Francis <alistair.francis@xilinx.com>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> Cc: "Michael S. Tsirkin" <mst@redhat.com>
>> Cc: Igor Mammedov <imammedo@redhat.com>
>> Cc: Peter Maydell <peter.maydell@linaro.org>
>> Cc: Shannon Zhao <shannon.zhaosl@gmail.com>
>> Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> Cc: Richard Henderson <richard.henderson@linaro.org>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>   hw/acpi/utils.c | 7 ++++++-
>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/acpi/utils.c b/hw/acpi/utils.c
>> index f2d69a6d92..0c486ea29f 100644
>> --- a/hw/acpi/utils.c
>> +++ b/hw/acpi/utils.c
>> @@ -29,14 +29,19 @@
>>   MemoryRegion *acpi_add_rom_blob(FWCfgCallback update, void *opaque,
>>                                   GArray *blob, const char *name)
>>   {
>> -    uint64_t max_size = 0;
>> +    uint64_t max_size;
> [...]
>> +    } else {
>> +        g_assert_not_reached();
>>       }
>> +    g_assert(acpi_data_len(blob) <= max_size);
> 
> though it's correct,
> but theoretically compiler might get unhappy about uninitialized max_size here
> 
> though if it compiles fine with our current CI it should be good enough

I think the compiler will respect g_assert_not_reached() as intended and 
suppress warnings.

For example, see block/qed.c:qed_aio_write_data() where be don't have a 
return statement on g_assert_not_reached() exit paths.

-- 
Thanks,

David / dhildenb