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

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(-)

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

>  
>      return rom_add_blob(name, blob->data, acpi_data_len(blob), max_size, -1,
>                          name, update, opaque, NULL, true);

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.