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.mayd...@linaro.org>
wrote:
> 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
>
--
Regards
Swetha Joshi.
