Re: [PATCH v1] acpi: increase maximum size for "etc/table-loader" blob

[David Hildenbrand <david@redhat.com>]

On 03.03.21 17:09, Igor Mammedov wrote:
> On Wed, 3 Mar 2021 16:03:36 +0100
> Laszlo Ersek <lersek@redhat.com> wrote:
> 
>> On 03/02/21 19:43, David Hildenbrand wrote:
>>
>>> We are dealing with different blobs here (tables_blob vs. cmd_blob).
>>
>> OK, thanks -- this was the important bit I was missing. Over time I've
>> lost track of the actual set of fw_cfg blobs that QEMU exposes, for the
>> purposes of the ACPI linker/loader.
>>
>> I've looked up the acpi_add_rom_blob() calls in "hw/i386/acpi-build.c"
>> and "hw/arm/virt-acpi-build.c":
>>
>>    hw       name                                         max_size                              notes
>>    -------  -------------------------------------------  ------------------------------------  ------
>>
>>    virt     ACPI_BUILD_TABLE_FILE ("etc/acpi/tables")    ACPI_BUILD_TABLE_MAX_SIZE (0x200000)  n/a
>>    virt     ACPI_BUILD_LOADER_FILE ("etc/table-loader")  0                                     n/a
>>    virt     ACPI_BUILD_RSDP_FILE ("etc/acpi/rsdp")       0                                     simply modeled on i386 (below)
>>
>>    i386     ACPI_BUILD_TABLE_FILE ("etc/acpi/tables")    ACPI_BUILD_TABLE_MAX_SIZE (0x200000)  n/a
>>    i386     ACPI_BUILD_LOADER_FILE ("etc/table-loader")  0                                     n/a
>>    i386     ACPI_BUILD_RSDP_FILE ("etc/acpi/rsdp")       0                                     d70414a5788c, 358774d780ee8
>>
>>    microvm  ACPI_BUILD_TABLE_FILE ("etc/acpi/tables")    ACPI_BUILD_TABLE_MAX_SIZE (0x200000)  n/a
>>    microvm  "etc/table-loader"                           0                                     no macro for name???
>>    microvm  ACPI_BUILD_RSDP_FILE ("etc/acpi/rsdp")       0                                     simply modeled on i386 (above)
>>
>> (I notice there are some other (optional) fw_cfg blobs too, related TPM,
>> vmgenid, nvdimm etc, using fw_cfg_add_file() rather than
>> acpi_add_rom_blob() -- so those are immutable (never regenerated). I
>> definitely needed this reminder...)
> 
> most of them are just guest RAM reservations (guest/hose exchange buffer)
> and "etc/tpm/config" seems to immutable for specific configuration
> 
> 
>> So, my observations:
>>
>> (1) microvm open-codes "etc/table-loader", rather than using the macro
>> ACPI_BUILD_LOADER_FILE.
>>
>> The proposed patch corrects it, which I welcome per se. However, it
>> should arguably be a separate patch. I found it distracting, in spite of
>> the commit message highlighting it. I don't insist though, I'm
>> admittedly rusty on this code.
>>
>>
>> (2) The proposed patch sets "max_size" to ACPI_BUILD_LOADER_MAX_SIZE for
>> each ACPI_BUILD_LOADER_FILE. Makes sense, upon constructing / reviewing
>> the above table.
>>
>> (I'm no longer sure if tweaking the alignment were the preferable path
>> forward.)
>>
>> Either way, I'd request including the above table in the commit message.
>> (Maybe drop the "notes" column.)
>>
>>
>> (3) The above 9 invocations are *all* of the acpi_add_rom_blob()
>> invocations. I find the interface brittle. It's not helpful to have so
>> many macros for the names and the max sizes. We should have a table with
>> three entries and -- minimally -- two columns, specifying name and
>> max_size -- possibly some more call arguments, if such can be extracted.
>> We should also have an enum type for selecting a row in this table, and
>> then acpi_add_rom_blob() should be called with an enum constant.
>>
>> Of course, talk is cheap. :)
>>
>>
>> (4) When do we plan to introduce a nonzero "max_size" for
>> ACPI_BUILD_RSDP_FILE ("etc/acpi/rsdp")?
>>
>> Is the current zero value a time bomb?
> 
> it's not likely to go over 4k, but if we enforce max_size!=0 we may set it 4k,
> which it's aligned to anyways.

Interestingly, the size is not aligned.

We end up calling rom_add_blob() with a size like "22". The memory 
region we create has size=22 / max_size=22. (max_size is not effective, 
we rely on the one from the underlying RAMBlock).

The resizeable RAMBlock, however, aligns both up to full pages (e.g., 4k 
/ 4k), so we can later grow it > 22 bytes.


Doesn't really matter in practice I guess, because we always expose full 
pages to the guest, and migrate full pages.

One corner case could be shrinking e.g., from 22 bytes to 14 bytes. I am 
not sure if we would zero-out these 8 bytes somewhere. Not sure if that 
is of any relevance.

Beautiful code.

-- 
Thanks,

David / dhildenb