From: Beata Michalska <beata.michalska@linaro.org> To: gengdongjiu <gengdongjiu@huawei.com> Cc: Igor Mammedov <imammedo@redhat.com>, Xiang Zheng <zhengxiang9@huawei.com>, Peter Maydell <peter.maydell@linaro.org>, ehabkost@redhat.com, kvm@vger.kernel.org, mst@redhat.com, wanghaibin.wang@huawei.com, mtosatti@redhat.com, qemu-devel@nongnu.org, linuxarm@huawei.com, shannon.zhaosl@gmail.com, qemu-arm@nongnu.org, james.morse@arm.com, xuwei5@huawei.com, jonathan.cameron@huawei.com, pbonzini@redhat.com, Laszlo Ersek <lersek@redhat.com>, rth@twiddle.net Subject: Re: [RESEND PATCH v21 3/6] ACPI: Add APEI GHES table generation support Date: Fri, 22 Nov 2019 15:44:05 +0000 Message-ID: <CADSWDzvYY7cRZ62N1QEtJz8=KCtCnO9A2vK0rjyn1Opn+KxAwQ@mail.gmail.com> (raw) In-Reply-To: <cf5e5aa4-2283-6cf9-70d0-278d167e3a13@huawei.com> Hi, On Mon, 18 Nov 2019 at 12:50, gengdongjiu <gengdongjiu@huawei.com> wrote: > > Hi,Igor, > Thanks for you review and time. > > > > >> + /* > >> + * Type: > >> + * Generic Hardware Error Source version 2(GHESv2 - Type 10) > >> + */ > >> + build_append_int_noprefix(table_data, ACPI_GHES_SOURCE_GENERIC_ERROR_V2, 2); > >> + /* > >> + * Source Id > > > >> + * Once we support more than one hardware error sources, we need to > >> + * increase the value of this field. > > I'm not sure ^^^ is correct, according to spec it's just unique id per > > distinct error structure, so we just assign arbitrary values to each > > declared source and that never changes once assigned. > The source id is used to distinct the error source, for each source, the ‘source id’ is unique, > but different source has different source id. for example, the 'source id' of the error source 0 is 0, > the 'source id' of the error source 1 is 1. > I might be wrong but the source id is not a sequence number and it can have any value as long as it is unique and the comment 're 'increasing the number' reads bit wrong. > > > > > For now I'd make source_id an enum with one member > > enum { > > ACPI_HEST_SRC_ID_SEA = 0, > > /* future ids go here */ > > ACPI_HEST_SRC_ID_RESERVED, > > } > If we only have one error source, we can use enum instead of allocating magic 0. > But if we have more error source , such as 10 error source. using enum maybe not a good idea. > > for example, if there are 10 error sources, I can just using below loop > > for(i=0; i< 10; i++) > build_ghes_v2(source_id++); > You can do that but using enum makes it more readable and maintainable. Also you can keep the source id as a sequence number but still represent that with enum, as it has been suggested, and use the 'RESERVED' field for loop control. I think it might be also worth to represent the HES type as enum as well : enum{ ACPI_HES_TYPE_GHESv2 = 10, }; > > > > and use that instead of allocating magic 0 at the beginning of the function. > > build_ghes_v2(ACPI_HEST_GHES_SEA); > > Also add a comment to declaration that already assigned values are not to be changed > > > >> + */ > >> + build_append_int_noprefix(table_data, source_id, 2); > >> + /* Related Source Id */ > >> + build_append_int_noprefix(table_data, 0xffff, 2); > >> + /* Flags */ > >> + build_append_int_noprefix(table_data, 0, 1); > >> + /* Enabled */ > >> + build_append_int_noprefix(table_data, 1, 1); > >> + > >> + /* Number of Records To Pre-allocate */ > >> + build_append_int_noprefix(table_data, 1, 4); > >> + /* Max Sections Per Record */ > >> + build_append_int_noprefix(table_data, 1, 4); > >> + /* Max Raw Data Length */ > >> + build_append_int_noprefix(table_data, ACPI_GHES_MAX_RAW_DATA_LENGTH, 4); > >> + > >> + /* Error Status Address */ > >> + build_append_gas(table_data, AML_AS_SYSTEM_MEMORY, 0x40, 0, > >> + 4 /* QWord access */, 0); > >> + bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE, > >> + ACPI_GHES_ERROR_STATUS_ADDRESS_OFFSET(hest_start, source_id), > > it's fine only if GHESv2 is the only entries in HEST, but once > > other types are added this macro will silently fall apart and > > cause table corruption. > > > > Instead of offset from hest_start, I suggest to use offset relative > > to GAS structure, here is an idea > > > > #define GAS_ADDR_OFFSET 4 > > > > off = table->len > > build_append_gas() > > bios_linker_loader_add_pointer(..., > > off + GAS_ADDR_OFFSET, ... > I think your suggestion is good. > > > > >> + ACPI_GHES_ADDRESS_SIZE, ACPI_GHES_ERRORS_FW_CFG_FILE, > >> + source_id * ACPI_GHES_ADDRESS_SIZE); > >> + > >> + /* > >> + * Notification Structure > >> + * Now only enable ARMv8 SEA notification type > >> + */ > >> + acpi_ghes_build_notify(table_data, ACPI_GHES_NOTIFY_SEA); > >> + > >> + /* Error Status Block Length */ > >> + build_append_int_noprefix(table_data, ACPI_GHES_MAX_RAW_DATA_LENGTH, 4); > >> + > >> + /* > >> + * Read Ack Register > >> + * ACPI 6.1: 18.3.2.8 Generic Hardware Error Source > >> + * version 2 (GHESv2 - Type 10) > >> + */ > >> + build_append_gas(table_data, AML_AS_SYSTEM_MEMORY, 0x40, 0, > >> + 4 /* QWord access */, 0); > >> + bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE, > >> + ACPI_GHES_READ_ACK_REGISTER_ADDRESS_OFFSET(hest_start, 0), > > ditto > > > >> + ACPI_GHES_ADDRESS_SIZE, ACPI_GHES_ERRORS_FW_CFG_FILE, > >> + (ACPI_GHES_ERROR_SOURCE_COUNT + source_id) * ACPI_GHES_ADDRESS_SIZE); > >> + > >> + /* > >> + * Read Ack Preserve > >> + * We only provide the first bit in Read Ack Register to OSPM to write > >> + * while the other bits are preserved. > >> + */ > >> + build_append_int_noprefix(table_data, ~0x1ULL, 8); > >> + /* Read Ack Write */ > >> + build_append_int_noprefix(table_data, 0x1, 8); > >> + > >> + build_header(linker, table_data, (void *)(table_data->data + hest_start), > >> + "HEST", table_data->len - hest_start, 1, NULL, "GHES"); > > hest is not GHEST specific so s/GHES/NULL/ > > > >> +} > >> + > >> +static AcpiGhesState ges; > >> +void acpi_ghes_add_fw_cfg(FWCfgState *s, GArray *hardware_error) > >> +{ > >> + > >> + size_t size = 2 * ACPI_GHES_ADDRESS_SIZE + 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, &ges.ghes_addr_le, sizeof(ges.ghes_addr_le), false); > >> +} > >> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c > >> index 2c3702b882..3681ec6e3d 100644 > >> --- a/hw/acpi/aml-build.c > >> +++ b/hw/acpi/aml-build.c > >> @@ -1578,6 +1578,7 @@ void acpi_build_tables_init(AcpiBuildTables *tables) > >> tables->table_data = g_array_new(false, true /* clear */, 1); > >> tables->tcpalog = g_array_new(false, true /* clear */, 1); > >> tables->vmgenid = g_array_new(false, true /* clear */, 1); > >> + tables->hardware_errors = g_array_new(false, true /* clear */, 1); > >> tables->linker = bios_linker_loader_init(); > >> } > >> > >> @@ -1588,6 +1589,7 @@ void acpi_build_tables_cleanup(AcpiBuildTables *tables, bool mfre) > >> g_array_free(tables->table_data, true); > >> g_array_free(tables->tcpalog, mfre); > >> g_array_free(tables->vmgenid, mfre); > >> + g_array_free(tables->hardware_errors, mfre); > >> } > >> > >> /* > >> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c > >> index 4cd50175e0..1b1fd273e4 100644 > >> --- a/hw/arm/virt-acpi-build.c > >> +++ b/hw/arm/virt-acpi-build.c > >> @@ -48,6 +48,7 @@ > >> #include "sysemu/reset.h" > >> #include "kvm_arm.h" > >> #include "migration/vmstate.h" > >> +#include "hw/acpi/acpi_ghes.h" > >> > >> #define ARM_SPI_BASE 32 > >> > >> @@ -825,6 +826,13 @@ 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_add_table(table_offsets, tables_blob); > >> + acpi_ghes_build_error_table(tables->hardware_errors, tables->linker); > >> + acpi_ghes_build_hest(tables_blob, tables->hardware_errors, > >> + tables->linker); > >> + } > >> + > >> if (ms->numa_state->num_nodes > 0) { > >> acpi_add_table(table_offsets, tables_blob); > >> build_srat(tables_blob, tables->linker, vms); > >> @@ -942,6 +950,10 @@ 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) { > >> + acpi_ghes_add_fw_cfg(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/acpi_ghes.h b/include/hw/acpi/acpi_ghes.h > >> new file mode 100644 > >> index 0000000000..cb62ec9c7b > >> --- /dev/null > >> +++ b/include/hw/acpi/acpi_ghes.h > >> @@ -0,0 +1,56 @@ > >> +/* > >> + * Support for generating APEI tables and recording CPER for Guests > >> + * > >> + * Copyright (c) 2019 HUAWEI TECHNOLOGIES CO., LTD. > >> + * > >> + * Author: Dongjiu Geng <gengdongjiu@huawei.com> > >> + * > >> + * This program is free software; you can redistribute it and/or modify > >> + * it under the terms of the GNU General Public License as published by > >> + * the Free Software Foundation; either version 2 of the License, or > >> + * (at your option) any later version. > >> + > >> + * This program is distributed in the hope that it will be useful, > >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of > >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > >> + * GNU General Public License for more details. > >> + > >> + * You should have received a copy of the GNU General Public License along > >> + * with this program; if not, see <http://www.gnu.org/licenses/>. > >> + */ > >> + > >> +#ifndef ACPI_GHES_H > >> +#define ACPI_GHES_H > >> + > >> +#include "hw/acpi/bios-linker-loader.h" > >> + > >> +/* > >> + * Values for Hardware Error Notification Type field > >> + */ > >> +enum AcpiGhesNotifyType { > >> + ACPI_GHES_NOTIFY_POLLED = 0, /* Polled */ > >> + ACPI_GHES_NOTIFY_EXTERNAL = 1, /* External Interrupt */ > >> + ACPI_GHES_NOTIFY_LOCAL = 2, /* Local Interrupt */ > >> + ACPI_GHES_NOTIFY_SCI = 3, /* SCI */ > >> + ACPI_GHES_NOTIFY_NMI = 4, /* NMI */ > >> + ACPI_GHES_NOTIFY_CMCI = 5, /* CMCI, ACPI 5.0: 18.3.2.7, Table 18-290 */ > >> + ACPI_GHES_NOTIFY_MCE = 6, /* MCE, ACPI 5.0: 18.3.2.7, Table 18-290 */ > >> + /* GPIO-Signal, ACPI 6.0: 18.3.2.7, Table 18-332 */ > >> + ACPI_GHES_NOTIFY_GPIO = 7, > >> + /* ARMv8 SEA, ACPI 6.1: 18.3.2.9, Table 18-345 */ > >> + ACPI_GHES_NOTIFY_SEA = 8, > >> + /* ARMv8 SEI, ACPI 6.1: 18.3.2.9, Table 18-345 */ > >> + ACPI_GHES_NOTIFY_SEI = 9, > >> + /* External Interrupt - GSIV, ACPI 6.1: 18.3.2.9, Table 18-345 */ > >> + ACPI_GHES_NOTIFY_GSIV = 10, > >> + /* Software Delegated Exception, ACPI 6.2: 18.3.2.9, Table 18-383 */ > >> + ACPI_GHES_NOTIFY_SDEI = 11, > >> + ACPI_GHES_NOTIFY_RESERVED = 12 /* 12 and greater are reserved */ > >> +}; > > maybe make all comment go on newline, otherwise zoo above look ugly > sure. > > > > >> + > >> +void acpi_ghes_build_hest(GArray *table_data, GArray *hardware_error, > >> + BIOSLinker *linker); > >> + > >> +void acpi_ghes_build_error_table(GArray *hardware_errors, BIOSLinker *linker); > >> +void acpi_ghes_add_fw_cfg(FWCfgState *s, GArray *hardware_errors); > >> +#endif > >> diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h > >> index de4a406568..8f13620701 100644 > >> --- a/include/hw/acpi/aml-build.h > >> +++ b/include/hw/acpi/aml-build.h > >> @@ -220,6 +220,7 @@ struct AcpiBuildTables { > >> GArray *rsdp; > >> GArray *tcpalog; > >> GArray *vmgenid; > >> + GArray *hardware_errors; > >> BIOSLinker *linker; > >> } AcpiBuildTables; > >> > > > > . > > > >
next prev parent reply index Thread overview: 44+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-11-11 1:40 [RESEND PATCH v21 0/6] Add ARMv8 RAS virtualization support in QEMU Xiang Zheng 2019-11-11 1:40 ` [RESEND PATCH v21 1/6] hw/arm/virt: Introduce a RAS machine option Xiang Zheng 2019-12-02 18:22 ` Peter Maydell 2019-12-07 12:10 ` gengdongjiu 2019-11-11 1:40 ` [RESEND PATCH v21 2/6] docs: APEI GHES generation and CPER record description Xiang Zheng 2019-11-15 9:44 ` Igor Mammedov 2019-11-27 1:37 ` Xiang Zheng 2019-11-27 8:12 ` Igor Mammedov 2019-11-11 1:40 ` [RESEND PATCH v21 3/6] ACPI: Add APEI GHES table generation support Xiang Zheng 2019-11-15 9:38 ` Igor Mammedov 2019-11-18 12:49 ` gengdongjiu 2019-11-18 13:18 ` gengdongjiu 2019-11-18 13:21 ` Michael S. Tsirkin 2019-11-18 13:57 ` gengdongjiu 2019-11-25 9:48 ` Igor Mammedov 2019-11-27 11:16 ` gengdongjiu 2019-11-22 15:44 ` Beata Michalska [this message] 2019-11-22 15:42 ` Beata Michalska 2019-11-25 9:23 ` Igor Mammedov 2019-11-11 1:40 ` [RESEND PATCH v21 4/6] KVM: Move hwpoison page related functions into kvm-all.c Xiang Zheng 2019-12-02 18:23 ` Peter Maydell 2019-11-11 1:40 ` [RESEND PATCH v21 5/6] target-arm: kvm64: handle SIGBUS signal from kernel or KVM Xiang Zheng 2019-11-15 16:37 ` Igor Mammedov 2019-11-22 15:47 ` Beata Michalska 2019-11-25 9:37 ` Igor Mammedov 2019-11-27 1:40 ` Xiang Zheng 2019-11-27 10:43 ` Igor Mammedov 2019-12-21 12:35 ` gengdongjiu 2019-11-22 15:47 ` Beata Michalska 2019-11-27 12:47 ` Xiang Zheng 2019-11-27 13:02 ` Igor Mammedov 2019-11-27 14:17 ` Beata Michalska 2019-12-03 3:35 ` Xiang Zheng 2019-11-27 14:17 ` Beata Michalska 2019-12-03 3:35 ` Xiang Zheng 2019-12-07 9:33 ` gengdongjiu 2019-12-09 13:05 ` Beata Michalska 2019-12-09 14:12 ` gengdongjiu 2019-11-11 1:40 ` [RESEND PATCH v21 6/6] MAINTAINERS: Add APCI/APEI/GHES entries Xiang Zheng 2019-12-02 18:27 ` [RESEND PATCH v21 0/6] Add ARMv8 RAS virtualization support in QEMU Peter Maydell 2019-12-03 2:09 ` gengdongjiu 2019-11-15 14:32 [RESEND PATCH v21 3/6] ACPI: Add APEI GHES table generation support gengdongjiu 2019-11-15 14:55 ` Igor Mammedov 2019-11-16 0:58 gengdongjiu
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to='CADSWDzvYY7cRZ62N1QEtJz8=KCtCnO9A2vK0rjyn1Opn+KxAwQ@mail.gmail.com' \ --to=beata.michalska@linaro.org \ --cc=ehabkost@redhat.com \ --cc=gengdongjiu@huawei.com \ --cc=imammedo@redhat.com \ --cc=james.morse@arm.com \ --cc=jonathan.cameron@huawei.com \ --cc=kvm@vger.kernel.org \ --cc=lersek@redhat.com \ --cc=linuxarm@huawei.com \ --cc=mst@redhat.com \ --cc=mtosatti@redhat.com \ --cc=pbonzini@redhat.com \ --cc=peter.maydell@linaro.org \ --cc=qemu-arm@nongnu.org \ --cc=qemu-devel@nongnu.org \ --cc=rth@twiddle.net \ --cc=shannon.zhaosl@gmail.com \ --cc=wanghaibin.wang@huawei.com \ --cc=xuwei5@huawei.com \ --cc=zhengxiang9@huawei.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
KVM Archive on lore.kernel.org Archives are clonable: git clone --mirror https://lore.kernel.org/kvm/0 kvm/git/0.git # If you have public-inbox 1.1+ installed, you may # initialize and index your mirror using the following commands: public-inbox-init -V2 kvm kvm/ https://lore.kernel.org/kvm \ kvm@vger.kernel.org public-inbox-index kvm Example config snippet for mirrors Newsgroup available over NNTP: nntp://nntp.lore.kernel.org/org.kernel.vger.kvm AGPL code for this site: git clone https://public-inbox.org/public-inbox.git