qemu-devel
[Top][All Lists]
Advanced
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v17 5/7] ppc: spapr: Handle "ibm,nmi-register" and "ibm,nmi-i

From: Ganesh
Subject: Re: [PATCH v17 5/7] ppc: spapr: Handle "ibm,nmi-register" and "ibm,nmi-interlock" RTAS calls
Date: Wed, 11 Dec 2019 19:20:16 +0530
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.3.1 

On 12/5/19 10:25 AM, Ganesh wrote:


On 11/19/19 8:09 AM, David Gibson wrote:

On Thu, Oct 24, 2019 at 01:13:05PM +0530, Ganesh Goudar wrote:

From: Aravinda Prasad <address@hidden>

This patch adds support in QEMU to handle "ibm,nmi-register"
and "ibm,nmi-interlock" RTAS calls.

The machine check notification address is saved when the
OS issues "ibm,nmi-register" RTAS call.

This patch also handles the case when multiple processors
experience machine check at or about the same time by
handling "ibm,nmi-interlock" call. In such cases, as per
PAPR, subsequent processors serialize waiting for the first
processor to issue the "ibm,nmi-interlock" call. The second
processor that also received a machine check error waits
till the first processor is done reading the error log.
The first processor issues "ibm,nmi-interlock" call
when the error log is consumed.

[Move fwnmi registeration to .apply hook]

s/registeration/registration/

Thanks



Signed-off-by: Ganesh Goudar <address@hidden>
Signed-off-by: Aravinda Prasad <address@hidden>
---
  hw/ppc/spapr_caps.c    |  9 +++++--
  hw/ppc/spapr_rtas.c    | 57 ++++++++++++++++++++++++++++++++++++++++++ 
  include/hw/ppc/spapr.h |  5 +++-
  3 files changed, 68 insertions(+), 3 deletions(-)

diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
index 976d709210..1675ebd45e 100644
--- a/hw/ppc/spapr_caps.c
+++ b/hw/ppc/spapr_caps.c
@@ -509,9 +509,14 @@ static void cap_fwnmi_mce_apply(SpaprMachineState *spapr, uint8_t val, 
           * of software injected faults like duplicate SLBs).
           */
          warn_report("Firmware Assisted Non-Maskable Interrupts not supported in TCG"); 
This logic still isn't quite right.  To start with the warn_report()
above possible wants to be more weakly worded.  With TCG, FWNMI won't
generally *do* anything, and there are some edge cases where the
behaviour is arguably incorrect.  However there's no reason we can't
make the RTAS calls work basically as expected and in almost all cases
things will behave correctly - at least according to the case where no
fwnmi events are delivered...

ok



-    } else if (kvm_enabled() && (kvmppc_set_fwnmi() != 0)) {
-        error_setg(errp,
+    } else if (kvm_enabled()) {
+        if (!kvmppc_set_fwnmi()) {
+            /* Register ibm,nmi-register and ibm,nmi-interlock RTAS calls */ 
+            spapr_fwnmi_register();

..but here you only register the RTAS calls in the KVM case, which
breaks that.  If there really is a strong reason to do this, then the
warn_report() above should be error_setg() and fail the apply.
Also I found a side effect of moving this fwnmi registration to apply hook, I see the following assert
when I reboot the guest. may be I must have a member to indicate if the registration is already done. 

Requesting system reboot
[  186.368745] sd 0:0:0:0: [sda] Synchronizing SCSI cache
[  186.370222] reboot: Restarting system
#######MACHINE RESET######
#####SPAPR_FWNMI_REGISTER######
qemu-system-ppc64: /home/jupit/qemu/hw/ppc/spapr_rtas.c:510: spapr_rtas_register: Assertion `!name || !rtas_table[token].name' failed. 
Aborted




+        } else {
+            error_setg(errp,
  "Firmware Assisted Non-Maskable Interrupts not supported by KVM, try cap-fwnmi-mce=off"); 
+        }
      }
  }
  diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
index 2c066a372d..0328b1f341 100644
--- a/hw/ppc/spapr_rtas.c
+++ b/hw/ppc/spapr_rtas.c
@@ -400,6 +400,55 @@ static void rtas_get_power_level(PowerPCCPU *cpu, SpaprMachineState *spapr, 
      rtas_st(rets, 1, 100);
  }
  +static void rtas_ibm_nmi_register(PowerPCCPU *cpu,
+                                  SpaprMachineState *spapr,
+                                  uint32_t token, uint32_t nargs,
+                                  target_ulong args,
+                                  uint32_t nret, target_ulong rets)
+{
+    hwaddr rtas_addr = spapr_get_rtas_addr();
+
+    if (!rtas_addr) {
+        rtas_st(rets, 0, RTAS_OUT_NOT_SUPPORTED);
+        return;
+    }
+
+    if (spapr_get_cap(spapr, SPAPR_CAP_FWNMI_MCE) == SPAPR_CAP_OFF) {
+        rtas_st(rets, 0, RTAS_OUT_NOT_SUPPORTED);

Actually, since you explicitly test for the cap being enabled here,
there's no reason not to *always* register this RTAS call.  Also this
test for the feature flag should go first, before delving into the
device tree for the RTAS address.

Sure, will do



+        return;
+    }
+
+    spapr->guest_machine_check_addr = rtas_ld(args, 1);
+    rtas_st(rets, 0, RTAS_OUT_SUCCESS);
+}
+
+static void rtas_ibm_nmi_interlock(PowerPCCPU *cpu,
+                                   SpaprMachineState *spapr,
+                                   uint32_t token, uint32_t nargs,
+                                   target_ulong args,
+                                   uint32_t nret, target_ulong rets)
+{
+    if (spapr->guest_machine_check_addr == -1) {
+        /* NMI register not called */
+        rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
+        return;
+    }
+
+    if (spapr->mc_status != cpu->vcpu_id) {
+        /* The vCPU that hit the NMI should invoke "ibm,nmi-interlock" */ 
+        rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
+        return;
+    }
+
+    /*
+     * vCPU issuing "ibm,nmi-interlock" is done with NMI handling,
+     * hence unset mc_status.
+     */
+    spapr->mc_status = -1;
+    qemu_cond_signal(&spapr->mc_delivery_cond);
+    rtas_st(rets, 0, RTAS_OUT_SUCCESS);
+}
+
  static struct rtas_call {
      const char *name;
      spapr_rtas_fn fn;
@@ -503,6 +552,14 @@ hwaddr spapr_get_rtas_addr(void)
      return (hwaddr)fdt32_to_cpu(*rtas_data);
  }
  +void spapr_fwnmi_register(void)
+{
+    spapr_rtas_register(RTAS_IBM_NMI_REGISTER, "ibm,nmi-register",
+                        rtas_ibm_nmi_register);
+    spapr_rtas_register(RTAS_IBM_NMI_INTERLOCK, "ibm,nmi-interlock",
+                        rtas_ibm_nmi_interlock);
+}
+
  static void core_rtas_register_types(void)
  {
      spapr_rtas_register(RTAS_DISPLAY_CHARACTER, "display-character",
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 4afa8d4d09..86f0fc8fdd 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -653,8 +653,10 @@ target_ulong spapr_hypercall(PowerPCCPU *cpu, target_ulong opcode, 
  #define RTAS_IBM_REMOVE_PE_DMA_WINDOW (RTAS_TOKEN_BASE + 0x28)
  #define RTAS_IBM_RESET_PE_DMA_WINDOW (RTAS_TOKEN_BASE + 0x29)
  #define RTAS_IBM_SUSPEND_ME (RTAS_TOKEN_BASE + 0x2A)
+#define RTAS_IBM_NMI_REGISTER (RTAS_TOKEN_BASE + 0x2B)
+#define RTAS_IBM_NMI_INTERLOCK (RTAS_TOKEN_BASE + 0x2C)
  -#define RTAS_TOKEN_MAX (RTAS_TOKEN_BASE + 0x2B)
+#define RTAS_TOKEN_MAX (RTAS_TOKEN_BASE + 0x2D)
    /* RTAS ibm,get-system-parameter token values */
  #define RTAS_SYSPARM_SPLPAR_CHARACTERISTICS      20
@@ -907,4 +909,5 @@ void spapr_check_pagesize(SpaprMachineState *spapr, hwaddr pagesize, 
    void spapr_set_all_lpcrs(target_ulong value, target_ulong mask);
  hwaddr spapr_get_rtas_addr(void);
+void spapr_fwnmi_register(void);
  #endif /* HW_SPAPR_H */
reply via email to
[Prev in Thread] Current Thread [Next in Thread]