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

[Laszlo Ersek]

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

(I haven't thought through what this would mean for migration, forward
or backward; I'm just brain-storming.)

Thanks
Laszlo