From: David Gibson <david@gibson.dropbear.id.au>
To: Aravinda Prasad <aravinda@linux.vnet.ibm.com>
Cc: aik@au1.ibm.com, qemu-devel@nongnu.org, groug@kaod.org,
paulus@ozlabs.org, qemu-ppc@nongnu.org
Subject: Re: [Qemu-devel] [PATCH v10 2/6] ppc: spapr: Introduce FWNMI capability
Date: Fri, 5 Jul 2019 11:07:55 +1000 [thread overview]
Message-ID: <20190705010755.GA3266@umbus.fritz.box> (raw)
In-Reply-To: <d5c75742-4f45-2f26-3837-098e8710ccd9@linux.vnet.ibm.com>
[-- Attachment #1: Type: text/plain, Size: 6766 bytes --]
On Thu, Jul 04, 2019 at 10:33:11AM +0530, Aravinda Prasad wrote:
>
>
> On Thursday 04 July 2019 06:37 AM, David Gibson wrote:
> > On Wed, Jul 03, 2019 at 02:58:24PM +0530, Aravinda Prasad wrote:
> >>
> >>
> >> On Wednesday 03 July 2019 08:33 AM, David Gibson wrote:
> >>> On Tue, Jul 02, 2019 at 11:54:26AM +0530, Aravinda Prasad wrote:
> >>>>
> >>>>
> >>>> On Tuesday 02 July 2019 09:21 AM, David Gibson wrote:
> >>>>> On Wed, Jun 12, 2019 at 02:51:04PM +0530, Aravinda Prasad wrote:
> >>>>>> Introduce the KVM capability KVM_CAP_PPC_FWNMI so that
> >>>>>> the KVM causes guest exit with NMI as exit reason
> >>>>>> when it encounters a machine check exception on the
> >>>>>> address belonging to a guest. Without this capability
> >>>>>> enabled, KVM redirects machine check exceptions to
> >>>>>> guest's 0x200 vector.
> >>>>>>
> >>>>>> This patch also introduces fwnmi-mce capability to
> >>>>>> deal with the case when a guest with the
> >>>>>> KVM_CAP_PPC_FWNMI capability enabled is attempted
> >>>>>> to migrate to a host that does not support this
> >>>>>> capability.
> >>>>>>
> >>>>>> Signed-off-by: Aravinda Prasad <aravinda@linux.vnet.ibm.com>
> >>>>>> ---
> >>>>>> hw/ppc/spapr.c | 1 +
> >>>>>> hw/ppc/spapr_caps.c | 26 ++++++++++++++++++++++++++
> >>>>>> include/hw/ppc/spapr.h | 4 +++-
> >>>>>> target/ppc/kvm.c | 19 +++++++++++++++++++
> >>>>>> target/ppc/kvm_ppc.h | 12 ++++++++++++
> >>>>>> 5 files changed, 61 insertions(+), 1 deletion(-)
> >>>>>>
> >>>>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> >>>>>> index 6dd8aaa..2ef86aa 100644
> >>>>>> --- a/hw/ppc/spapr.c
> >>>>>> +++ b/hw/ppc/spapr.c
> >>>>>> @@ -4360,6 +4360,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
> >>>>>> smc->default_caps.caps[SPAPR_CAP_NESTED_KVM_HV] = SPAPR_CAP_OFF;
> >>>>>> smc->default_caps.caps[SPAPR_CAP_LARGE_DECREMENTER] = SPAPR_CAP_ON;
> >>>>>> smc->default_caps.caps[SPAPR_CAP_CCF_ASSIST] = SPAPR_CAP_OFF;
> >>>>>> + smc->default_caps.caps[SPAPR_CAP_FWNMI_MCE] = SPAPR_CAP_OFF;
> >>>>>> spapr_caps_add_properties(smc, &error_abort);
> >>>>>> smc->irq = &spapr_irq_dual;
> >>>>>> smc->dr_phb_enabled = true;
> >>>>>> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
> >>>>>> index 31b4661..2e92eb6 100644
> >>>>>> --- a/hw/ppc/spapr_caps.c
> >>>>>> +++ b/hw/ppc/spapr_caps.c
> >>>>>> @@ -479,6 +479,22 @@ static void cap_ccf_assist_apply(SpaprMachineState *spapr, uint8_t val,
> >>>>>> }
> >>>>>> }
> >>>>>>
> >>>>>> +static void cap_fwnmi_mce_apply(SpaprMachineState *spapr, uint8_t val,
> >>>>>> + Error **errp)
> >>>>>> +{
> >>>>>> + if (!val) {
> >>>>>> + return; /* Disabled by default */
> >>>>>> + }
> >>>>>> +
> >>>>>> + if (tcg_enabled()) {
> >>>>>> + error_setg(errp,
> >>>>>> +"No Firmware Assisted Non-Maskable Interrupts support in TCG, try cap-fwnmi-mce=off");
> >>>>>
> >>>>> Not allowing this for TCG creates an awkward incompatibility between
> >>>>> KVM and TCG guests. I can't actually see any reason to ban it for TCG
> >>>>> - with the current code TCG won't ever generate NMIs, but I don't see
> >>>>> that anything will actually break.
> >>>>>
> >>>>> In fact, we do have an nmi monitor command, currently wired to the
> >>>>> spapr_nmi() function which resets each cpu, but it probably makes
> >>>>> sense to wire it up to the fwnmi stuff when present.
> >>>>
> >>>> Yes, but that nmi support is not enough to inject a synchronous error
> >>>> into the guest kernel. For example, we should provide the faulty address
> >>>> along with other information such as the type of error (slb multi-hit,
> >>>> memory error, TLB multi-hit) and when the error occurred (load/store)
> >>>> and whether the error was completely recovered or not. Without such
> >>>> information we cannot build the error log and pass it on to the guest
> >>>> kernel. Right now nmi monitor command takes cpu number as the only argument.
> >>>
> >>> Obviously we can't inject an arbitrary MCE event with that monitor
> >>> command. But isn't there some sort of catch-all / unknown type of MCE
> >>> event which we could inject?
> >>
> >> We have "unknown" type of error, but we should also pass an address in
> >> the MCE event log. Strictly speaking this address should be a valid
> >> address in the current CPU context as MCEs are synchronous errors
> >> triggered when we touch a bad address.
> >
> > Well, some of them are. At least historically both synchronous and
> > asnchronous MCEs were possible. Are there really no versions where
> > you can report an MCE with unknown address?
>
> I am not aware of any such versions. Will cross check.
>
> >
> >> We can pass a default address with every nmi, but I am not sure whether
> >> that will be practically helpful.
> >>
> >>> It seems very confusing to me to have 2 totally separate "nmi"
> >>> mechanisms.
> >>>
> >>>> So I think TCG support should be a separate patch by itself.
> >>>
> >>> Even if we don't wire up the monitor command, I still don't see
> >>> anything that this patch breaks - we can support the nmi-register and
> >>> nmi-interlock calls without ever actually creating MCE events.
> >>
> >> If we support nmi-register and nmi-interlock calls without the monitor
> >> command wire-up then we will be falsely claiming the nmi support to the
> >> guest while it is not actually supported.
> >
> > How so? AFAICT, from the point of view of the guest this is not
> > observably different from supporting the NMI mechanism but NMIs never
> > occurring.
>
> A guest inserting a duplicate SLB will expect the machine check
> exception delivered to the handler registered via nmi,register.
> But we actually don't do that in TCG.
Ah, true, I was thinking of external hardware fault triggered MCEs
rather than software error ones like duplicate SLB.
That said, I strongly suspect TCG is buggy enough at present that
exact behaviour in rare error conditions like duplicate SLB is not
really a big problem in the scheme of things.
I really don't think we can enable this by default until we allow it
for TCG - we don't want starting a TCG guest to involve manually
switching other options.
We could consider allowing it for TCG but just printing a warning that
the behaviour may not be correct in some conditions - we do something
similar for some of the Spectre workarounds already.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
next prev parent reply other threads:[~2019-07-05 4:34 UTC|newest]
Thread overview: 37+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-06-12 9:20 [Qemu-devel] [PATCH v10 0/6] target-ppc/spapr: Add FWNMI support in QEMU for PowerKVM guests Aravinda Prasad
2019-06-12 9:20 ` [Qemu-devel] [PATCH v10 1/6] Wrapper function to wait on condition for the main loop mutex Aravinda Prasad
2019-06-12 9:21 ` [Qemu-devel] [PATCH v10 2/6] ppc: spapr: Introduce FWNMI capability Aravinda Prasad
2019-07-02 3:51 ` David Gibson
2019-07-02 6:24 ` Aravinda Prasad
2019-07-03 3:03 ` David Gibson
2019-07-03 9:28 ` Aravinda Prasad
2019-07-04 1:07 ` David Gibson
2019-07-04 5:03 ` Aravinda Prasad
2019-07-05 1:07 ` David Gibson [this message]
2019-07-05 11:19 ` Aravinda Prasad
2019-07-05 13:23 ` [Qemu-devel] [Qemu-ppc] " Greg Kurz
2019-07-08 10:26 ` Aravinda Prasad
2019-06-12 9:21 ` [Qemu-devel] [PATCH v10 3/6] target/ppc: Handle NMI guest exit Aravinda Prasad
2019-07-02 3:52 ` David Gibson
2019-06-12 9:21 ` [Qemu-devel] [PATCH v10 4/6] target/ppc: Build rtas error log upon an MCE Aravinda Prasad
2019-07-02 4:03 ` David Gibson
2019-07-02 9:49 ` Aravinda Prasad
2019-07-03 3:07 ` David Gibson
2019-07-03 7:16 ` Aravinda Prasad
2019-06-12 9:21 ` [Qemu-devel] [PATCH v10 5/6] migration: Include migration support for machine check handling Aravinda Prasad
2019-06-12 9:21 ` [Qemu-devel] [PATCH v10 6/6] ppc: spapr: Handle "ibm, nmi-register" and "ibm, nmi-interlock" RTAS calls Aravinda Prasad
2019-06-24 14:29 ` Greg Kurz
2019-06-25 6:16 ` Aravinda Prasad
2019-06-25 7:00 ` Greg Kurz
2019-06-26 5:13 ` [Qemu-devel] [Qemu-ppc] " Aravinda Prasad
2019-07-02 4:12 ` David Gibson
2019-07-02 4:11 ` [Qemu-devel] " David Gibson
2019-07-02 10:40 ` Aravinda Prasad
2019-07-03 3:20 ` David Gibson
2019-07-03 9:00 ` Aravinda Prasad
2019-07-04 1:12 ` David Gibson
2019-07-04 5:19 ` Aravinda Prasad
2019-07-04 18:37 ` [Qemu-devel] [Qemu-ppc] " Greg Kurz
2019-07-05 11:11 ` Aravinda Prasad
2019-07-05 12:35 ` Greg Kurz
2019-07-09 6:41 ` [Qemu-devel] " David Gibson
Reply instructions:
You may reply publicly 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=20190705010755.GA3266@umbus.fritz.box \
--to=david@gibson.dropbear.id.au \
--cc=aik@au1.ibm.com \
--cc=aravinda@linux.vnet.ibm.com \
--cc=groug@kaod.org \
--cc=paulus@ozlabs.org \
--cc=qemu-devel@nongnu.org \
--cc=qemu-ppc@nongnu.org \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).