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

[Igor Mammedov]

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.


> 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.

main use-case for using acpi_add_rom_blob() is for mutable blobs,
so that all these blobs were transferred during migration to the destination,
to ensure that guest sees consistent data set (from source instead of mix of
source/dst blobs).

Resize came later on, when we got sick of ad-hock (align)/size bumping of
"etc/acpi/tables" in configurations where size was on verge of crossing
border to the next aligned size and related knobs to keep that mess
migratable.

> 
> 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).
> 
> (I haven't thought through what this would mean for migration, forward
> or backward; I'm just brain-storming.)
> 
> Thanks
> Laszlo