Re: [PATCH v22 8/9] target-arm: kvm64: handle SIGBUS signal from kernel or KVM

[Peter Maydell]

On Wed, 8 Jan 2020 at 11:33, Dongjiu Geng <address@hidden> wrote:

> +void kvm_arch_on_sigbus_vcpu(CPUState *c, int code, void *addr)
> +{
> +    ram_addr_t ram_addr;
> +    hwaddr paddr;
> +
> +    assert(code == BUS_MCEERR_AR || code == BUS_MCEERR_AO);
> +
> +    if (acpi_enabled && addr &&
> +            object_property_get_bool(qdev_get_machine(), "ras", NULL)) {
> +        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)) {
> +            kvm_hwpoison_page_add(ram_addr);
> +            /*
> +             * Asynchronous signal will be masked by main thread, so
> +             * only handle synchronous signal.
> +             */

I don't understand this comment. (I think we've had discussions
about it before, but it's still not clear to me.)

This function (kvm_arch_on_sigbus_vcpu()) will be called in two contexts:

(1) in the vcpu thread:
  * the real SIGBUS handler sigbus_handler() sets a flag and arranges
    for an immediate vcpu exit
  * the vcpu thread reads the flag on exit from KVM_RUN and
    calls kvm_arch_on_sigbus_vcpu() directly
  * the error could be MCEERR_AR or MCEERR_AO
(2) MCE errors on other threads:
  * here SIGBUS is blocked, so MCEERR_AR (action-required)
    errors will cause the kernel to just kill the QEMU process
  * MCEERR_AO errors will be handled via the iothread's use
    of signalfd(), so kvm_on_sigbus() will get called from
    the main thread, and it will call kvm_arch_on_sigbus_vcpu()
  * in this case the passed in CPUState will (arbitrarily) be that
    for the first vCPU

For MCEERR_AR, the code here looks correct -- we know this is
only going to happen for the relevant vCPU so we can go ahead
and do the "record it in the ACPI table and tell the guest" bit.

But shouldn't we be doing something with the MCEERR_AO too?
That of course will be trickier because we're not necessarily
in the vcpu thread, but it would be nice to let the guest
know about it.

One comment that would work with the current code would be:

/*
 * If this is a BUS_MCEERR_AR, we know we have been called
 * synchronously from the vCPU thread, so we can easily
 * synchronize the state and inject an error.
 *
 * TODO: we currently don't tell the guest at all about BUS_MCEERR_AO.
 * In that case we might either be being called synchronously from
 * the vCPU thread, or a bit later from the main thread, so doing
 * the injection of the error would be more complicated.
 */

but I don't know if that's what you meant to say/implement...

> +            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 {
> +                    error_report("failed to record the error");
> +                    abort();
> +                }
> +            }
> +            return;
> +        }
> +        if (code == BUS_MCEERR_AO) {
> +            error_report("Hardware memory error at addr %p for memory used 
> by "
> +                "QEMU itself instead of guest system!", addr);
> +        }
> +    }
> +
> +    if (code == BUS_MCEERR_AR) {
> +        error_report("Hardware memory error!");
> +        exit(1);
> +    }
> +}
>

thanks
-- PMM