Re: [PATCH] hw/acpi: limit warning on acpi table size to pc machines older than version 2.3

[Ani Sinha]

> On 28-Mar-2023, at 7:35 PM, Anirban Sinha <anisinha@redhat.com> wrote:
> 
> 
> 
>> On 28-Mar-2023, at 5:46 PM, Igor Mammedov <imammedo@redhat.com> wrote:
>> 
>> On Mon, 20 Mar 2023 16:59:02 +0530
>> Ani Sinha <anisinha@redhat.com> wrote:
>> 
>>> i440fx machine versions 2.3 and newer and q35 machines supports dynamic ram
>>> resizing. Please see commit a1666142db6233 ("acpi-build: make ROMs RAM 
>>> blocks resizeable") .
>>> Hence the warning when the ACPI table size exceeds a pre-defined value does
>>> not apply to those machines. Add a check limiting the warning message to 
>>> only
>>> those machines that does not support expandable ram blocks, that is, i440fx
>>> machines with version 2.2 and older.
>> So q35 is not affected at all? If yes, then mention it here.
> 
> Hmm, initially I thought q35 was unaffected but seems I was mistaken. Looking 
> at 94dec5948aeb240 ("pc: rename machine types”), seems q35 machine types 
> started with version 1.4 and so any version earlier than 2.3 in q35 should 
> also be affected.

Ah never mind. We have deprecated all q35 machines older than version 2.4:

commit 86165b499edf8b03bb2d0e926d116c2f12a95bfe
Author: Eduardo Habkost <ehabkost@redhat.com>
Date:   Sat Jan 23 14:02:09 2016 -0200

    q35: Remove old machine versions
    
    Migration with q35 was not possible before commit
    04329029a8c539eb5f75dcb6d8b016f0c53a031a, because q35
    unconditionally creates an ich9-ahci device, that was marked as
    unmigratable. So all q35 machine classes before pc-q35-2.4 were
    not migratable, so there's no point in keeping compatibility code
    for them.
    
    Remove all old pc-q35 machine classes and keep only pc-q35-2.4
    and newer.
    
    Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
    Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
    Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
    Reviewed-by: Laszlo Ersek <lersek@redhat.com>
    Reviewed-by: Markus Armbruster <armbru@redhat.com>
    Reviewed-by: Igor Mammedov <imammedo@redhat.com>

So yes q35 is unaffected. I will update the commit log accordingly.


> I will take care of this.
> 
> 
>> 
>>> 
>>> Signed-off-by: Ani Sinha <anisinha@redhat.com>
>>> ---
>>> hw/i386/acpi-build.c | 6 ++++--
>>> hw/i386/pc.c         | 1 +
>>> hw/i386/pc_piix.c    | 1 +
>>> include/hw/i386/pc.h | 3 +++
>>> 4 files changed, 9 insertions(+), 2 deletions(-)
>>> 
>>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>>> index b19fb4259e..2311bea082 100644
>>> --- a/hw/i386/acpi-build.c
>>> +++ b/hw/i386/acpi-build.c
>>> @@ -2616,7 +2616,8 @@ void acpi_build(AcpiBuildTables *tables, MachineState 
>>> *machine)
>>>        int legacy_table_size =
>>>            ROUND_UP(tables_blob->len - aml_len + legacy_aml_len,
>>>                     ACPI_BUILD_ALIGN_SIZE);
>>> -        if (tables_blob->len > legacy_table_size) {
>>> +        if ((tables_blob->len > legacy_table_size) &&
>>> +            !pcmc->resizable_ram_block) {
>>>            /* Should happen only with PCI bridges and -M pc-i440fx-2.0.  */
>>>            warn_report("ACPI table size %u exceeds %d bytes,"
>>>                        " migration may not work",
>>> @@ -2627,7 +2628,8 @@ void acpi_build(AcpiBuildTables *tables, MachineState 
>>> *machine)
>>>        g_array_set_size(tables_blob, legacy_table_size);
>>>    } else {
>>>        /* Make sure we have a buffer in case we need to resize the tables. 
>>> */
>>> -        if (tables_blob->len > ACPI_BUILD_TABLE_SIZE / 2) {
>>> +        if ((tables_blob->len > ACPI_BUILD_TABLE_SIZE / 2) &&
>>> +            !pcmc->resizable_ram_block) {
>>>            /* As of QEMU 2.1, this fires with 160 VCPUs and 255 memory 
>>> slots.  */
>>>            warn_report("ACPI table size %u exceeds %d bytes,"
>>>                        " migration may not work",
>>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>>> index 7bebea57e3..822d5de333 100644
>>> --- a/hw/i386/pc.c
>>> +++ b/hw/i386/pc.c
>>> @@ -1936,6 +1936,7 @@ static void pc_machine_class_init(ObjectClass *oc, 
>>> void *data)
>>>    pcmc->acpi_data_size = 0x20000 + 0x8000;
>>>    pcmc->pvh_enabled = true;
>>>    pcmc->kvmclock_create_always = true;
>>> +    pcmc->resizable_ram_block = true;
>>>    assert(!mc->get_hotplug_handler);
>>>    mc->get_hotplug_handler = pc_get_hotplug_handler;
>>>    mc->hotplug_allowed = pc_hotplug_allowed;
>>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>>> index 2f16011bab..3c74dfcfb4 100644
>>> --- a/hw/i386/pc_piix.c
>>> +++ b/hw/i386/pc_piix.c
>>> @@ -745,6 +745,7 @@ static void pc_i440fx_2_2_machine_options(MachineClass 
>>> *m)
>>>    compat_props_add(m->compat_props, hw_compat_2_2, hw_compat_2_2_len);
>>>    compat_props_add(m->compat_props, pc_compat_2_2, pc_compat_2_2_len);
>>>    pcmc->rsdp_in_ram = false;
>>> +    pcmc->resizable_ram_block = false;
>>> }
>>> 
>>> DEFINE_I440FX_MACHINE(v2_2, "pc-i440fx-2.2", pc_compat_2_2_fn,
>>> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
>>> index 8206d5405a..3427a35f73 100644
>>> --- a/include/hw/i386/pc.h
>>> +++ b/include/hw/i386/pc.h
>>> @@ -127,6 +127,9 @@ struct PCMachineClass {
>>> 
>>>    /* create kvmclock device even when KVM PV features are not exposed */
>>>    bool kvmclock_create_always;
>>> +
>>> +    /* resizable memory block compat */
>>> +    bool resizable_ram_block;
>> perhaps more specific
>>  resizeable_acpi_blob
>> would be better
> 
> Sure, will rename accordingly.
> 
>> 
>>> };
>>> 
>>> #define TYPE_PC_MACHINE "generic-pc-machine"
>