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

[David Hildenbrand <david@redhat.com>]

On 03.03.21 16:03, Laszlo Ersek 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...)
> 
> 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?
> 
> Put differently: acpi_add_rom_blob() should be *impossible* to call with
> "max_size=0", arguably. *Whenever* we call acpi_add_rom_blob(), we do
> that because the blob is resizable (mutable) -- but that also means we
> should have a safety margin, does it not? So calling acpi_add_rom_blob()
> with "max_size=0" looks self-contradictory.
> 
> FWIW, this could be covered by the table proposed in point (3).
> 
> 
> In total, I don't disagree with the patch (beyond the fact that the new
> macro's value doesn't match the commit message), functionally speaking.
> However, wrt. readability, I think the patch further complicates the
> code. I'd suggest five patches:
> 
> #1 -- use "etc/table-loader" via the proper macro name in "microvm",
> 
> #2 -- rework acpi_add_rom_blob() for using a table of constants + an
>        enum type,
> 
> #3 -- bump the "max_size" field for ACPI_BUILD_LOADER_FILE, for the
>        current symptom,
> 
> #4 -- set a nonzero "max_size" for the remaining ACPI_BUILD_RSDP_FILE,
>        for "future-proofing",
> 
> #5 -- in the new acpi_add_rom_blob() implementation, taking the enum,
>        assert(max_size != 0).
> 

Mostly sounds sane to me, however, I'm leaning towards putting the real 
fix upfront (so we e.g., can easily backport to stable) and doing all 
the refactorings on top.

I think we have an agreement that the current approach is the right one. 
I'll look into it today. Thanks!

-- 
Thanks,

David / dhildenb