Re: Invalid blob size on NVDIMM hot-add

[Laszlo Ersek]

On 09/26/19 13:37, Shameerali Kolothum Thodi wrote:

>>> Note: *all* relevant fw_cfg files must have their "size" fields updated
>>> in the directory blob (FW_CFG_FILE_DIR). I.e. the requirement applies to
>>> both the linker-loader script, and to all blobs that are referenced (by
>>> name) by the commands in the linker-loader script.
>>>
>>>
>>> (b) The firmware to re-read the size from the directory, after selecting
>>> the key for the sake of ACPI regeneration.
>>
>> Hmm...I am not sure this is required. In my testing with the above fix I
>> mentioned,
>> I can see that firmware is reading the correct(modified) size on select.
>> I will double check this though.
> 
> Hi Laszlo,
> 
> Right. I think the reason it works for me without your patch is when firmware
> selects the first item("etc/table-loader"), the Qemu side runs the callback
> and try to update all the acpi ram sizes including " etc/acpi/tables" which 
> is the 
> one that matters in this specific case.

This is a good explanation.

Currently, the *only* action that occurs in the firmware before
selecting "etc/table-loader", is fetching the selector key, and size, of
"etc/table-loader".

Therefore, if the ACPI re-generation does not affect the size of
"etc/table-loader", just the size of "etc/acpi/tables" -- and QEMU
updates the size in the fw_cfg directory for "etc/acpi/tables" --, then
there is indeed no need for updating the firmware.

I was just not sure if QEMU could guarantee this invariant (i.e. that
the size of "etc/table-loader" would not change). After all, if you
modify the ACPI tables in the "etc/acpi/tables" blob, possibly even add
new tables to it, then you will likely have to append new *commands*,
for those additional ACPI objects / tables, to the linker-loader script
in "etc/table-loader".

> So with my fix with the tables_blob size align, qemu_ram_resize() calls the
> fw_cfg_modify_file() and updates the " etc/acpi/tables" size when firmware
> selects the " etc/table-loader" item.

Right, that's good.

> But I think, your below patch is still required in case " etc/table-loader" is
> changed by Qemu.

Exactly.

It really depends on whether the platform devices hot-plugged before
platform reset require new commands in the linker/loader script.

Thanks
Laszlo