| Message ID | 1578483143-14905-6-git-send-email-gengdongjiu@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add ARMv8 RAS virtualization support in QEMU | expand |
On Wed, 8 Jan 2020 at 11:33, Dongjiu Geng <gengdongjiu@huawei.com> wrote: > > Record the GHEB address via fw_cfg file, when recording > a error to CPER, it will use this address to find out > Generic Error Data Entries and write the error. > > Make the HEST GHES to a GED device. > > Signed-off-by: Dongjiu Geng <gengdongjiu@huawei.com> > Signed-off-by: Xiang Zheng <zhengxiang9@huawei.com> > --- > hw/acpi/generic_event_device.c | 15 ++++++++++++++- > hw/acpi/ghes.c | 16 ++++++++++++++++ > hw/arm/virt-acpi-build.c | 13 ++++++++++++- > include/hw/acpi/generic_event_device.h | 2 ++ > include/hw/acpi/ghes.h | 6 ++++++ > 5 files changed, 50 insertions(+), 2 deletions(-) > > diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c > index 9cee90c..9bf37e4 100644 > --- a/hw/acpi/generic_event_device.c > +++ b/hw/acpi/generic_event_device.c > @@ -234,12 +234,25 @@ static const VMStateDescription vmstate_ged_state = { > } > }; > > +static const VMStateDescription vmstate_ghes_state = { > + .name = "acpi-ghes-state", > + .version_id = 1, > + .minimum_version_id = 1, > + .fields = (VMStateField[]) { > + VMSTATE_UINT64(ghes_addr_le, AcpiGhesState), > + VMSTATE_END_OF_LIST() > + } > +}; > + > static const VMStateDescription vmstate_acpi_ged = { > .name = "acpi-ged", > .version_id = 1, > .minimum_version_id = 1, > .fields = (VMStateField[]) { > - VMSTATE_STRUCT(ged_state, AcpiGedState, 1, vmstate_ged_state, GEDState), > + VMSTATE_STRUCT(ged_state, AcpiGedState, 1, > + vmstate_ged_state, GEDState), > + VMSTATE_STRUCT(ghes_state, AcpiGedState, 1, > + vmstate_ghes_state, AcpiGhesState), > VMSTATE_END_OF_LIST(), > }, > .subsections = (const VMStateDescription * []) { You can't just add fields to an existing VMStateDescription like this -- it will break migration compatibility. Instead you need to add a new subsection to this vmstate, with a '.needed' function which indicates when the subsection should be present. thanks -- PMM
On 1/8/20 12:32 PM, Dongjiu Geng wrote: > Record the GHEB address via fw_cfg file, when recording > a error to CPER, it will use this address to find out > Generic Error Data Entries and write the error. > > Make the HEST GHES to a GED device. > > Signed-off-by: Dongjiu Geng <gengdongjiu@huawei.com> > Signed-off-by: Xiang Zheng <zhengxiang9@huawei.com> > --- > hw/acpi/generic_event_device.c | 15 ++++++++++++++- > hw/acpi/ghes.c | 16 ++++++++++++++++ > hw/arm/virt-acpi-build.c | 13 ++++++++++++- > include/hw/acpi/generic_event_device.h | 2 ++ > include/hw/acpi/ghes.h | 6 ++++++ > 5 files changed, 50 insertions(+), 2 deletions(-) > > diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c > index 9cee90c..9bf37e4 100644 > --- a/hw/acpi/generic_event_device.c > +++ b/hw/acpi/generic_event_device.c > @@ -234,12 +234,25 @@ static const VMStateDescription vmstate_ged_state = { > } > }; > > +static const VMStateDescription vmstate_ghes_state = { > + .name = "acpi-ghes-state", > + .version_id = 1, > + .minimum_version_id = 1, > + .fields = (VMStateField[]) { > + VMSTATE_UINT64(ghes_addr_le, AcpiGhesState), > + VMSTATE_END_OF_LIST() > + } > +}; > + > static const VMStateDescription vmstate_acpi_ged = { > .name = "acpi-ged", > .version_id = 1, > .minimum_version_id = 1, > .fields = (VMStateField[]) { > - VMSTATE_STRUCT(ged_state, AcpiGedState, 1, vmstate_ged_state, GEDState), > + VMSTATE_STRUCT(ged_state, AcpiGedState, 1, > + vmstate_ged_state, GEDState), > + VMSTATE_STRUCT(ghes_state, AcpiGedState, 1, > + vmstate_ghes_state, AcpiGhesState), > VMSTATE_END_OF_LIST(), > }, > .subsections = (const VMStateDescription * []) { > diff --git a/hw/acpi/ghes.c b/hw/acpi/ghes.c > index 9d37798..68f4abf 100644 > --- a/hw/acpi/ghes.c > +++ b/hw/acpi/ghes.c > @@ -23,6 +23,7 @@ > #include "hw/acpi/acpi.h" > #include "hw/acpi/ghes.h" > #include "hw/acpi/aml-build.h" > +#include "hw/acpi/generic_event_device.h" > #include "hw/nvram/fw_cfg.h" > #include "sysemu/sysemu.h" > #include "qemu/error-report.h" > @@ -208,3 +209,18 @@ void acpi_build_hest(GArray *table_data, GArray *hardware_errors, > build_header(linker, table_data, (void *)(table_data->data + hest_start), > "HEST", table_data->len - hest_start, 1, NULL, ""); > } > + > +void acpi_ghes_add_fw_cfg(AcpiGhesState *ags, FWCfgState *s, > + GArray *hardware_error) > +{ > + size_t size = 2 * sizeof(uint64_t) + ACPI_GHES_MAX_RAW_DATA_LENGTH; > + size_t request_block_size = ACPI_GHES_ERROR_SOURCE_COUNT * size; > + > + /* Create a read-only fw_cfg file for GHES */ > + fw_cfg_add_file(s, ACPI_GHES_ERRORS_FW_CFG_FILE, hardware_error->data, > + request_block_size); > + > + /* Create a read-write fw_cfg file for Address */ > + fw_cfg_add_file_callback(s, ACPI_GHES_DATA_ADDR_FW_CFG_FILE, NULL, NULL, > + NULL, &(ags->ghes_addr_le), sizeof(ags->ghes_addr_le), false); > +} > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c > index 837bbf9..c8aa94d 100644 > --- a/hw/arm/virt-acpi-build.c > +++ b/hw/arm/virt-acpi-build.c > @@ -797,6 +797,7 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables) > unsigned dsdt, xsdt; > GArray *tables_blob = tables->table_data; > MachineState *ms = MACHINE(vms); > + AcpiGedState *acpi_ged_state; > > table_offsets = g_array_new(false, true /* clear */, > sizeof(uint32_t)); > @@ -831,7 +832,9 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables) > acpi_add_table(table_offsets, tables_blob); > build_spcr(tables_blob, tables->linker, vms); > > - if (vms->ras) { > + acpi_ged_state = ACPI_GED(object_resolve_path_type("", TYPE_ACPI_GED, > + NULL)); Testing vms->ras first is cheaper than calling object_resolve_path_type(). Since some people are spending lot of time to reduce VM boot time, it might be worth considering. > + if (acpi_ged_state && vms->ras) { > acpi_add_table(table_offsets, tables_blob); > build_ghes_error_table(tables->hardware_errors, tables->linker); > acpi_build_hest(tables_blob, tables->hardware_errors, > @@ -925,6 +928,7 @@ void virt_acpi_setup(VirtMachineState *vms) > { > AcpiBuildTables tables; > AcpiBuildState *build_state; > + AcpiGedState *acpi_ged_state; > > if (!vms->fw_cfg) { > trace_virt_acpi_setup(); > @@ -955,6 +959,13 @@ void virt_acpi_setup(VirtMachineState *vms) > fw_cfg_add_file(vms->fw_cfg, ACPI_BUILD_TPMLOG_FILE, tables.tcpalog->data, > acpi_data_len(tables.tcpalog)); > > + acpi_ged_state = ACPI_GED(object_resolve_path_type("", TYPE_ACPI_GED, > + NULL)); > + if (acpi_ged_state && vms->ras) { > + acpi_ghes_add_fw_cfg(&acpi_ged_state->ghes_state, > + vms->fw_cfg, tables.hardware_errors); > + } > + > build_state->rsdp_mr = acpi_add_rom_blob(virt_acpi_build_update, > build_state, tables.rsdp, > ACPI_BUILD_RSDP_FILE, 0); > diff --git a/include/hw/acpi/generic_event_device.h b/include/hw/acpi/generic_event_device.h > index d157eac..037d2b5 100644 > --- a/include/hw/acpi/generic_event_device.h > +++ b/include/hw/acpi/generic_event_device.h > @@ -61,6 +61,7 @@ > > #include "hw/sysbus.h" > #include "hw/acpi/memory_hotplug.h" > +#include "hw/acpi/ghes.h" > > #define ACPI_POWER_BUTTON_DEVICE "PWRB" > > @@ -95,6 +96,7 @@ typedef struct AcpiGedState { > GEDState ged_state; > uint32_t ged_event_bitmap; > qemu_irq irq; > + AcpiGhesState ghes_state; > } AcpiGedState; > > void build_ged_aml(Aml *table, const char* name, HotplugHandler *hotplug_dev, > diff --git a/include/hw/acpi/ghes.h b/include/hw/acpi/ghes.h > index 09a7f86..a6761e6 100644 > --- a/include/hw/acpi/ghes.h > +++ b/include/hw/acpi/ghes.h > @@ -60,7 +60,13 @@ enum { > ACPI_HEST_SRC_ID_RESERVED, > }; > > +typedef struct AcpiGhesState { > + uint64_t ghes_addr_le; > +} AcpiGhesState; > + > void build_ghes_error_table(GArray *hardware_errors, BIOSLinker *linker); > void acpi_build_hest(GArray *table_data, GArray *hardware_error, > BIOSLinker *linker); > +void acpi_ghes_add_fw_cfg(AcpiGhesState *vms, FWCfgState *s, > + GArray *hardware_errors); > #endif >
On 2020/1/17 0:44, Peter Maydell wrote: >> .minimum_version_id = 1, >> .fields = (VMStateField[]) { >> - VMSTATE_STRUCT(ged_state, AcpiGedState, 1, vmstate_ged_state, GEDState), >> + VMSTATE_STRUCT(ged_state, AcpiGedState, 1, >> + vmstate_ged_state, GEDState), >> + VMSTATE_STRUCT(ghes_state, AcpiGedState, 1, >> + vmstate_ghes_state, AcpiGhesState), >> VMSTATE_END_OF_LIST(), >> }, >> .subsections = (const VMStateDescription * []) { > You can't just add fields to an existing VMStateDescription > like this -- it will break migration compatibility. Instead you > need to add a new subsection to this vmstate, with a '.needed' > function which indicates when the subsection should be present. Thanks Peter's pointing out. I will change it. > > thanks > -- PMM > . >
On 2020/1/17 15:39, Philippe Mathieu-Daudé wrote: >> table_offsets = g_array_new(false, true /* clear */, >> sizeof(uint32_t)); >> @@ -831,7 +832,9 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables) >> acpi_add_table(table_offsets, tables_blob); >> build_spcr(tables_blob, tables->linker, vms); >> - if (vms->ras) { >> + acpi_ged_state = ACPI_GED(object_resolve_path_type("", TYPE_ACPI_GED, >> + NULL)); > > Testing vms->ras first is cheaper than calling object_resolve_path_type(). Since some people are spending lot of time to reduce VM boot time, it might be worth considering. Thanks Philippe's comments. Do you think it should be written to below[1]? right? [1]: if (vms->ras && acpi_ged_state) > >> + if (acpi_ged_state && vms->ras) { >> acpi_add_table(table_offsets, tables_blob); >> build_ghes_error_table(tables->hardware_errors, tables->linker); >> acpi_build_hest(tables_blob, tables->hardware_errors, >> @@ -925,6 +928,7 @@ void virt_acpi_setup(VirtMachineState *vms) >> {
On 1/17/20 11:47 AM, gengdongjiu wrote: > On 2020/1/17 15:39, Philippe Mathieu-Daudé wrote: >>> table_offsets = g_array_new(false, true /* clear */, >>> sizeof(uint32_t)); >>> @@ -831,7 +832,9 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables) >>> acpi_add_table(table_offsets, tables_blob); >>> build_spcr(tables_blob, tables->linker, vms); >>> - if (vms->ras) { >>> + acpi_ged_state = ACPI_GED(object_resolve_path_type("", TYPE_ACPI_GED, >>> + NULL)); >> >> Testing vms->ras first is cheaper than calling object_resolve_path_type(). Since some people are spending lot of time to reduce VM boot time, it might be worth considering. > Thanks Philippe's comments. > > Do you think it should be written to below[1]? right? > > [1]: > if (vms->ras && acpi_ged_state) No, as: if (vms->ras) { AcpiGedState *acpi_ged_state; acpi_ged_state = ACPI_GED(object_resolve_path_type("", TYPE_ACPI_GED, NULL)); if (acpi_ged_state) { acpi_add_table(table_offsets, tables_blob); ... Maybe I'm wrong and there is not much impact in VM boot time here, reviews welcomed :) >> >>> + if (acpi_ged_state && vms->ras) { >>> acpi_add_table(table_offsets, tables_blob); >>> build_ghes_error_table(tables->hardware_errors, tables->linker); >>> acpi_build_hest(tables_blob, tables->hardware_errors, >>> @@ -925,6 +928,7 @@ void virt_acpi_setup(VirtMachineState *vms) >>> { >
On Wed, 8 Jan 2020 19:32:19 +0800 Dongjiu Geng <gengdongjiu@huawei.com> wrote: in addition to comments of others: > Record the GHEB address via fw_cfg file, when recording > a error to CPER, it will use this address to find out > Generic Error Data Entries and write the error. > > Make the HEST GHES to a GED device. It's hard to parse this even kno Pls rephrase/make commit message more verbose, so it would describe why and what patch is supposed to do > Signed-off-by: Dongjiu Geng <gengdongjiu@huawei.com> > Signed-off-by: Xiang Zheng <zhengxiang9@huawei.com> > --- > hw/acpi/generic_event_device.c | 15 ++++++++++++++- > hw/acpi/ghes.c | 16 ++++++++++++++++ > hw/arm/virt-acpi-build.c | 13 ++++++++++++- > include/hw/acpi/generic_event_device.h | 2 ++ > include/hw/acpi/ghes.h | 6 ++++++ > 5 files changed, 50 insertions(+), 2 deletions(-) > > diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c > index 9cee90c..9bf37e4 100644 > --- a/hw/acpi/generic_event_device.c > +++ b/hw/acpi/generic_event_device.c > @@ -234,12 +234,25 @@ static const VMStateDescription vmstate_ged_state = { > } > }; > > +static const VMStateDescription vmstate_ghes_state = { > + .name = "acpi-ghes-state", > + .version_id = 1, > + .minimum_version_id = 1, > + .fields = (VMStateField[]) { > + VMSTATE_UINT64(ghes_addr_le, AcpiGhesState), > + VMSTATE_END_OF_LIST() > + } > +}; > + > static const VMStateDescription vmstate_acpi_ged = { > .name = "acpi-ged", > .version_id = 1, > .minimum_version_id = 1, > .fields = (VMStateField[]) { > - VMSTATE_STRUCT(ged_state, AcpiGedState, 1, vmstate_ged_state, GEDState), > + VMSTATE_STRUCT(ged_state, AcpiGedState, 1, > + vmstate_ged_state, GEDState), > + VMSTATE_STRUCT(ghes_state, AcpiGedState, 1, > + vmstate_ghes_state, AcpiGhesState), > VMSTATE_END_OF_LIST(), > }, > .subsections = (const VMStateDescription * []) { > diff --git a/hw/acpi/ghes.c b/hw/acpi/ghes.c > index 9d37798..68f4abf 100644 > --- a/hw/acpi/ghes.c > +++ b/hw/acpi/ghes.c > @@ -23,6 +23,7 @@ > #include "hw/acpi/acpi.h" > #include "hw/acpi/ghes.h" > #include "hw/acpi/aml-build.h" > +#include "hw/acpi/generic_event_device.h" > #include "hw/nvram/fw_cfg.h" > #include "sysemu/sysemu.h" > #include "qemu/error-report.h" > @@ -208,3 +209,18 @@ void acpi_build_hest(GArray *table_data, GArray *hardware_errors, > build_header(linker, table_data, (void *)(table_data->data + hest_start), > "HEST", table_data->len - hest_start, 1, NULL, ""); > } > + > +void acpi_ghes_add_fw_cfg(AcpiGhesState *ags, FWCfgState *s, > + GArray *hardware_error) not aligned properly > +{ > + size_t size = 2 * sizeof(uint64_t) + ACPI_GHES_MAX_RAW_DATA_LENGTH; > + size_t request_block_size = ACPI_GHES_ERROR_SOURCE_COUNT * size; > + > + /* Create a read-only fw_cfg file for GHES */ > + fw_cfg_add_file(s, ACPI_GHES_ERRORS_FW_CFG_FILE, hardware_error->data, > + request_block_size); > + > + /* Create a read-write fw_cfg file for Address */ > + fw_cfg_add_file_callback(s, ACPI_GHES_DATA_ADDR_FW_CFG_FILE, NULL, NULL, > + NULL, &(ags->ghes_addr_le), sizeof(ags->ghes_addr_le), false); > +} > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c > index 837bbf9..c8aa94d 100644 > --- a/hw/arm/virt-acpi-build.c > +++ b/hw/arm/virt-acpi-build.c > @@ -797,6 +797,7 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables) > unsigned dsdt, xsdt; > GArray *tables_blob = tables->table_data; > MachineState *ms = MACHINE(vms); > + AcpiGedState *acpi_ged_state; > > table_offsets = g_array_new(false, true /* clear */, > sizeof(uint32_t)); > @@ -831,7 +832,9 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables) > acpi_add_table(table_offsets, tables_blob); > build_spcr(tables_blob, tables->linker, vms); > > - if (vms->ras) { > + acpi_ged_state = ACPI_GED(object_resolve_path_type("", TYPE_ACPI_GED, > + NULL)); > + if (acpi_ged_state && vms->ras) { there is vms->acpi_dev which is GED, so you don't need to look it up suggest: if (ras) { assert(ged) do other fun stuff ... } > acpi_add_table(table_offsets, tables_blob); > build_ghes_error_table(tables->hardware_errors, tables->linker); > acpi_build_hest(tables_blob, tables->hardware_errors, > @@ -925,6 +928,7 @@ void virt_acpi_setup(VirtMachineState *vms) > { > AcpiBuildTables tables; > AcpiBuildState *build_state; > + AcpiGedState *acpi_ged_state; > > if (!vms->fw_cfg) { > trace_virt_acpi_setup(); > @@ -955,6 +959,13 @@ void virt_acpi_setup(VirtMachineState *vms) > fw_cfg_add_file(vms->fw_cfg, ACPI_BUILD_TPMLOG_FILE, tables.tcpalog->data, > acpi_data_len(tables.tcpalog)); > > + acpi_ged_state = ACPI_GED(object_resolve_path_type("", TYPE_ACPI_GED, > + NULL)); > + if (acpi_ged_state && vms->ras) { ditto > + acpi_ghes_add_fw_cfg(&acpi_ged_state->ghes_state, > + vms->fw_cfg, tables.hardware_errors); > + } > + > build_state->rsdp_mr = acpi_add_rom_blob(virt_acpi_build_update, > build_state, tables.rsdp, > ACPI_BUILD_RSDP_FILE, 0); > diff --git a/include/hw/acpi/generic_event_device.h b/include/hw/acpi/generic_event_device.h > index d157eac..037d2b5 100644 > --- a/include/hw/acpi/generic_event_device.h > +++ b/include/hw/acpi/generic_event_device.h > @@ -61,6 +61,7 @@ > > #include "hw/sysbus.h" > #include "hw/acpi/memory_hotplug.h" > +#include "hw/acpi/ghes.h" > > #define ACPI_POWER_BUTTON_DEVICE "PWRB" > > @@ -95,6 +96,7 @@ typedef struct AcpiGedState { > GEDState ged_state; > uint32_t ged_event_bitmap; > qemu_irq irq; > + AcpiGhesState ghes_state; > } AcpiGedState; > > void build_ged_aml(Aml *table, const char* name, HotplugHandler *hotplug_dev, > diff --git a/include/hw/acpi/ghes.h b/include/hw/acpi/ghes.h > index 09a7f86..a6761e6 100644 > --- a/include/hw/acpi/ghes.h > +++ b/include/hw/acpi/ghes.h > @@ -60,7 +60,13 @@ enum { > ACPI_HEST_SRC_ID_RESERVED, > }; > > +typedef struct AcpiGhesState { > + uint64_t ghes_addr_le; > +} AcpiGhesState; > + > void build_ghes_error_table(GArray *hardware_errors, BIOSLinker *linker); > void acpi_build_hest(GArray *table_data, GArray *hardware_error, > BIOSLinker *linker); > +void acpi_ghes_add_fw_cfg(AcpiGhesState *vms, FWCfgState *s, > + GArray *hardware_errors); > #endif
On Tue, 28 Jan 2020 15:41:10 +0100 Igor Mammedov <imammedo@redhat.com> wrote: > On Wed, 8 Jan 2020 19:32:19 +0800 > Dongjiu Geng <gengdongjiu@huawei.com> wrote: > > in addition to comments of others: > > > Record the GHEB address via fw_cfg file, when recording > > a error to CPER, it will use this address to find out > > Generic Error Data Entries and write the error. > > > > Make the HEST GHES to a GED device. > > It's hard to parse this even kno mine comments are not readable either, let me try again: What I've lost at the end and wanted to say is that even when one knows context it's hard to read commit message and it will definitely be unreadable when one forgets context. > Pls rephrase/make commit message more verbose, > so it would describe why and what patch is supposed to do [...]
sorry for the late response due to Chinese new year On 2020/1/28 22:41, Igor Mammedov wrote: > On Wed, 8 Jan 2020 19:32:19 +0800 > Dongjiu Geng <gengdongjiu@huawei.com> wrote: > > in addition to comments of others: > >> Record the GHEB address via fw_cfg file, when recording >> a error to CPER, it will use this address to find out >> Generic Error Data Entries and write the error. >> >> Make the HEST GHES to a GED device. > > It's hard to parse this even kno > Pls rephrase/make commit message more verbose, > so it would describe why and what patch is supposed to do Ok, thanks for the comments. > > >> Signed-off-by: Dongjiu Geng <gengdongjiu@huawei.com> >> Signed-off-by: Xiang Zheng <zhengxiang9@huawei.com> >> --- >> hw/acpi/generic_event_device.c | 15 ++++++++++++++- >> hw/acpi/ghes.c | 16 ++++++++++++++++ >> hw/arm/virt-acpi-build.c | 13 ++++++++++++- >> include/hw/acpi/generic_event_device.h | 2 ++ >> include/hw/acpi/ghes.h | 6 ++++++ >> 5 files changed, 50 insertions(+), 2 deletions(-) >> >> diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c >> index 9cee90c..9bf37e4 100644 >> --- a/hw/acpi/generic_event_device.c >> +++ b/hw/acpi/generic_event_device.c >> @@ -234,12 +234,25 @@ static const VMStateDescription vmstate_ged_state = { >> } >> }; >> >> +static const VMStateDescription vmstate_ghes_state = { >> + .name = "acpi-ghes-state", >> + .version_id = 1, >> + .minimum_version_id = 1, >> + .fields = (VMStateField[]) { >> + VMSTATE_UINT64(ghes_addr_le, AcpiGhesState), >> + VMSTATE_END_OF_LIST() >> + } >> +}; >> + >> static const VMStateDescription vmstate_acpi_ged = { >> .name = "acpi-ged", >> .version_id = 1, >> .minimum_version_id = 1, >> .fields = (VMStateField[]) { >> - VMSTATE_STRUCT(ged_state, AcpiGedState, 1, vmstate_ged_state, GEDState), >> + VMSTATE_STRUCT(ged_state, AcpiGedState, 1, >> + vmstate_ged_state, GEDState), >> + VMSTATE_STRUCT(ghes_state, AcpiGedState, 1, >> + vmstate_ghes_state, AcpiGhesState), >> VMSTATE_END_OF_LIST(), >> }, >> .subsections = (const VMStateDescription * []) { >> diff --git a/hw/acpi/ghes.c b/hw/acpi/ghes.c >> index 9d37798..68f4abf 100644 >> --- a/hw/acpi/ghes.c >> +++ b/hw/acpi/ghes.c >> @@ -23,6 +23,7 @@ >> #include "hw/acpi/acpi.h" >> #include "hw/acpi/ghes.h" >> #include "hw/acpi/aml-build.h" >> +#include "hw/acpi/generic_event_device.h" >> #include "hw/nvram/fw_cfg.h" >> #include "sysemu/sysemu.h" >> #include "qemu/error-report.h" >> @@ -208,3 +209,18 @@ void acpi_build_hest(GArray *table_data, GArray *hardware_errors, >> build_header(linker, table_data, (void *)(table_data->data + hest_start), >> "HEST", table_data->len - hest_start, 1, NULL, ""); >> } >> + >> +void acpi_ghes_add_fw_cfg(AcpiGhesState *ags, FWCfgState *s, >> + GArray *hardware_error) > > not aligned properly will modify it. > >> +{ >> + size_t size = 2 * sizeof(uint64_t) + ACPI_GHES_MAX_RAW_DATA_LENGTH; >> + size_t request_block_size = ACPI_GHES_ERROR_SOURCE_COUNT * size; >> + >> + /* Create a read-only fw_cfg file for GHES */ >> + fw_cfg_add_file(s, ACPI_GHES_ERRORS_FW_CFG_FILE, hardware_error->data, >> + request_block_size); >> + >> + /* Create a read-write fw_cfg file for Address */ >> + fw_cfg_add_file_callback(s, ACPI_GHES_DATA_ADDR_FW_CFG_FILE, NULL, NULL, >> + NULL, &(ags->ghes_addr_le), sizeof(ags->ghes_addr_le), false); >> +} >> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c >> index 837bbf9..c8aa94d 100644 >> --- a/hw/arm/virt-acpi-build.c >> +++ b/hw/arm/virt-acpi-build.c >> @@ -797,6 +797,7 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables) >> unsigned dsdt, xsdt; >> GArray *tables_blob = tables->table_data; >> MachineState *ms = MACHINE(vms); >> + AcpiGedState *acpi_ged_state; >> >> table_offsets = g_array_new(false, true /* clear */, >> sizeof(uint32_t)); >> @@ -831,7 +832,9 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables) >> acpi_add_table(table_offsets, tables_blob); >> build_spcr(tables_blob, tables->linker, vms); >> >> - if (vms->ras) { >> + acpi_ged_state = ACPI_GED(object_resolve_path_type("", TYPE_ACPI_GED, >> + NULL)); >> + if (acpi_ged_state && vms->ras) { > > there is vms->acpi_dev which is GED, so you don't need to look it up > > suggest: Thanks for the suggestion. > if (ras) { > assert(ged) assert(vms->acpi_dev), right? > do other fun stuff ... > } > >> acpi_add_table(table_offsets, tables_blob); >> build_ghes_error_table(tables->hardware_errors, tables->linker); >> acpi_build_hest(tables_blob, tables->hardware_errors, >> @@ -925,6 +928,7 @@ void virt_acpi_setup(VirtMachineState *vms) >> { >> AcpiBuildTables tables; >> AcpiBuildState *build_state; >> + AcpiGedState *acpi_ged_state; >> >> if (!vms->fw_cfg) { >> trace_virt_acpi_setup(); >> @@ -955,6 +959,13 @@ void virt_acpi_setup(VirtMachineState *vms) >> fw_cfg_add_file(vms->fw_cfg, ACPI_BUILD_TPMLOG_FILE, tables.tcpalog->data, >> acpi_data_len(tables.tcpalog)); >> >> + acpi_ged_state = ACPI_GED(object_resolve_path_type("", TYPE_ACPI_GED, >> + NULL)); >> + if (acpi_ged_state && vms->ras) { > > ditto Ok. > >> + acpi_ghes_add_fw_cfg(&acpi_ged_state->ghes_state, >> + vms->fw_cfg, tables.hardware_errors); >> + } >> + >> build_state->rsdp_mr = acpi_add_rom_blob(virt_acpi_build_update, >> build_state, tables.rsdp, >> ACPI_BUILD_RSDP_FILE, 0); >> diff --git a/include/hw/acpi/generic_event_device.h b/include/hw/acpi/generic_event_device.h >> index d157eac..037d2b5 100644 >> --- a/include/hw/acpi/generic_event_device.h >> +++ b/include/hw/acpi/generic_event_device.h >> @@ -61,6 +61,7 @@ >> >> #include "hw/sysbus.h" >> #include "hw/acpi/memory_hotplug.h" >> +#include "hw/acpi/ghes.h" >> >> #define ACPI_POWER_BUTTON_DEVICE "PWRB" >> >> @@ -95,6 +96,7 @@ typedef struct AcpiGedState { >> GEDState ged_state; >> uint32_t ged_event_bitmap; >> qemu_irq irq; >> + AcpiGhesState ghes_state; >> } AcpiGedState; >> >> void build_ged_aml(Aml *table, const char* name, HotplugHandler *hotplug_dev, >> diff --git a/include/hw/acpi/ghes.h b/include/hw/acpi/ghes.h >> index 09a7f86..a6761e6 100644 >> --- a/include/hw/acpi/ghes.h >> +++ b/include/hw/acpi/ghes.h >> @@ -60,7 +60,13 @@ enum { >> ACPI_HEST_SRC_ID_RESERVED, >> }; >> >> +typedef struct AcpiGhesState { >> + uint64_t ghes_addr_le; >> +} AcpiGhesState; >> + >> void build_ghes_error_table(GArray *hardware_errors, BIOSLinker *linker); >> void acpi_build_hest(GArray *table_data, GArray *hardware_error, >> BIOSLinker *linker); >> +void acpi_ghes_add_fw_cfg(AcpiGhesState *vms, FWCfgState *s, >> + GArray *hardware_errors); >> #endif > > . >
On Sun, 2 Feb 2020 20:44:35 +0800 gengdongjiu <gengdongjiu@huawei.com> wrote: > sorry for the late response due to Chinese new year > > On 2020/1/28 22:41, Igor Mammedov wrote: > > On Wed, 8 Jan 2020 19:32:19 +0800 > > Dongjiu Geng <gengdongjiu@huawei.com> wrote: > > > > in addition to comments of others: > > > >> Record the GHEB address via fw_cfg file, when recording > >> a error to CPER, it will use this address to find out > >> Generic Error Data Entries and write the error. > >> > >> Make the HEST GHES to a GED device. [...] > >> @@ -831,7 +832,9 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables) > >> acpi_add_table(table_offsets, tables_blob); > >> build_spcr(tables_blob, tables->linker, vms); > >> > >> - if (vms->ras) { > >> + acpi_ged_state = ACPI_GED(object_resolve_path_type("", TYPE_ACPI_GED, > >> + NULL)); > >> + if (acpi_ged_state && vms->ras) { > > > > there is vms->acpi_dev which is GED, so you don't need to look it up > > > > suggest: > Thanks for the suggestion. > > > if (ras) { > > assert(ged) > assert(vms->acpi_dev), right? yes, something like this. > > do other fun stuff ... > > } > > > > >> acpi_add_table(table_offsets, tables_blob); > >> build_ghes_error_table(tables->hardware_errors, tables->linker); > >> acpi_build_hest(tables_blob, tables->hardware_errors, [...]
On 2020/1/17 0:44, Peter Maydell wrote: > On Wed, 8 Jan 2020 at 11:33, Dongjiu Geng <gengdongjiu@huawei.com> wrote: >> >> Record the GHEB address via fw_cfg file, when recording >> a error to CPER, it will use this address to find out >> Generic Error Data Entries and write the error. >> >> Make the HEST GHES to a GED device. >> >> Signed-off-by: Dongjiu Geng <gengdongjiu@huawei.com> >> Signed-off-by: Xiang Zheng <zhengxiang9@huawei.com> >> --- >> hw/acpi/generic_event_device.c | 15 ++++++++++++++- >> hw/acpi/ghes.c | 16 ++++++++++++++++ >> hw/arm/virt-acpi-build.c | 13 ++++++++++++- >> include/hw/acpi/generic_event_device.h | 2 ++ >> include/hw/acpi/ghes.h | 6 ++++++ >> 5 files changed, 50 insertions(+), 2 deletions(-) >> >> diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c >> index 9cee90c..9bf37e4 100644 >> --- a/hw/acpi/generic_event_device.c >> +++ b/hw/acpi/generic_event_device.c >> @@ -234,12 +234,25 @@ static const VMStateDescription vmstate_ged_state = { >> } >> }; >> >> +static const VMStateDescription vmstate_ghes_state = { >> + .name = "acpi-ghes-state", >> + .version_id = 1, >> + .minimum_version_id = 1, >> + .fields = (VMStateField[]) { >> + VMSTATE_UINT64(ghes_addr_le, AcpiGhesState), >> + VMSTATE_END_OF_LIST() >> + } >> +}; >> + >> static const VMStateDescription vmstate_acpi_ged = { >> .name = "acpi-ged", >> .version_id = 1, >> .minimum_version_id = 1, >> .fields = (VMStateField[]) { >> - VMSTATE_STRUCT(ged_state, AcpiGedState, 1, vmstate_ged_state, GEDState), >> + VMSTATE_STRUCT(ged_state, AcpiGedState, 1, >> + vmstate_ged_state, GEDState), >> + VMSTATE_STRUCT(ghes_state, AcpiGedState, 1, >> + vmstate_ghes_state, AcpiGhesState), >> VMSTATE_END_OF_LIST(), >> }, >> .subsections = (const VMStateDescription * []) { > > You can't just add fields to an existing VMStateDescription > like this -- it will break migration compatibility. Instead you > need to add a new subsection to this vmstate, with a '.needed' > function which indicates when the subsection should be present. Hi Peter/Igor In order to avoid migration failure, do you think whether below change is Ok to make error table address(AcpiGhesState) to a part of GED device? thanks a lot in advance. --------------------------------------------------------------------------------------------- diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c index 021ed2b..264154d 100644 --- a/hw/acpi/generic_event_device.c +++ b/hw/acpi/generic_event_device.c @@ -234,16 +234,34 @@ static const VMStateDescription vmstate_ged_state = { } }; +static bool ghes_needed(void *opaque) +{ + return object_property_get_bool(qdev_get_machine(), "ras", NULL); +} + +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() + } +}; + static const VMStateDescription vmstate_acpi_ged = { .name = "acpi-ged", .version_id = 1, .minimum_version_id = 1, .fields = (VMStateField[]) { - VMSTATE_STRUCT(ged_state, AcpiGedState, 1, vmstate_ged_state, GEDState), + VMSTATE_STRUCT(ged_state, AcpiGedState, 1, + vmstate_ged_state, GEDState), VMSTATE_END_OF_LIST(), }, .subsections = (const VMStateDescription * []) { &vmstate_memhp_state, + &vmstate_ghes_state, NULL } }; diff --git a/hw/acpi/ghes.c b/hw/acpi/ghes.c index a67b1de..3bf32ec 100644 --- a/hw/acpi/ghes.c +++ b/hw/acpi/ghes.c @@ -24,6 +24,7 @@ #include "hw/acpi/acpi.h" #include "hw/acpi/ghes.h" #include "hw/acpi/aml-build.h" +#include "hw/acpi/generic_event_device.h" #include "hw/nvram/fw_cfg.h" #include "sysemu/sysemu.h" #include "qemu/error-report.h" @@ -216,3 +217,18 @@ void acpi_build_hest(GArray *table_data, BIOSLinker *linker) build_header(linker, table_data, (void *)(table_data->data + hest_start), "HEST", table_data->len - hest_start, 1, NULL, ""); } + +void acpi_ghes_add_fw_cfg(AcpiGhesState *ags, FWCfgState *s, + GArray *hardware_error) +{ + size_t size = 2 * sizeof(uint64_t) + ACPI_GHES_MAX_RAW_DATA_LENGTH; + size_t request_block_size = ACPI_GHES_ERROR_SOURCE_COUNT * size; + + /* Create a read-only fw_cfg file for GHES */ + fw_cfg_add_file(s, ACPI_GHES_ERRORS_FW_CFG_FILE, hardware_error->data, + request_block_size); + + /* Create a read-write fw_cfg file for Address */ + fw_cfg_add_file_callback(s, ACPI_GHES_DATA_ADDR_FW_CFG_FILE, NULL, NULL, + NULL, &(ags->ghes_addr_le), sizeof(ags->ghes_addr_le), false); +} diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c index 12a9a78..d6e7521 100644 --- a/hw/arm/virt-acpi-build.c +++ b/hw/arm/virt-acpi-build.c @@ -832,6 +832,7 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables) build_spcr(tables_blob, tables->linker, vms); if (vms->ras) { + assert(vms->acpi_dev); acpi_add_table(table_offsets, tables_blob); build_ghes_error_table(tables->hardware_errors, tables->linker); acpi_build_hest(tables_blob, tables->linker); @@ -924,6 +925,7 @@ void virt_acpi_setup(VirtMachineState *vms) { AcpiBuildTables tables; AcpiBuildState *build_state; + AcpiGedState *acpi_ged_state; if (!vms->fw_cfg) { trace_virt_acpi_setup(); @@ -954,6 +956,14 @@ void virt_acpi_setup(VirtMachineState *vms) fw_cfg_add_file(vms->fw_cfg, ACPI_BUILD_TPMLOG_FILE, tables.tcpalog->data, acpi_data_len(tables.tcpalog)); + if (vms->ras) { + assert(vms->acpi_dev); + acpi_ged_state = ACPI_GED(object_resolve_path_type("", TYPE_ACPI_GED, + NULL)); + acpi_ghes_add_fw_cfg(&acpi_ged_state->ghes_state, + vms->fw_cfg, tables.hardware_errors); + } + build_state->rsdp_mr = acpi_add_rom_blob(virt_acpi_build_update, build_state, tables.rsdp, ACPI_BUILD_RSDP_FILE, 0); diff --git a/include/hw/acpi/generic_event_device.h b/include/hw/acpi/generic_event_device.h index d157eac..037d2b5 100644 --- a/include/hw/acpi/generic_event_device.h +++ b/include/hw/acpi/generic_event_device.h @@ -61,6 +61,7 @@ #include "hw/sysbus.h" #include "hw/acpi/memory_hotplug.h" +#include "hw/acpi/ghes.h" #define ACPI_POWER_BUTTON_DEVICE "PWRB" @@ -95,6 +96,7 @@ typedef struct AcpiGedState { GEDState ged_state; uint32_t ged_event_bitmap; qemu_irq irq; + AcpiGhesState ghes_state; } AcpiGedState; void build_ged_aml(Aml *table, const char* name, HotplugHandler *hotplug_dev, diff --git a/include/hw/acpi/ghes.h b/include/hw/acpi/ghes.h index 18debd8..a3420fc 100644 --- a/include/hw/acpi/ghes.h +++ b/include/hw/acpi/ghes.h @@ -62,6 +62,12 @@ enum { ACPI_HEST_SRC_ID_RESERVED, }; +typedef struct AcpiGhesState { + uint64_t ghes_addr_le; +} AcpiGhesState; + void build_ghes_error_table(GArray *hardware_errors, BIOSLinker *linker); void acpi_build_hest(GArray *table_data, BIOSLinker *linker); +void acpi_ghes_add_fw_cfg(AcpiGhesState *vms, FWCfgState *s, + GArray *hardware_errors); #endif > > thanks > -- PMM > . >
diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c index 9cee90c..9bf37e4 100644 --- a/hw/acpi/generic_event_device.c +++ b/hw/acpi/generic_event_device.c @@ -234,12 +234,25 @@ static const VMStateDescription vmstate_ged_state = { } }; +static const VMStateDescription vmstate_ghes_state = { + .name = "acpi-ghes-state", + .version_id = 1, + .minimum_version_id = 1, + .fields = (VMStateField[]) { + VMSTATE_UINT64(ghes_addr_le, AcpiGhesState), + VMSTATE_END_OF_LIST() + } +}; + static const VMStateDescription vmstate_acpi_ged = { .name = "acpi-ged", .version_id = 1, .minimum_version_id = 1, .fields = (VMStateField[]) { - VMSTATE_STRUCT(ged_state, AcpiGedState, 1, vmstate_ged_state, GEDState), + VMSTATE_STRUCT(ged_state, AcpiGedState, 1, + vmstate_ged_state, GEDState), + VMSTATE_STRUCT(ghes_state, AcpiGedState, 1, + vmstate_ghes_state, AcpiGhesState), VMSTATE_END_OF_LIST(), }, .subsections = (const VMStateDescription * []) { diff --git a/hw/acpi/ghes.c b/hw/acpi/ghes.c index 9d37798..68f4abf 100644 --- a/hw/acpi/ghes.c +++ b/hw/acpi/ghes.c @@ -23,6 +23,7 @@ #include "hw/acpi/acpi.h" #include "hw/acpi/ghes.h" #include "hw/acpi/aml-build.h" +#include "hw/acpi/generic_event_device.h" #include "hw/nvram/fw_cfg.h" #include "sysemu/sysemu.h" #include "qemu/error-report.h" @@ -208,3 +209,18 @@ void acpi_build_hest(GArray *table_data, GArray *hardware_errors, build_header(linker, table_data, (void *)(table_data->data + hest_start), "HEST", table_data->len - hest_start, 1, NULL, ""); } + +void acpi_ghes_add_fw_cfg(AcpiGhesState *ags, FWCfgState *s, + GArray *hardware_error) +{ + size_t size = 2 * sizeof(uint64_t) + ACPI_GHES_MAX_RAW_DATA_LENGTH; + size_t request_block_size = ACPI_GHES_ERROR_SOURCE_COUNT * size; + + /* Create a read-only fw_cfg file for GHES */ + fw_cfg_add_file(s, ACPI_GHES_ERRORS_FW_CFG_FILE, hardware_error->data, + request_block_size); + + /* Create a read-write fw_cfg file for Address */ + fw_cfg_add_file_callback(s, ACPI_GHES_DATA_ADDR_FW_CFG_FILE, NULL, NULL, + NULL, &(ags->ghes_addr_le), sizeof(ags->ghes_addr_le), false); +} diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c index 837bbf9..c8aa94d 100644 --- a/hw/arm/virt-acpi-build.c +++ b/hw/arm/virt-acpi-build.c @@ -797,6 +797,7 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables) unsigned dsdt, xsdt; GArray *tables_blob = tables->table_data; MachineState *ms = MACHINE(vms); + AcpiGedState *acpi_ged_state; table_offsets = g_array_new(false, true /* clear */, sizeof(uint32_t)); @@ -831,7 +832,9 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables) acpi_add_table(table_offsets, tables_blob); build_spcr(tables_blob, tables->linker, vms); - if (vms->ras) { + acpi_ged_state = ACPI_GED(object_resolve_path_type("", TYPE_ACPI_GED, + NULL)); + if (acpi_ged_state && vms->ras) { acpi_add_table(table_offsets, tables_blob); build_ghes_error_table(tables->hardware_errors, tables->linker); acpi_build_hest(tables_blob, tables->hardware_errors, @@ -925,6 +928,7 @@ void virt_acpi_setup(VirtMachineState *vms) { AcpiBuildTables tables; AcpiBuildState *build_state; + AcpiGedState *acpi_ged_state; if (!vms->fw_cfg) { trace_virt_acpi_setup(); @@ -955,6 +959,13 @@ void virt_acpi_setup(VirtMachineState *vms) fw_cfg_add_file(vms->fw_cfg, ACPI_BUILD_TPMLOG_FILE, tables.tcpalog->data, acpi_data_len(tables.tcpalog)); + acpi_ged_state = ACPI_GED(object_resolve_path_type("", TYPE_ACPI_GED, + NULL)); + if (acpi_ged_state && vms->ras) { + acpi_ghes_add_fw_cfg(&acpi_ged_state->ghes_state, + vms->fw_cfg, tables.hardware_errors); + } + build_state->rsdp_mr = acpi_add_rom_blob(virt_acpi_build_update, build_state, tables.rsdp, ACPI_BUILD_RSDP_FILE, 0); diff --git a/include/hw/acpi/generic_event_device.h b/include/hw/acpi/generic_event_device.h index d157eac..037d2b5 100644 --- a/include/hw/acpi/generic_event_device.h +++ b/include/hw/acpi/generic_event_device.h @@ -61,6 +61,7 @@ #include "hw/sysbus.h" #include "hw/acpi/memory_hotplug.h" +#include "hw/acpi/ghes.h" #define ACPI_POWER_BUTTON_DEVICE "PWRB" @@ -95,6 +96,7 @@ typedef struct AcpiGedState { GEDState ged_state; uint32_t ged_event_bitmap; qemu_irq irq; + AcpiGhesState ghes_state; } AcpiGedState; void build_ged_aml(Aml *table, const char* name, HotplugHandler *hotplug_dev, diff --git a/include/hw/acpi/ghes.h b/include/hw/acpi/ghes.h index 09a7f86..a6761e6 100644 --- a/include/hw/acpi/ghes.h +++ b/include/hw/acpi/ghes.h @@ -60,7 +60,13 @@ enum { ACPI_HEST_SRC_ID_RESERVED, }; +typedef struct AcpiGhesState { + uint64_t ghes_addr_le; +} AcpiGhesState; + void build_ghes_error_table(GArray *hardware_errors, BIOSLinker *linker); void acpi_build_hest(GArray *table_data, GArray *hardware_error, BIOSLinker *linker); +void acpi_ghes_add_fw_cfg(AcpiGhesState *vms, FWCfgState *s, + GArray *hardware_errors); #endif