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

[Michael S. Tsirkin]

On Wed, Mar 03, 2021 at 11:21:45AM -0500, Michael S. Tsirkin wrote:
> 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.

Hmm I tried and I take it back. There are just 5 tables that are
immuatable. Not sure it's worth the trouble.


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