Re: [PATCH_V3] Adding ifdefs to call the respective routines only when their configs are enabled

[Swetha Joshi]

Hey Peter, Phil,

Yeah like Peter mentioned, when KVM is enabled and we don't want VIRT enabled, there are a couple of routines that are being called from virt.h and ghes.h, which is resulting in errors. I came up with this simple fix but if you think there is a better solution to it I'll let you/ other developers who own it decide and fix it because I don't have much experience or visibility into what happens internally, my knowledge is restricted to just using the configs. 

Thank you for the feedback.

On Tue, May 25, 2021 at 2:05 AM Peter Maydell <peter.maydell@linaro.org> wrote:

On Tue, 25 May 2021 at 04:21, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 5/24/21 7:58 PM, Swetha Joshi wrote:
> > Signed-off-by: Swetha Joshi <swethajoshi139@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

--

Regards

Swetha Joshi.