[PATCH 0/3] hw/acpi, arm: Provide and use acpi_ghes_present()

[PATCH 0/3] hw/acpi, arm: Provide and use acpi_ghes_present()

[Peter Maydell]

Currently the Arm code in target/arm/kvm64.c which decides whether
it should report memory errors via the ACPI GHES table works
only with the 'virt' board; in fact it won't even link if the
virt board is configured out of the QEMU binary.

This patchset replaces those virt-specific checks with a single
new acpi_ghes_present() function which tells the caller whether
it's OK to report errors by calling acpi_ghes_record_errors().
The mechanism we use is a simple flag in the AcpiGhesState
which gets sent if the board calls acpi_ghes_add_fw_cfg() to
set up the GHES stuff.

We need also to provide 'stub' versions of both acpi_ghes_present()
and acpi_ghes_record_errors() so that we can link even if no
board using ACPI GHES has been configured into the binary.

(You can test that this series is necessary by commenting out the
'CONFIG_ARM_VIRT=y' line in default-configs/devices/arm-softmmu.mak
and building with KVM enabled on an AArch64 host.)

I have tested with 'make' and 'make check' but nothing beyond
that; testing by somebody who has a guest setup that uses GHES
would be helpful just to check I haven't accidentally broken it.

thanks
-- PMM

Peter Maydell (3):
  hw/acpi: Provide stub version of acpi_ghes_record_errors()
  hw/acpi: Provide function acpi_ghes_present()
  target/arm: Use acpi_ghes_present() to see if we report ACPI memory
    errors

 include/hw/acpi/ghes.h |  9 +++++++++
 hw/acpi/ghes-stub.c    | 22 ++++++++++++++++++++++
 hw/acpi/ghes.c         | 17 +++++++++++++++++
 target/arm/kvm64.c     |  6 +-----
 hw/acpi/meson.build    |  6 +++---
 5 files changed, 52 insertions(+), 8 deletions(-)
 create mode 100644 hw/acpi/ghes-stub.c

-- 
2.20.1

[PATCH 1/3] hw/acpi: Provide stub version of acpi_ghes_record_errors()

[Peter Maydell]

Generic code in target/arm wants to call acpi_ghes_record_errors();
provide a stub version so that we don't fail to link when
CONFIG_ACPI_APEI is not set. This requires us to add a new
ghes-stub.c file to contain it and the meson.build mechanics
to use it when appropriate.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/acpi/ghes-stub.c | 17 +++++++++++++++++
 hw/acpi/meson.build |  6 +++---
 2 files changed, 20 insertions(+), 3 deletions(-)
 create mode 100644 hw/acpi/ghes-stub.c

diff --git a/hw/acpi/ghes-stub.c b/hw/acpi/ghes-stub.c
new file mode 100644
index 00000000000..9faba043b85
--- /dev/null
+++ b/hw/acpi/ghes-stub.c
@@ -0,0 +1,17 @@
+/*
+ * Support for generating APEI tables and recording CPER for Guests:
+ * stub functions.
+ *
+ * Copyright (c) 2021 Linaro, Ltd
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "hw/acpi/ghes.h"
+
+int acpi_ghes_record_errors(uint8_t source_id, uint64_t physical_address)
+{
+    return -1;
+}
diff --git a/hw/acpi/meson.build b/hw/acpi/meson.build
index dd69577212a..03ea43f8627 100644
--- a/hw/acpi/meson.build
+++ b/hw/acpi/meson.build
@@ -13,13 +13,13 @@ acpi_ss.add(when: 'CONFIG_ACPI_PCI', if_true: files('pci.c'))
 acpi_ss.add(when: 'CONFIG_ACPI_VMGENID', if_true: files('vmgenid.c'))
 acpi_ss.add(when: 'CONFIG_ACPI_HW_REDUCED', if_true: files('generic_event_device.c'))
 acpi_ss.add(when: 'CONFIG_ACPI_HMAT', if_true: files('hmat.c'))
-acpi_ss.add(when: 'CONFIG_ACPI_APEI', if_true: files('ghes.c'))
+acpi_ss.add(when: 'CONFIG_ACPI_APEI', if_true: files('ghes.c'), if_false:('ghes-stub.c'))
 acpi_ss.add(when: 'CONFIG_ACPI_X86', if_true: files('core.c', 'piix4.c', 'pcihp.c'), if_false: files('acpi-stub.c'))
 acpi_ss.add(when: 'CONFIG_ACPI_X86_ICH', if_true: files('ich9.c', 'tco.c'))
 acpi_ss.add(when: 'CONFIG_IPMI', if_true: files('ipmi.c'), if_false: files('ipmi-stub.c'))
 acpi_ss.add(when: 'CONFIG_PC', if_false: files('acpi-x86-stub.c'))
 acpi_ss.add(when: 'CONFIG_TPM', if_true: files('tpm.c'))
-softmmu_ss.add(when: 'CONFIG_ACPI', if_false: files('acpi-stub.c', 'aml-build-stub.c'))
+softmmu_ss.add(when: 'CONFIG_ACPI', if_false: files('acpi-stub.c', 'aml-build-stub.c', 'ghes-stub.c'))
 softmmu_ss.add_all(when: 'CONFIG_ACPI', if_true: acpi_ss)
 softmmu_ss.add(when: 'CONFIG_ALL', if_true: files('acpi-stub.c', 'aml-build-stub.c',
-                                                  'acpi-x86-stub.c', 'ipmi-stub.c'))
+                                                  'acpi-x86-stub.c', 'ipmi-stub.c', 'ghes-stub.c'))
-- 
2.20.1

Re: [PATCH 1/3] hw/acpi: Provide stub version of acpi_ghes_record_errors()

[Richard Henderson]

On 6/3/21 10:12 AM, Peter Maydell wrote:
> +softmmu_ss.add(when: 'CONFIG_ACPI', if_false: files('acpi-stub.c', 'aml-build-stub.c', 'ghes-stub.c'))
>   softmmu_ss.add_all(when: 'CONFIG_ACPI', if_true: acpi_ss)
>   softmmu_ss.add(when: 'CONFIG_ALL', if_true: files('acpi-stub.c', 'aml-build-stub.c',
> -                                                  'acpi-x86-stub.c', 'ipmi-stub.c'))
> +                                                  'acpi-x86-stub.c', 'ipmi-stub.c', 'ghes-stub.c'))

Gosh that last line is confusing.  I see it's documented in build-system.rst, 
but yeesh.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 1/3] hw/acpi: Provide stub version of acpi_ghes_record_errors()

[Paolo Bonzini]

On 03/06/21 20:52, Richard Henderson wrote:
> On 6/3/21 10:12 AM, Peter Maydell wrote:
>> +softmmu_ss.add(when: 'CONFIG_ACPI', if_false: files('acpi-stub.c', 
>> 'aml-build-stub.c', 'ghes-stub.c'))
>>   softmmu_ss.add_all(when: 'CONFIG_ACPI', if_true: acpi_ss)
>>   softmmu_ss.add(when: 'CONFIG_ALL', if_true: files('acpi-stub.c', 
>> 'aml-build-stub.c',
>> -                                                  'acpi-x86-stub.c', 
>> 'ipmi-stub.c'))
>> +                                                  'acpi-x86-stub.c', 
>> 'ipmi-stub.c', 'ghes-stub.c'))
> 
> Gosh that last line is confusing.  I see it's documented in 
> build-system.rst, but yeesh.
> 
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

Yeah, it's vestigial of the makefiles and I should remove it.

That said, here:

> +acpi_ss.add(when: 'CONFIG_ACPI_APEI', if_true: files('ghes.c'), if_false:('ghes-stub.c'))

There's a missing "files" after if_false.

Paolo

Re: [PATCH 1/3] hw/acpi: Provide stub version of acpi_ghes_record_errors()

[Peter Maydell]

On Thu, 17 Jun 2021 at 11:28, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 03/06/21 20:52, Richard Henderson wrote:
> > On 6/3/21 10:12 AM, Peter Maydell wrote:
> >> +softmmu_ss.add(when: 'CONFIG_ACPI', if_false: files('acpi-stub.c',
> >> 'aml-build-stub.c', 'ghes-stub.c'))
> >>   softmmu_ss.add_all(when: 'CONFIG_ACPI', if_true: acpi_ss)
> >>   softmmu_ss.add(when: 'CONFIG_ALL', if_true: files('acpi-stub.c',
> >> 'aml-build-stub.c',
> >> -                                                  'acpi-x86-stub.c',
> >> 'ipmi-stub.c'))
> >> +                                                  'acpi-x86-stub.c',
> >> 'ipmi-stub.c', 'ghes-stub.c'))
> >
> > Gosh that last line is confusing.  I see it's documented in
> > build-system.rst, but yeesh.
> >
> > Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>
> Yeah, it's vestigial of the makefiles and I should remove it.
>
> That said, here:
>
> > +acpi_ss.add(when: 'CONFIG_ACPI_APEI', if_true: files('ghes.c'), if_false:('ghes-stub.c'))
>
> There's a missing "files" after if_false.

Thanks for finding this bug -- I have requeued the patches
to target-arm.next with the "files" added.

-- PMM

Re: [PATCH 1/3] hw/acpi: Provide stub version of acpi_ghes_record_errors()

[Dongjiu Geng]

On Fri, 4 Jun 2021 at 01:13, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> Generic code in target/arm wants to call acpi_ghes_record_errors();
> provide a stub version so that we don't fail to link when
> CONFIG_ACPI_APEI is not set. This requires us to add a new
> ghes-stub.c file to contain it and the meson.build mechanics
> to use it when appropriate.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/acpi/ghes-stub.c | 17 +++++++++++++++++
>  hw/acpi/meson.build |  6 +++---
>  2 files changed, 20 insertions(+), 3 deletions(-)
>  create mode 100644 hw/acpi/ghes-stub.c
>
> diff --git a/hw/acpi/ghes-stub.c b/hw/acpi/ghes-stub.c
> new file mode 100644
> index 00000000000..9faba043b85
> --- /dev/null
> +++ b/hw/acpi/ghes-stub.c
> @@ -0,0 +1,17 @@
> +/*
> + * Support for generating APEI tables and recording CPER for Guests:
> + * stub functions.
> + *
> + * Copyright (c) 2021 Linaro, Ltd
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "hw/acpi/ghes.h"
> +
> +int acpi_ghes_record_errors(uint8_t source_id, uint64_t physical_address)
> +{
> +    return -1;
> +}
> diff --git a/hw/acpi/meson.build b/hw/acpi/meson.build
> index dd69577212a..03ea43f8627 100644
> --- a/hw/acpi/meson.build
> +++ b/hw/acpi/meson.build
> @@ -13,13 +13,13 @@ acpi_ss.add(when: 'CONFIG_ACPI_PCI', if_true: files('pci.c'))
>  acpi_ss.add(when: 'CONFIG_ACPI_VMGENID', if_true: files('vmgenid.c'))
>  acpi_ss.add(when: 'CONFIG_ACPI_HW_REDUCED', if_true: files('generic_event_device.c'))
>  acpi_ss.add(when: 'CONFIG_ACPI_HMAT', if_true: files('hmat.c'))
> -acpi_ss.add(when: 'CONFIG_ACPI_APEI', if_true: files('ghes.c'))
> +acpi_ss.add(when: 'CONFIG_ACPI_APEI', if_true: files('ghes.c'), if_false:('ghes-stub.c'))
>  acpi_ss.add(when: 'CONFIG_ACPI_X86', if_true: files('core.c', 'piix4.c', 'pcihp.c'), if_false: files('acpi-stub.c'))
>  acpi_ss.add(when: 'CONFIG_ACPI_X86_ICH', if_true: files('ich9.c', 'tco.c'))
>  acpi_ss.add(when: 'CONFIG_IPMI', if_true: files('ipmi.c'), if_false: files('ipmi-stub.c'))
>  acpi_ss.add(when: 'CONFIG_PC', if_false: files('acpi-x86-stub.c'))
>  acpi_ss.add(when: 'CONFIG_TPM', if_true: files('tpm.c'))
> -softmmu_ss.add(when: 'CONFIG_ACPI', if_false: files('acpi-stub.c', 'aml-build-stub.c'))
> +softmmu_ss.add(when: 'CONFIG_ACPI', if_false: files('acpi-stub.c', 'aml-build-stub.c', 'ghes-stub.c'))
>  softmmu_ss.add_all(when: 'CONFIG_ACPI', if_true: acpi_ss)
>  softmmu_ss.add(when: 'CONFIG_ALL', if_true: files('acpi-stub.c', 'aml-build-stub.c',
> -                                                  'acpi-x86-stub.c', 'ipmi-stub.c'))
> +                                                  'acpi-x86-stub.c', 'ipmi-stub.c', 'ghes-stub.c'))
> --
> 2.20.1
>

Reviewed-by: Dongjiu Geng <gengdongjiu1@gmail.com>

[PATCH 2/3] hw/acpi: Provide function acpi_ghes_present()

[Peter Maydell]

Allow code elsewhere in the system to check whether the ACPI GHES
table is present, so it can determine whether it is OK to try to
record an error by calling acpi_ghes_record_errors().

(We don't need to migrate the new 'present' field in AcpiGhesState,
because it is set once at system initialization and doesn't change.)

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/hw/acpi/ghes.h |  9 +++++++++
 hw/acpi/ghes-stub.c    |  5 +++++
 hw/acpi/ghes.c         | 17 +++++++++++++++++
 3 files changed, 31 insertions(+)

diff --git a/include/hw/acpi/ghes.h b/include/hw/acpi/ghes.h
index 2ae8bc1ded3..674f6958e90 100644
--- a/include/hw/acpi/ghes.h
+++ b/include/hw/acpi/ghes.h
@@ -64,6 +64,7 @@ enum {
 
 typedef struct AcpiGhesState {
     uint64_t ghes_addr_le;
+    bool present; /* True if GHES is present at all on this board */
 } AcpiGhesState;
 
 void build_ghes_error_table(GArray *hardware_errors, BIOSLinker *linker);
@@ -72,4 +73,12 @@ void acpi_build_hest(GArray *table_data, BIOSLinker *linker,
 void acpi_ghes_add_fw_cfg(AcpiGhesState *vms, FWCfgState *s,
                           GArray *hardware_errors);
 int acpi_ghes_record_errors(uint8_t notify, uint64_t error_physical_addr);
+
+/**
+ * acpi_ghes_present: Report whether ACPI GHES table is present
+ *
+ * Returns: true if the system has an ACPI GHES table and it is
+ * safe to call acpi_ghes_record_errors() to record a memory error.
+ */
+bool acpi_ghes_present(void);
 #endif
diff --git a/hw/acpi/ghes-stub.c b/hw/acpi/ghes-stub.c
index 9faba043b85..c315de1802d 100644
--- a/hw/acpi/ghes-stub.c
+++ b/hw/acpi/ghes-stub.c
@@ -15,3 +15,8 @@ int acpi_ghes_record_errors(uint8_t source_id, uint64_t physical_address)
 {
     return -1;
 }
+
+bool acpi_ghes_present(void)
+{
+    return false;
+}
diff --git a/hw/acpi/ghes.c b/hw/acpi/ghes.c
index a4dac6bf15e..a749b84d624 100644
--- a/hw/acpi/ghes.c
+++ b/hw/acpi/ghes.c
@@ -386,6 +386,8 @@ void acpi_ghes_add_fw_cfg(AcpiGhesState *ags, FWCfgState *s,
     /* 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);
+
+    ags->present = true;
 }
 
 int acpi_ghes_record_errors(uint8_t source_id, uint64_t physical_address)
@@ -443,3 +445,18 @@ int acpi_ghes_record_errors(uint8_t source_id, uint64_t physical_address)
 
     return ret;
 }
+
+bool acpi_ghes_present(void)
+{
+    AcpiGedState *acpi_ged_state;
+    AcpiGhesState *ags;
+
+    acpi_ged_state = ACPI_GED(object_resolve_path_type("", TYPE_ACPI_GED,
+                                                       NULL));
+
+    if (!acpi_ged_state) {
+        return false;
+    }
+    ags = &acpi_ged_state->ghes_state;
+    return ags->present;
+}
-- 
2.20.1

Re: [PATCH 2/3] hw/acpi: Provide function acpi_ghes_present()

[Richard Henderson]

On 6/3/21 10:12 AM, Peter Maydell wrote:
> Allow code elsewhere in the system to check whether the ACPI GHES
> table is present, so it can determine whether it is OK to try to
> record an error by calling acpi_ghes_record_errors().
> 
> (We don't need to migrate the new 'present' field in AcpiGhesState,
> because it is set once at system initialization and doesn't change.)
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   include/hw/acpi/ghes.h |  9 +++++++++
>   hw/acpi/ghes-stub.c    |  5 +++++
>   hw/acpi/ghes.c         | 17 +++++++++++++++++
>   3 files changed, 31 insertions(+)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 2/3] hw/acpi: Provide function acpi_ghes_present()

[Dongjiu Geng]

On Fri, 4 Jun 2021 at 01:13, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> Allow code elsewhere in the system to check whether the ACPI GHES
> table is present, so it can determine whether it is OK to try to
> record an error by calling acpi_ghes_record_errors().
>
> (We don't need to migrate the new 'present' field in AcpiGhesState,
> because it is set once at system initialization and doesn't change.)
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  include/hw/acpi/ghes.h |  9 +++++++++
>  hw/acpi/ghes-stub.c    |  5 +++++
>  hw/acpi/ghes.c         | 17 +++++++++++++++++
>  3 files changed, 31 insertions(+)
>
> diff --git a/include/hw/acpi/ghes.h b/include/hw/acpi/ghes.h
> index 2ae8bc1ded3..674f6958e90 100644
> --- a/include/hw/acpi/ghes.h
> +++ b/include/hw/acpi/ghes.h
> @@ -64,6 +64,7 @@ enum {
>
>  typedef struct AcpiGhesState {
>      uint64_t ghes_addr_le;
> +    bool present; /* True if GHES is present at all on this board */
>  } AcpiGhesState;
>
>  void build_ghes_error_table(GArray *hardware_errors, BIOSLinker *linker);
> @@ -72,4 +73,12 @@ void acpi_build_hest(GArray *table_data, BIOSLinker *linker,
>  void acpi_ghes_add_fw_cfg(AcpiGhesState *vms, FWCfgState *s,
>                            GArray *hardware_errors);
>  int acpi_ghes_record_errors(uint8_t notify, uint64_t error_physical_addr);
> +
> +/**
> + * acpi_ghes_present: Report whether ACPI GHES table is present
> + *
> + * Returns: true if the system has an ACPI GHES table and it is
> + * safe to call acpi_ghes_record_errors() to record a memory error.
> + */
> +bool acpi_ghes_present(void);
>  #endif
> diff --git a/hw/acpi/ghes-stub.c b/hw/acpi/ghes-stub.c
> index 9faba043b85..c315de1802d 100644
> --- a/hw/acpi/ghes-stub.c
> +++ b/hw/acpi/ghes-stub.c
> @@ -15,3 +15,8 @@ int acpi_ghes_record_errors(uint8_t source_id, uint64_t physical_address)
>  {
>      return -1;
>  }
> +
> +bool acpi_ghes_present(void)
> +{
> +    return false;
> +}
> diff --git a/hw/acpi/ghes.c b/hw/acpi/ghes.c
> index a4dac6bf15e..a749b84d624 100644
> --- a/hw/acpi/ghes.c
> +++ b/hw/acpi/ghes.c
> @@ -386,6 +386,8 @@ void acpi_ghes_add_fw_cfg(AcpiGhesState *ags, FWCfgState *s,
>      /* 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);
> +
> +    ags->present = true;
>  }
>
>  int acpi_ghes_record_errors(uint8_t source_id, uint64_t physical_address)
> @@ -443,3 +445,18 @@ int acpi_ghes_record_errors(uint8_t source_id, uint64_t physical_address)
>
>      return ret;
>  }
> +
> +bool acpi_ghes_present(void)
> +{
> +    AcpiGedState *acpi_ged_state;
> +    AcpiGhesState *ags;
> +
> +    acpi_ged_state = ACPI_GED(object_resolve_path_type("", TYPE_ACPI_GED,
> +                                                       NULL));
> +
> +    if (!acpi_ged_state) {
> +        return false;
> +    }
> +    ags = &acpi_ged_state->ghes_state;
> +    return ags->present;
> +}
> --
> 2.20.1
>

Reviewed-by: Dongjiu Geng <gengdongjiu1@gmail.com>

[PATCH 3/3] target/arm: Use acpi_ghes_present() to see if we report ACPI memory errors

[Peter Maydell]

The virt_is_acpi_enabled() function is specific to the virt board, as
is the check for its 'ras' property.  Use the new acpi_ghes_present()
function to check whether we should report memory errors via
acpi_ghes_record_errors().

This avoids a link error if QEMU was built without support for the
virt board, and provides a mechanism that can be used by any future
board models that want to add ACPI memory error reporting support
(they only need to call acpi_ghes_add_fw_cfg()).

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/kvm64.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
index 37ceadd9a9d..59982d470d3 100644
--- a/target/arm/kvm64.c
+++ b/target/arm/kvm64.c
@@ -1410,14 +1410,10 @@ void kvm_arch_on_sigbus_vcpu(CPUState *c, int code, void *addr)
 {
     ram_addr_t ram_addr;
     hwaddr paddr;
-    Object *obj = qdev_get_machine();
-    VirtMachineState *vms = VIRT_MACHINE(obj);
-    bool acpi_enabled = virt_is_acpi_enabled(vms);
 
     assert(code == BUS_MCEERR_AR || code == BUS_MCEERR_AO);
 
-    if (acpi_enabled && addr &&
-            object_property_get_bool(obj, "ras", NULL)) {
+    if (acpi_ghes_present() && addr) {
         ram_addr = qemu_ram_addr_from_host(addr);
         if (ram_addr != RAM_ADDR_INVALID &&
             kvm_physical_memory_addr_from_host(c->kvm_state, addr, &paddr)) {
-- 
2.20.1

Re: [PATCH 3/3] target/arm: Use acpi_ghes_present() to see if we report ACPI memory errors

[Richard Henderson]

On 6/3/21 10:12 AM, Peter Maydell wrote:
> The virt_is_acpi_enabled() function is specific to the virt board, as
> is the check for its 'ras' property.  Use the new acpi_ghes_present()
> function to check whether we should report memory errors via
> acpi_ghes_record_errors().
> 
> This avoids a link error if QEMU was built without support for the
> virt board, and provides a mechanism that can be used by any future
> board models that want to add ACPI memory error reporting support
> (they only need to call acpi_ghes_add_fw_cfg()).
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   target/arm/kvm64.c | 6 +-----
>   1 file changed, 1 insertion(+), 5 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 3/3] target/arm: Use acpi_ghes_present() to see if we report ACPI memory errors

[Dongjiu Geng]

On Fri, 4 Jun 2021 at 01:13, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> The virt_is_acpi_enabled() function is specific to the virt board, as
> is the check for its 'ras' property.  Use the new acpi_ghes_present()
> function to check whether we should report memory errors via
> acpi_ghes_record_errors().
>
> This avoids a link error if QEMU was built without support for the
> virt board, and provides a mechanism that can be used by any future
> board models that want to add ACPI memory error reporting support
> (they only need to call acpi_ghes_add_fw_cfg()).
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/kvm64.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
> index 37ceadd9a9d..59982d470d3 100644
> --- a/target/arm/kvm64.c
> +++ b/target/arm/kvm64.c
> @@ -1410,14 +1410,10 @@ void kvm_arch_on_sigbus_vcpu(CPUState *c, int code, void *addr)
>  {
>      ram_addr_t ram_addr;
>      hwaddr paddr;
> -    Object *obj = qdev_get_machine();
> -    VirtMachineState *vms = VIRT_MACHINE(obj);
> -    bool acpi_enabled = virt_is_acpi_enabled(vms);
>
>      assert(code == BUS_MCEERR_AR || code == BUS_MCEERR_AO);
>
> -    if (acpi_enabled && addr &&
> -            object_property_get_bool(obj, "ras", NULL)) {
> +    if (acpi_ghes_present() && addr) {
>          ram_addr = qemu_ram_addr_from_host(addr);
>          if (ram_addr != RAM_ADDR_INVALID &&
>              kvm_physical_memory_addr_from_host(c->kvm_state, addr, &paddr)) {
> --
> 2.20.1
>

Reviewed-by: Dongjiu Geng <gengdongjiu1@gmail.com>

Re: [PATCH 0/3] hw/acpi, arm: Provide and use acpi_ghes_present()

[Peter Maydell]

On Thu, 3 Jun 2021 at 18:13, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> Currently the Arm code in target/arm/kvm64.c which decides whether
> it should report memory errors via the ACPI GHES table works
> only with the 'virt' board; in fact it won't even link if the
> virt board is configured out of the QEMU binary.
>
> This patchset replaces those virt-specific checks with a single
> new acpi_ghes_present() function which tells the caller whether
> it's OK to report errors by calling acpi_ghes_record_errors().
> The mechanism we use is a simple flag in the AcpiGhesState
> which gets sent if the board calls acpi_ghes_add_fw_cfg() to
> set up the GHES stuff.
>
> We need also to provide 'stub' versions of both acpi_ghes_present()
> and acpi_ghes_record_errors() so that we can link even if no
> board using ACPI GHES has been configured into the binary.
>
> (You can test that this series is necessary by commenting out the
> 'CONFIG_ARM_VIRT=y' line in default-configs/devices/arm-softmmu.mak
> and building with KVM enabled on an AArch64 host.)
>
> I have tested with 'make' and 'make check' but nothing beyond
> that; testing by somebody who has a guest setup that uses GHES
> would be helpful just to check I haven't accidentally broken it.

Thanks for the review; I've applied this to target-arm.next.

-- PMM