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.