Re: [PATCH v9 01/12] acpi/ghes: add a firmware file with HEST address

[Mauro Carvalho Chehab]

Em Wed, 11 Sep 2024 15:51:08 +0200
Igor Mammedov <imammedo@redhat.com> escreveu:

> On Sun, 25 Aug 2024 05:45:56 +0200
> Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:
> 
> > Store HEST table address at GPA, placing its content at
> > hest_addr_le variable.
> > 
> > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>  
> 
> This looks good to me.
> 
> in addition to this, it needs a patch on top to make sure
> that we migrate hest_addr_le.
> See a08a64627b6b 'ACPI: Record the Generic Error Status Block address'
> and fixes on top of that for an example.

Hmm... If I understood such change well, vmstate_ghes_state() will
use this structure as basis to do migration:

        /* ghes.h */
        typedef struct AcpiGhesState {
            uint64_t hest_addr_le;
            uint64_t ghes_addr_le;
            bool present; /* True if GHES is present at all on this board */
        } AcpiGhesState;

        /* generic_event_device.c */
        static const VMStateDescription vmstate_ghes_state = {
            .name = "acpi-ged/ghes",
            .version_id = 1,
            .minimum_version_id = 1,
            .needed = ghes_needed,
            .fields      = (VMStateField[]) {
                VMSTATE_STRUCT(ghes_state, AcpiGedState, 1,
                               vmstate_ghes_state, AcpiGhesState),
                VMSTATE_END_OF_LIST()
            }
        };

        /* hw/arm/virt-acpi-build.c */
        void virt_acpi_setup(VirtMachineState *vms)
        {
            ...
            if (vms->ras) {
                assert(vms->acpi_dev);
                acpi_ged_state = ACPI_GED(vms->acpi_dev);
                acpi_ghes_add_fw_cfg(&acpi_ged_state->ghes_state,
                                     vms->fw_cfg, tables.hardware_errors);
            }

Which relies on acpi_ghes_add_fw_cfg() function to setup callbacks for
the migration:

        /* ghes.c */
        void acpi_ghes_add_fw_cfg(AcpiGhesState *ags, FWCfgState *s,
                                  GArray *hardware_error)
        {
            /* Create a read-only fw_cfg file for GHES */
            fw_cfg_add_file(s, ACPI_HW_ERROR_FW_CFG_FILE, hardware_error->data,
                            hardware_error->len);

            /* Create a read-write fw_cfg file for Address */
            fw_cfg_add_file_callback(s, ACPI_HW_ERROR_ADDR_FW_CFG_FILE, NULL, 
NULL,
                NULL, &(ags->ghes_addr_le), sizeof(ags->ghes_addr_le), false);

            fw_cfg_add_file_callback(s, ACPI_HEST_ADDR_FW_CFG_FILE, NULL, NULL,
                NULL, &(ags->hest_addr_le), sizeof(ags->hest_addr_le), false);

            ags->present = true;
        }

It sounds to me that both ghes_addr_le and hest_addr_le will be migrated
altogether.

Did I miss something?

Thanks,
Mauro