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

[Swetha Joshi <swethajoshi139@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3688 bytes --]

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.

[-- Attachment #2: Type: text/html, Size: 4931 bytes --]