Re: [PATCH] fw_cfg: Don't set callback_opaque NULL in fw_cfg_modify_bytes_read()

[Gerd Hoffmann]

> > QEMU_EFI reports the below error:
> > ProcessCmdAddPointer: invalid pointer value in "etc/acpi/tables"
> > OnRootBridgesConnected: InstallAcpiTables: Protocol Error
> > 
> > Debugging shows that on first reboot(after hot-adding NVDIMM),
> > Qemu updates the etc/table-loader len,
> > 
> > qemu_ram_resize()
> >   fw_cfg_modify_file()
> >      fw_cfg_modify_bytes_read()
> > 
> > And in fw_cfg_modify_bytes_read() we set the "callback_opaque" for
> > the "key" entry to NULL. Because of this, on the second reboot,
> > virt_acpi_build_update() is called with a NULL "build_state" and
> > returns without updating the ACPI tables. This seems to be 
> > upsetting the firmware.
> > 
> > To fix this, don't change the callback_opaque in fw_cfg_modify_bytes_read().
> 
> Fixes: bdbb5b1706d165 ("fw_cfg: add fw_cfg_machine_reset function")
> Acked-by: Igor Mammedov <imammedo@redhat.com>
> 
> CCing Gerd to have a second set of eyes on it

Hmm.  Original patch clears both 'callback_opaque' and 'callback' (where
'callback' used to be what 'select_cb' is today I think).  Not fully
sure what the motivation was for that.  Maybe because using both
fw_cfg_modify*() calls and a callback for update-on-read for a given
entry looks pointless.  Should that be the case there are better ways to
catch that, like having fw_cfg_modify_bytes_read() throw an error in
case select_cb is not NULL instead of silently clearing the callback.

In any case clearing callback_opaque only is obviously wrong, so
Acked-by: Gerd Hoffmann <kraxel@redhat.com>

take care,
  Gerd