KVM Archive on lore.kernel.org
 help / color / Atom feed
From: Peter Maydell <peter.maydell@linaro.org>
To: Dongjiu Geng <gengdongjiu@huawei.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>,
	"Michael S. Tsirkin" <mst@redhat.com>,
	Igor Mammedov <imammedo@redhat.com>,
	Shannon Zhao <shannon.zhaosl@gmail.com>,
	Fam Zheng <fam@euphon.net>, Richard Henderson <rth@twiddle.net>,
	Eduardo Habkost <ehabkost@redhat.com>,
	Marcelo Tosatti <mtosatti@redhat.com>,
	"xuwei (O)" <xuwei5@huawei.com>,
	Jonathan Cameron <jonathan.cameron@huawei.com>,
	James Morse <james.morse@arm.com>,
	QEMU Developers <qemu-devel@nongnu.org>,
	kvm-devel <kvm@vger.kernel.org>, qemu-arm <qemu-arm@nongnu.org>,
	Zheng Xiang <zhengxiang9@huawei.com>,
	Linuxarm <linuxarm@huawei.com>
Subject: Re: [PATCH v22 8/9] target-arm: kvm64: handle SIGBUS signal from kernel or KVM
Date: Thu, 16 Jan 2020 16:28:34 +0000
Message-ID: <CAFEAcA_=PgkrWjwPxD89fCi85XPpcTHssXkSmE04Ctoj7AX0kA@mail.gmail.com> (raw)
In-Reply-To: <1578483143-14905-9-git-send-email-gengdongjiu@huawei.com>

On Wed, 8 Jan 2020 at 11:33, Dongjiu Geng <gengdongjiu@huawei.com> 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

  reply index

Thread overview: 27+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-01-08 11:32 [PATCH v22 0/9] Add ARMv8 RAS virtualization support in QEMU Dongjiu Geng
2020-01-08 11:32 ` [PATCH v22 1/9] hw/arm/virt: Introduce a RAS machine option Dongjiu Geng
2020-01-08 11:32 ` [PATCH v22 2/9] docs: APEI GHES generation and CPER record description Dongjiu Geng
2020-01-15 15:11   ` Igor Mammedov
2020-01-08 11:32 ` [PATCH v22 3/9] ACPI: Build related register address fields via hardware error fw_cfg blob Dongjiu Geng
2020-01-08 11:32 ` [PATCH v22 4/9] ACPI: Build Hardware Error Source Table Dongjiu Geng
2020-01-08 11:32 ` [PATCH v22 5/9] ACPI: Record the Generic Error Status Block address Dongjiu Geng
2020-01-16 16:44   ` Peter Maydell
2020-01-17 10:36     ` gengdongjiu
2020-01-17  7:39   ` Philippe Mathieu-Daudé
2020-01-17 10:47     ` gengdongjiu
2020-01-17 11:20       ` Philippe Mathieu-Daudé
2020-01-08 11:32 ` [PATCH v22 6/9] KVM: Move hwpoison page related functions into kvm-all.c Dongjiu Geng
2020-01-08 11:32 ` [PATCH v22 7/9] ACPI: Record Generic Error Status Block(GESB) table Dongjiu Geng
2020-01-08 11:32 ` [PATCH v22 8/9] target-arm: kvm64: handle SIGBUS signal from kernel or KVM Dongjiu Geng
2020-01-16 16:28   ` Peter Maydell [this message]
2020-01-16 16:40     ` Peter Maydell
2020-01-17 10:04     ` gengdongjiu
2020-01-08 11:32 ` [PATCH v22 9/9] MAINTAINERS: Add ACPI/HEST/GHES entries Dongjiu Geng
2020-01-16 16:46   ` Peter Maydell
2020-01-17  7:22     ` Philippe Mathieu-Daudé
2020-01-17 10:16       ` gengdongjiu
2020-01-17 11:09       ` Peter Maydell
2020-01-17 11:22         ` Philippe Mathieu-Daudé
2020-01-19  8:19           ` gengdongjiu
2020-01-17 12:19         ` Michael S. Tsirkin
2020-01-09  3:38 ` [PATCH v22 0/9] Add ARMv8 RAS virtualization support in QEMU gengdongjiu

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAFEAcA_=PgkrWjwPxD89fCi85XPpcTHssXkSmE04Ctoj7AX0kA@mail.gmail.com' \
    --to=peter.maydell@linaro.org \
    --cc=ehabkost@redhat.com \
    --cc=fam@euphon.net \
    --cc=gengdongjiu@huawei.com \
    --cc=imammedo@redhat.com \
    --cc=james.morse@arm.com \
    --cc=jonathan.cameron@huawei.com \
    --cc=kvm@vger.kernel.org \
    --cc=linuxarm@huawei.com \
    --cc=mst@redhat.com \
    --cc=mtosatti@redhat.com \
    --cc=pbonzini@redhat.com \
    --cc=qemu-arm@nongnu.org \
    --cc=qemu-devel@nongnu.org \
    --cc=rth@twiddle.net \
    --cc=shannon.zhaosl@gmail.com \
    --cc=xuwei5@huawei.com \
    --cc=zhengxiang9@huawei.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

KVM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/kvm/0 kvm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 kvm kvm/ https://lore.kernel.org/kvm \
		kvm@vger.kernel.org
	public-inbox-index kvm

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.kvm


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git