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

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

Posted by Shameer Kolothum via 1 year, 2 months ago

On arm/virt platform, Chen Xiang reported a Guest crash while
attempting the below steps,

1. Launch the Guest with nvdimm=on
2. Hot-add a NVDIMM dev
3. Reboot
4. Guest boots fine.
5. Reboot again.
6. Guest boot fails.

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")
Reported-by: chenxiang <chenxiang66@hisilicon.com>
Acked-by: Igor Mammedov <imammedo@redhat.com>
Acked-by: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
---
Hi,

I forgot to follow-up on the v2 and it never got picked up.
Thanks to Wangzhou who recently re-run the tests and found that
the problem mentioned above still exists. Hence resending the v2.

v2-->v3:
 -Just rebase.

v2: https://lore.kernel.org/qemu-devel/20220908160354.2023-1-shameerali.kolothum.thodi@huawei.com/
v1: https://lore.kernel.org/all/20220825161842.841-1-shameerali.kolothum.thodi@huawei.com/

Thanks,
Shameer
---
 hw/nvram/fw_cfg.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index b644577734..74edb1e4cf 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -730,7 +730,6 @@ static void *fw_cfg_modify_bytes_read(FWCfgState *s, uint16_t key,
     ptr = s->entries[arch][key].data;
     s->entries[arch][key].data = data;
     s->entries[arch][key].len = len;
-    s->entries[arch][key].callback_opaque = NULL;
     s->entries[arch][key].allow_write = false;
 
     return ptr;
-- 
2.34.1

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

Posted by Philippe Mathieu-Daudé 1 year, 1 month ago

On 3/12/24 14:18, Shameer Kolothum wrote:
> On arm/virt platform, Chen Xiang reported a Guest crash while
> attempting the below steps,
> 
> 1. Launch the Guest with nvdimm=on
> 2. Hot-add a NVDIMM dev
> 3. Reboot
> 4. Guest boots fine.
> 5. Reboot again.
> 6. Guest boot fails.
> 
> 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")
> Reported-by: chenxiang <chenxiang66@hisilicon.com>
> Acked-by: Igor Mammedov <imammedo@redhat.com>
> Acked-by: Gerd Hoffmann <kraxel@redhat.com>
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> ---
> Hi,
> 
> I forgot to follow-up on the v2 and it never got picked up.
> Thanks to Wangzhou who recently re-run the tests and found that
> the problem mentioned above still exists. Hence resending the v2.
> 
> v2-->v3:
>   -Just rebase.
> 
> v2: https://lore.kernel.org/qemu-devel/20220908160354.2023-1-shameerali.kolothum.thodi@huawei.com/
> v1: https://lore.kernel.org/all/20220825161842.841-1-shameerali.kolothum.thodi@huawei.com/
> 
> Thanks,
> Shameer
> ---
>   hw/nvram/fw_cfg.c | 1 -
>   1 file changed, 1 deletion(-)

Patch queued, thanks!

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

Posted by Shameerali Kolothum Thodi via 1 year, 2 months ago

Hi,

A gentle ping on this.

Hope the fix here is still valid and can be picked up. 
Not sure by whom this will get picked up though.

@Gerd?

Thanks,
Shameer

> -----Original Message-----
> From: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> Sent: Tuesday, December 3, 2024 1:18 PM
> To: qemu-devel@nongnu.org
> Cc: philmd@linaro.org; kraxel@redhat.com; imammedo@redhat.com;
> wangzhou1@hisilicon.com; linuxarm@huawei.com
> Subject: [PATCH v3] fw_cfg: Don't set callback_opaque NULL in
> fw_cfg_modify_bytes_read()
> 
> On arm/virt platform, Chen Xiang reported a Guest crash while
> attempting the below steps,
> 
> 1. Launch the Guest with nvdimm=on
> 2. Hot-add a NVDIMM dev
> 3. Reboot
> 4. Guest boots fine.
> 5. Reboot again.
> 6. Guest boot fails.
> 
> 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")
> Reported-by: chenxiang <chenxiang66@hisilicon.com>
> Acked-by: Igor Mammedov <imammedo@redhat.com>
> Acked-by: Gerd Hoffmann <kraxel@redhat.com>
> Signed-off-by: Shameer Kolothum
> <shameerali.kolothum.thodi@huawei.com>
> ---
> Hi,
> 
> I forgot to follow-up on the v2 and it never got picked up.
> Thanks to Wangzhou who recently re-run the tests and found that
> the problem mentioned above still exists. Hence resending the v2.
> 
> v2-->v3:
>  -Just rebase.
> 
> v2: https://lore.kernel.org/qemu-devel/20220908160354.2023-1-
> shameerali.kolothum.thodi@huawei.com/
> v1: https://lore.kernel.org/all/20220825161842.841-1-
> shameerali.kolothum.thodi@huawei.com/
> 
> Thanks,
> Shameer
> ---
>  hw/nvram/fw_cfg.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index b644577734..74edb1e4cf 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -730,7 +730,6 @@ static void *fw_cfg_modify_bytes_read(FWCfgState
> *s, uint16_t key,
>      ptr = s->entries[arch][key].data;
>      s->entries[arch][key].data = data;
>      s->entries[arch][key].len = len;
> -    s->entries[arch][key].callback_opaque = NULL;
>      s->entries[arch][key].allow_write = false;
> 
>      return ptr;
> --
> 2.34.1

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

Posted by Igor Mammedov 1 year, 1 month ago

On Wed, 11 Dec 2024 09:15:53 +0000
Shameerali Kolothum Thodi via <qemu-devel@nongnu.org> wrote:

> Hi,
> 
> A gentle ping on this.
> 
> Hope the fix here is still valid and can be picked up. 
> Not sure by whom this will get picked up though.

Perhaps Michael an pik it up, CCed.

> 
> @Gerd?
> 
> Thanks,
> Shameer
> 
> > -----Original Message-----
> > From: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> > Sent: Tuesday, December 3, 2024 1:18 PM
> > To: qemu-devel@nongnu.org
> > Cc: philmd@linaro.org; kraxel@redhat.com; imammedo@redhat.com;
> > wangzhou1@hisilicon.com; linuxarm@huawei.com
> > Subject: [PATCH v3] fw_cfg: Don't set callback_opaque NULL in
> > fw_cfg_modify_bytes_read()
> > 
> > On arm/virt platform, Chen Xiang reported a Guest crash while
> > attempting the below steps,
> > 
> > 1. Launch the Guest with nvdimm=on
> > 2. Hot-add a NVDIMM dev
> > 3. Reboot
> > 4. Guest boots fine.
> > 5. Reboot again.
> > 6. Guest boot fails.
> > 
> > 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")
> > Reported-by: chenxiang <chenxiang66@hisilicon.com>
> > Acked-by: Igor Mammedov <imammedo@redhat.com>
> > Acked-by: Gerd Hoffmann <kraxel@redhat.com>
> > Signed-off-by: Shameer Kolothum
> > <shameerali.kolothum.thodi@huawei.com>
> > ---
> > Hi,
> > 
> > I forgot to follow-up on the v2 and it never got picked up.
> > Thanks to Wangzhou who recently re-run the tests and found that
> > the problem mentioned above still exists. Hence resending the v2.
> > 
> > v2-->v3:
> >  -Just rebase.
> > 
> > v2: https://lore.kernel.org/qemu-devel/20220908160354.2023-1-
> > shameerali.kolothum.thodi@huawei.com/
> > v1: https://lore.kernel.org/all/20220825161842.841-1-
> > shameerali.kolothum.thodi@huawei.com/
> > 
> > Thanks,
> > Shameer
> > ---
> >  hw/nvram/fw_cfg.c | 1 -
> >  1 file changed, 1 deletion(-)
> > 
> > diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> > index b644577734..74edb1e4cf 100644
> > --- a/hw/nvram/fw_cfg.c
> > +++ b/hw/nvram/fw_cfg.c
> > @@ -730,7 +730,6 @@ static void *fw_cfg_modify_bytes_read(FWCfgState
> > *s, uint16_t key,
> >      ptr = s->entries[arch][key].data;
> >      s->entries[arch][key].data = data;
> >      s->entries[arch][key].len = len;
> > -    s->entries[arch][key].callback_opaque = NULL;
> >      s->entries[arch][key].allow_write = false;
> > 
> >      return ptr;
> > --
> > 2.34.1  
>