On Tue, 25 May 2021 at 04:21, Richard Henderson
<richard.hender...@linaro.org> wrote:
>
> On 5/24/21 7:58 PM, Swetha Joshi wrote:
> > Signed-off-by: Swetha Joshi <swethajoshi...@gmail.com>
> > ---
> > target/arm/kvm64.c | 12 ++++++++----
> > 1 file changed, 8 insertions(+), 4 deletions(-)
>
> You're still missing the commit message.
>
> >
> > diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
> > index dff85f6db9..47a4d9d831 100644
> > --- a/target/arm/kvm64.c
> > +++ b/target/arm/kvm64.c
> > @@ -1403,7 +1403,10 @@ void kvm_arch_on_sigbus_vcpu(CPUState *c, int code,
> > void *addr)
> > hwaddr paddr;
> > Object *obj = qdev_get_machine();
> > VirtMachineState *vms = VIRT_MACHINE(obj);
> > - bool acpi_enabled = virt_is_acpi_enabled(vms);
> > + bool acpi_enabled = false;
> > +#ifdef CONFIG_ARM_VIRT
> > + acpi_enabled = virt_is_acpi_enabled(vms);
> > +#endif /* CONFIG_ARM_VIRT */
> >
> > assert(code == BUS_MCEERR_AR || code == BUS_MCEERR_AO);
> >
> > @@ -1426,12 +1429,13 @@ void kvm_arch_on_sigbus_vcpu(CPUState *c, int code,
> > void *addr)
> > */
> > if (code == BUS_MCEERR_AR) {
> > kvm_cpu_synchronize_state(c);
> > - if (!acpi_ghes_record_errors(ACPI_HEST_SRC_ID_SEA, paddr))
> > {
> > - kvm_inject_arm_sea(c);
> > - } else {
> > +#ifdef CONFIG_ACPI_APEI
> > + if (acpi_ghes_record_errors(ACPI_HEST_SRC_ID_SEA, paddr)) {
> > error_report("failed to record the error");
> > abort();
> > }
> > +#endif /* CONFIG_ACPI_APEI */
> > + kvm_inject_arm_sea(c);
> > }
>
> Otherwise the actual patch looks ok.
I feel like the underlying problem here is that we let a virt-board-specific
bit of code slip into what should be generic Arm/KVM code here. We do
need to know "does the board actually have an ACPI table we can record the
memory error into", but that shouldn't be a specific query to virt board
code. I think (but have not tested) that a nicer solution would look like:
bool acpi_ghes_present(void)
{
return object_resolve_path_type("", TYPE_ACPI_GED, NULL) != NULL;
}
in hw/acpi/ghes.c, and then using that function instead of
virt_is_acpi_enabled().
That avoids the CONFIG_ARM_VIRT specific reference and the need for ifdefs,
and means that any future Arm board that wants to can support memory errors
via ACPI tables by creating the ACPI_GED device and setting up the ACPI
tables as virt does.
(You will also need: a stub "return false" version in a new ghes-stub.c file,
in an if_false clause in the meson.build line for ghes.c similar to the way
ipmi.c/ipmi-stub.c are done; a prototype in include/hw/acpi/ghes.h with a
doc-comment block documenting the function; possibly to add a stub for
acpi_ghes_record_errors() in the new ghes-stub.c.)
thanks
-- PMM
