KVM Archive on lore.kernel.org
 help / color / Atom feed
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;
> >>
> >
> > .
> >
>
>

  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 publically 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