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

[Michael S. Tsirkin]

On Wed, Mar 03, 2021 at 05:09:16PM +0100, 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.

Right. BTW there is an alternative I did not think of earlier.

Lots of tables are actually fixed. We currently let guest calculate
the checksum for all tables but that is not a must. We could prefill the
checksum for most of them and cut the size by almost half.

This fixes the issues in a way that seems cleaner to me as
it migrates both ways for all configs and saves some resources.
I'm not against making it resizeable too though.


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