RE: Invalid blob size on NVDIMM hot-add (was: RE: [RFC PATCH 0/4] ARM virt: ACPI memory hotplug support)

[Shameerali Kolothum Thodi]

> -----Original Message-----
> From: Shameerali Kolothum Thodi
> Sent: 24 September 2019 17:39
> To: 'Laszlo Ersek' <address@hidden>; Igor Mammedov
> <address@hidden>
> Cc: Auger Eric <address@hidden>; address@hidden;
> address@hidden; address@hidden; address@hidden;
> xuwei (O) <address@hidden>; Linuxarm <address@hidden>; Ard
> Biesheuvel <address@hidden>; Leif Lindholm (Linaro address)
> <address@hidden>
> Subject: RE: Invalid blob size on NVDIMM hot-add (was: RE: [RFC PATCH 0/4]
> ARM virt: ACPI memory hotplug support)
 
[...]
 
> 
> > >>>> How about this.
> > >>>>
> > >>>> (1) The firmware looks up the fw_cfg file called "etc/table-loader"
> > >>>> in the fw_cfg file directory (identified by constant selector key
> > >>>> 0x0019, FW_CFG_FILE_DIR).
> > >>>>
> > >>>> (2) The directory entry, once found, tells the firmware two things
> > >>>> simultaneously. The selector key, and the size of the blob.
> > >>>>
> > >>>> (3) The firmware selects the selector key from step (2).
> > >>>>
> > >>>> (4) QEMU regenerates the ACPI payload (as a select callback).
> > >>>>
> > >>>> (5) The firmware reads the number of bytes from the fw_cfg blob
> > >>>> that it learned in step (2).
> > >>>>
> > >>>> Here's the problem. As long as QEMU used to perform step (4) only
> > >>>> for the purpose of refreshing PCI resources in the ACPI payload,
> > >>>> step (4) wouldn't *resize* the blob.
> > >>>>
> > >>>> However, if step (4) enlarges the blob, then the byte count that
> > >>>> step (5) uses -- from step (2) -- for reading, is obsolete.
> > >>
> > >>> I've thought that was a problem with IO based fw_cfg, as reading
> > >>> size/content were separates steps and that it was solved by DMA
> > >>> based fw_cfg file read.
> > >>
> > >> The DMA backend is not relevant for this question, for two reasons:
> > >>
> > >> (a) The question whether the fw_cfg transfer takes places with port
> > >> IO vs. DMA is hidden from the fw_cfg client code; that code goes
> > >> through an abstract library API.
> > >>
> > >> (b) While the DMA method indeed lets the firmware specify the details
> > >> of the transfer with one action, the issue is with the number of
> > >> bytes that the firmware requests (that is, not with *how* the
> > >> firmware requests the transfer). The firmware has to know the size of
> > >> the transfer before it can initiate the transfer (regardless of port
> > >> IO vs. DMA).
> > >>
> > >>
> > >> My question is: assume the firmware item in question is selected, and
> > >> the QEMU-side select callback runs (regenerating the ACPI payload).
> > >> Does this action update the blob size in the fw_cfg file directory as
> > >> well?
> > >
> > > I think it doesn't update the blob size on select callback which is
> > > the root cause of this issue. And the reason looks like,
> > > qemu_ram_resize() function returns without invoking the callback to
> > > update the blob size.
> > >
> > > On boot up, Qemu builds the table and exposes it to guest,
> > >       virt_acpi_setup()
> > >         acpi_add_rom_blob()
> > >           rom_add_blob()
> > >             rom_set_mr()  --> mr is allocated here and ram_block
> > used_length = HOST_PAGE_ALIGN(blob size);
> > >             fw_cfg_add_file_callback()
> > >               fw_cfg_add_bytes_callback() --> This uses the blob size
> > passed into it.
> > >
> > > On select callback path,
> > >
> > > virt_acpi_build_update()
> > >    acpi_ram_update()
> > >     memory_region_ram_resize()
> > >       qemu_ram_resize() -->. Here the newsize gets aligned to
> HOST_PAGE
> > and callback is only called used_length != newsize.
> > >
> > > https://github.com/qemu/qemu/blob/master/exec.c#L2180
> > >
> > > Debug logs:
> > > Initial boot:
> > > ##QEMU_DEBUG## rom_add_blob: file etc/acpi/tables size 0x64f7
> > > ##QEMU_DEBUG## fw_cfg_add_bytes_callback: key 0x21 len 0x64f7
> > > ........
> > > ........
> > > ###UEFI#### InstallQemuFwCfgTables: "etc/table-loader" has FwCfgItem
> > 0x27 size 0xD00
> > > ##QEMU_DEBUG## virt_acpi_build_update:
> > > ##QEMU_DEBUG## acpi_ram_update: size 0x64f7
> > > ##QEMU_DEBUG## qemu_ram_resize: idstr /rom@etc/acpi/tables
> > used_length  0x7000 newsize 0x7000 --> No callback.
> > > .....
> > > ######UEFI###### ProcessCmdAllocate:
> > QemuFwCfgFindFile("etc/acpi/tables"): size 0x64F7 --> UEFI get the actual
> size,
> > which is fine for now.
> > >
> > > Hot-add nvdimms and reboot.
> > >
> > > root@ubuntu:/# reboot
> > > .......
> > > ........
> > > ###UEFI#### InstallQemuFwCfgTables: "etc/table-loader" has FwCfgItem
> > 0x27 size 0xD00
> > > ##QEMU_DEBUG## virt_acpi_build_update:
> > > ##QEMU_DEBUG## acpi_ram_update: size 0x6667 --> Size changed.
> > > ##QEMU_DEBUG## qemu_ram_resize: idstr /rom@etc/acpi/tables
> > used_length  0x7000 newsize 0x7000 --> newsize is still aligned to 0x7000
> and
> > no callback to update.
> > > ......
> > > ######UEFI###### ProcessCmdAllocate:
> > QemuFwCfgFindFile("etc/acpi/tables"): size 0x64F7 -->UEFI still sees the old
> > value and fails.
> > >
> > > This can be fixed by,
> > >
> > > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c index
> > f3bd45675b..79da3fd35d 100644
> > > --- a/hw/arm/virt-acpi-build.c
> > > +++ b/hw/arm/virt-acpi-build.c
> > > @@ -854,6 +854,9 @@ void virt_acpi_build(VirtMachineState *vms,
> > AcpiBuildTables *tables)
> > >          build_rsdp(tables->rsdp, tables->linker, &rsdp_data);
> > >      }
> > >
> > > +    g_array_set_size(tables_blob,
> > > +
> > TARGET_PAGE_ALIGN(acpi_data_len(tables_blob)));
> > > +
> > >      /* Cleanup memory that's no longer used. */
> > >      g_array_free(table_offsets, true);
> > >  }
> > >
> > > But I am not sure this is the best to way fix this issue (Or I am
> > > missing something here).
> > >
> > > Please let me know.
> >
> > The above QEMU patch, for virt_acpi_build(), may be necessary, but I
> > don't think it is sufficient.
> >
> > For the firmware to see the updated (enlarged) blob, two things are
> > required:
> >
> > (a) QEMU to update the blob size in the fw_cfg directory entry.
> >
> > Note: to the firmware, it is totally irrelevant if QEMU updates some
> > *other* value or field that reflects the fresh blob size. The only thing
> > the firmware can see is the entry in the FW_CFG_FILE_DIR blob.
> >
> > To illustrate the field I'm referring to, see:
> >
> >     s->files->f[index].size   = cpu_to_be32(len);
> >
> > in fw_cfg_add_file_callback().
> >
> > See also:
> >
> >             s->files->f[i].size   = cpu_to_be32(len);
> >
> > in fw_cfg_modify_file().
> >
> > That "size" field is what the firmware can see.
> 
> Agree. And what I am trying to illustrate above is why this update is not
> happening.
> 
> The Qemu path on select key from firmware is(I think),
> 
> fw_cfg_select()
>  e->select_cb() -->Callback registered in virt_acpi_setup() -->
> acpi_add_rom_blob() ..... -> fw_cfg_add_file_callback()
>   virt_acpi_build_update()
>    acpi_ram_update()
>     memory_region_ram_resize()
>      qemu_ram_resize()
>       block->resized()  --> Callback registered in virt_acpi_setup() ->
> acpi_add_rom_blob()-> rom_add_blob() -> rom_set_mr()
>        fw_cfg_resized()
>         fw_cfg_modify_file()
>          s->files->f[i].size  = cpu_to_be32(len); Updates the size for
> firmware
> 
> But as I was trying to explain above, the callback function fw_cfg_resized() 
> is
> not
> invoked in qemu_ram_resize()because while adding the mr
> region(rom_set_mr()),
> it uses the aligned size. And as a result if the updated/modified blob size 
> is not
> greater
> than the aligned size the qemu_ram_resize() returns immediately.
> 
> int qemu_ram_resize(RAMBlock *block, ram_addr_t newsize, Error **errp)
> {
>     assert(block);
> 
>       newsize = HOST_PAGE_ALIGN(newsize);
> 
>     if (block->used_length == newsize) {
>         return 0;
>     }
> ...
> 
>     if (block->resized) {
>         block->resized(block->idstr, newsize, block->host);  ---> registered
> callback, fw_cfg_resized()
>     }
>     return 0;
> }

Hi Igor,

Could you please take a look at the above Qemu side problem I am trying to
explain here. Please let me if it is not clear or more details are required.

> > 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 the log I have with prints added to both firmware and Qemu,

OnRootBridgesConnected: root bridges have been connected, installing ACPI tables
#UEFI# InstallQemuFwCfgTables: Find file "etc/table-loader"
#UEFI# QemuFwCfgFindFile:  Select QemuFwCfgItemFileDir for etc/table-loader
#UEFI# QemuFwCfgSelectItem: Select Item 0x19
#QEMU# fw_cfg_select: key 0x19 No callback
#UEFI# QemuFwCfgFindFile:  FileSize 0xD00 FileSelect 0x27 FName etc/table-loader
#UEFI# QemuFwCfgSelectItem: Select Item 0x27
#QEMU# fw_cfg_select: key 0x27 Invoking callback...
#QEMU# virt_acpi_build_update: Updating ACPI tables...
#QEMU# acpi_ram_update: mr name /rom@etc/acpi/tables size 0x64f7
#QEMU# qemu_ram_resize: newsize 0x7000(used_length == newsize). Returning
#QEMU# acpi_ram_update: mr name /rom@etc/acpi/rsdp size 0x24
#QEMU# qemu_ram_resize: newsize 0x1000(used_length == newsize). Returning
#QEMU# acpi_ram_update: mr name /rom@etc/table-loader size 0xd00
#QEMU# qemu_ram_resize: newsize 0x1000(used_length == newsize). Returning
#UEFI# QemuFwCfgFindFile:  Select QemuFwCfgItemFileDir for etc/acpi/rsdp
#UEFI# QemuFwCfgSelectItem: Select Item 0x19
#QEMU# fw_cfg_select: key 0x19 No callback
#UEFI# QemuFwCfgFindFile:  FileSize 0x24 FileSelect 0x22 FName etc/acpi/rsdp
#UEFI# QemuFwCfgSelectItem: Select Item 0x22
#QEMU# fw_cfg_select: key 0x22 Invoking callback...
#QEMU# virt_acpi_build_update: No update required
.....
#UEFI# QemuFwCfgFindFile:  Select QemuFwCfgItemFileDir for etc/acpi/tables
#UEFI# QemuFwCfgSelectItem: Select Item 0x19
#QEMU# fw_cfg_select: key 0x19 No callback
#UEFI# QemuFwCfgFindFile:  FileSize 0x64F7 FileSelect 0x23 FName etc/acpi/tables
#UEFI# QemuFwCfgSelectItem: Select Item 0x23
#QEMU# fw_cfg_select: key 0x23 Invoking callback...
#QEMU# virt_acpi_build_update: No update required
InstallQemuFwCfgTables: installed 9 tables

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.

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

Thanks,
Shameer

> Thanks,
> Shameer
> 
> > I wrote:
> >
> > >> If it does, then I can work around the problem in the firmware. I can
> > >> add a re-lookup to the code after the item selection, in order to get
> > >> the fresh blob size from the fw_cfg file directory. Then we can use
> > >> that size for the actual transfer.
> > >>
> > >> This won't help old firmware on new QEMU, but at least new firmware
> > >> on old QEMU will not be hurt (the re-fetching of the fw_cfg file
> > >> directory will come with a small performance penalty, but
> > >> functionally it will be a no-op).
> >
> > Thus, the firmware patch in question would be:
> >
> > | diff --git a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
> > b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
> > | index bc1a891dbaf1..07f70ffe158a 100644
> > | --- a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
> > | +++ b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
> > | @@ -975,6 +975,24 @@ InstallQemuFwCfgTables (
> > |    ORDERED_COLLECTION       *SeenPointers;
> > |    ORDERED_COLLECTION_ENTRY *SeenPointerEntry,
> *SeenPointerEntry2;
> > |
> > | +  Status = QemuFwCfgFindFile ("etc/table-loader", &FwCfgItem,
> > &FwCfgSize);
> > | +  if (EFI_ERROR (Status)) {
> > | +    return Status;
> > | +  }
> > | +
> > | +  //
> > | +  // By selecting "FwCfgItem", ask QEMU to regenerate the ACPI payload,
> > with
> > | +  // all PCI devices decoding their resources. Note: further selections
> > | +  // of the same key will not repeat the patching.
> > | +  //
> > | +  EnablePciDecoding (&OriginalPciAttributes,
> &OriginalPciAttributesCount);
> > | +  QemuFwCfgSelectItem (FwCfgItem);
> > | +  RestorePciDecoding (OriginalPciAttributes, OriginalPciAttributesCount);
> > | +
> > | +  //
> > | +  // The size of the script may have changed, possibly due to platform
> > devices
> > | +  // having been hot-plugged before platform reset. Re-read the size.
> > | +  //
> > |    Status = QemuFwCfgFindFile ("etc/table-loader", &FwCfgItem,
> > &FwCfgSize);
> > |    if (EFI_ERROR (Status)) {
> > |      return Status;
> > | @@ -989,10 +1007,8 @@ InstallQemuFwCfgTables (
> > |    if (LoaderStart == NULL) {
> > |      return EFI_OUT_OF_RESOURCES;
> > |    }
> > | -  EnablePciDecoding (&OriginalPciAttributes,
> &OriginalPciAttributesCount);
> > |    QemuFwCfgSelectItem (FwCfgItem);
> > |    QemuFwCfgReadBytes (FwCfgSize, LoaderStart);
> > | -  RestorePciDecoding (OriginalPciAttributes, OriginalPciAttributesCount);
> > |    LoaderEnd = LoaderStart + FwCfgSize / sizeof *LoaderEntry;
> > |
> > |    AllocationsRestrictedTo32Bit = NULL;
> >
> > But, again, this only makes sense if QEMU implements (a).
> >
> > Thanks
> > Laszlo