qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [for-5.0 0/4] spapr: Improvements to CAS feature negotiation
@ 2019-11-29  5:33 David Gibson
  2019-11-29  5:33 ` [for-5.0 1/4] spapr: Don't trigger a CAS reboot for XICS/XIVE mode changeover David Gibson
                   ` (4 more replies)
  0 siblings, 5 replies; 16+ messages in thread
From: David Gibson @ 2019-11-29  5:33 UTC (permalink / raw)
  To: groug, clg; +Cc: lvivier, David Gibson, qemu-ppc, mdroth, qemu-devel

This series contains several cleanups to the handling of the
ibm,client-architecture-support firmware call used for boot time
feature negotiation between the guest OS and the firmware &
hypervisor.

Mostly it's just internal polish, but one significant user visible
change is that we no longer generate an extra CAS reboot to switch
between XICS and XIVE interrupt modes (by far the most common cause of
CAS reboots in practice).

David Gibson (4):
  spapr: Don't trigger a CAS reboot for XICS/XIVE mode changeover
  spapr: Improve handling of fdt buffer size
  spapr: Fold h_cas_compose_response() into
    h_client_architecture_support()
  spapr: Simplify ovec diff

 hw/ppc/spapr.c              | 92 +++----------------------------------
 hw/ppc/spapr_hcall.c        | 90 +++++++++++++++++++++++++-----------
 hw/ppc/spapr_ovec.c         | 30 ++++--------
 include/hw/ppc/spapr.h      |  4 +-
 include/hw/ppc/spapr_ovec.h |  4 +-
 5 files changed, 83 insertions(+), 137 deletions(-)

-- 
2.23.0



^ permalink raw reply	[flat|nested] 16+ messages in thread

* [for-5.0 1/4] spapr: Don't trigger a CAS reboot for XICS/XIVE mode changeover
  2019-11-29  5:33 [for-5.0 0/4] spapr: Improvements to CAS feature negotiation David Gibson
@ 2019-11-29  5:33 ` David Gibson
  2019-12-02  7:03   ` Cédric Le Goater
  2019-12-02  7:50   ` Greg Kurz
  2019-11-29  5:33 ` [for-5.0 2/4] spapr: Improve handling of fdt buffer size David Gibson
                   ` (3 subsequent siblings)
  4 siblings, 2 replies; 16+ messages in thread
From: David Gibson @ 2019-11-29  5:33 UTC (permalink / raw)
  To: groug, clg; +Cc: lvivier, David Gibson, qemu-ppc, mdroth, qemu-devel

PAPR allows the interrupt controller used on a POWER9 machine (XICS or
XIVE) to be selected by the guest operating system, by using the
ibm,client-architecture-support (CAS) feature negotiation call.

Currently, if the guest selects an interrupt controller different from the
one selected at initial boot, this causes the system to be reset with the
new model and the boot starts again.  This means we run through the SLOF
boot process twice, as well as any other bootloader (e.g. grub) in use
before the OS calls CAS.  This can be confusing and/or inconvenient for
users.

Thanks to two fairly recent changes, we no longer need this reboot.  1) we
now completely regenerate the device tree when CAS is called (meaning we
don't need special case updates for all the device tree changes caused by
the interrupt controller mode change),  2) we now have explicit code paths
to activate and deactivate the different interrupt controllers, rather than
just implicitly calling those at machine reset time.

We can therefore eliminate the reboot for changing irq mode, simply by
putting a called to spapr_irq_update_active_intc() before we call
spapr_h_cas_compose_response() (which gives the updated device tree to the
guest firmware and OS).

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr_hcall.c | 33 +++++++++++++--------------------
 1 file changed, 13 insertions(+), 20 deletions(-)

diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index 140f05c1c6..05a7ca275b 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -1767,21 +1767,10 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
     }
     spapr->cas_pre_isa3_guest = !spapr_ovec_test(ov1_guest, OV1_PPC_3_00);
     spapr_ovec_cleanup(ov1_guest);
-    if (!spapr->cas_reboot) {
-        /* If spapr_machine_reset() did not set up a HPT but one is necessary
-         * (because the guest isn't going to use radix) then set it up here. */
-        if ((spapr->patb_entry & PATE1_GR) && !guest_radix) {
-            /* legacy hash or new hash: */
-            spapr_setup_hpt_and_vrma(spapr);
-        }
-        spapr->cas_reboot =
-            (spapr_h_cas_compose_response(spapr, args[1], args[2],
-                                          ov5_updates) != 0);
-    }
 
     /*
-     * Ensure the guest asks for an interrupt mode we support; otherwise
-     * terminate the boot.
+     * Ensure the guest asks for an interrupt mode we support;
+     * otherwise terminate the boot.
      */
     if (guest_xive) {
         if (!spapr->irq->xive) {
@@ -1797,14 +1786,18 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
         }
     }
 
-    /*
-     * Generate a machine reset when we have an update of the
-     * interrupt mode. Only required when the machine supports both
-     * modes.
-     */
+    spapr_irq_update_active_intc(spapr);
+
     if (!spapr->cas_reboot) {
-        spapr->cas_reboot = spapr_ovec_test(ov5_updates, OV5_XIVE_EXPLOIT)
-            && spapr->irq->xics && spapr->irq->xive;
+        /* If spapr_machine_reset() did not set up a HPT but one is necessary
+         * (because the guest isn't going to use radix) then set it up here. */
+        if ((spapr->patb_entry & PATE1_GR) && !guest_radix) {
+            /* legacy hash or new hash: */
+            spapr_setup_hpt_and_vrma(spapr);
+        }
+        spapr->cas_reboot =
+            (spapr_h_cas_compose_response(spapr, args[1], args[2],
+                                          ov5_updates) != 0);
     }
 
     spapr_ovec_cleanup(ov5_updates);
-- 
2.23.0



^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [for-5.0 2/4] spapr: Improve handling of fdt buffer size
  2019-11-29  5:33 [for-5.0 0/4] spapr: Improvements to CAS feature negotiation David Gibson
  2019-11-29  5:33 ` [for-5.0 1/4] spapr: Don't trigger a CAS reboot for XICS/XIVE mode changeover David Gibson
@ 2019-11-29  5:33 ` David Gibson
  2019-12-02  7:04   ` Cédric Le Goater
  2019-12-02  7:52   ` Greg Kurz
  2019-11-29  5:33 ` [for-5.0 3/4] spapr: Fold h_cas_compose_response() into h_client_architecture_support() David Gibson
                   ` (2 subsequent siblings)
  4 siblings, 2 replies; 16+ messages in thread
From: David Gibson @ 2019-11-29  5:33 UTC (permalink / raw)
  To: groug, clg; +Cc: lvivier, David Gibson, qemu-ppc, mdroth, qemu-devel

Previously, spapr_build_fdt() constructed the device tree in a fixed
buffer of size FDT_MAX_SIZE.  This is a bit inflexible, but more
importantly it's awkward for the case where we use it during CAS.  In
that case the guest firmware supplies a buffer and we have to
awkwardly check that what we generated fits into it afterwards, after
doing a lot of size checks during spapr_build_fdt().

Simplify this by having spapr_build_fdt() take a 'space' parameter.
For the CAS case, we pass in the buffer size provided by SLOF, for the
machine init case, we continue to pass FDT_MAX_SIZE.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr.c | 33 +++++++++++----------------------
 1 file changed, 11 insertions(+), 22 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index d9c9a2bcee..d34e317f48 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -917,7 +917,8 @@ static bool spapr_hotplugged_dev_before_cas(void)
     return false;
 }
 
-static void *spapr_build_fdt(SpaprMachineState *spapr, bool reset);
+static void *spapr_build_fdt(SpaprMachineState *spapr, bool reset,
+                             size_t space);
 
 int spapr_h_cas_compose_response(SpaprMachineState *spapr,
                                  target_ulong addr, target_ulong size,
@@ -930,24 +931,17 @@ int spapr_h_cas_compose_response(SpaprMachineState *spapr,
         return 1;
     }
 
-    if (size < sizeof(hdr) || size > FW_MAX_SIZE) {
-        error_report("SLOF provided an unexpected CAS buffer size "
-                     TARGET_FMT_lu " (min: %zu, max: %u)",
-                     size, sizeof(hdr), FW_MAX_SIZE);
+    if (size < sizeof(hdr)) {
+        error_report("SLOF provided insufficient CAS buffer "
+                     TARGET_FMT_lu " (min: %zu)", size, sizeof(hdr));
         exit(EXIT_FAILURE);
     }
 
     size -= sizeof(hdr);
 
-    fdt = spapr_build_fdt(spapr, false);
+    fdt = spapr_build_fdt(spapr, false, size);
     _FDT((fdt_pack(fdt)));
 
-    if (fdt_totalsize(fdt) + sizeof(hdr) > size) {
-        g_free(fdt);
-        trace_spapr_cas_failed(size);
-        return -1;
-    }
-
     cpu_physical_memory_write(addr, &hdr, sizeof(hdr));
     cpu_physical_memory_write(addr + sizeof(hdr), fdt, fdt_totalsize(fdt));
     trace_spapr_cas_continue(fdt_totalsize(fdt) + sizeof(hdr));
@@ -1197,7 +1191,8 @@ static void spapr_dt_hypervisor(SpaprMachineState *spapr, void *fdt)
     }
 }
 
-static void *spapr_build_fdt(SpaprMachineState *spapr, bool reset)
+static void *spapr_build_fdt(SpaprMachineState *spapr, bool reset,
+                             size_t space)
 {
     MachineState *machine = MACHINE(spapr);
     MachineClass *mc = MACHINE_GET_CLASS(machine);
@@ -1207,8 +1202,8 @@ static void *spapr_build_fdt(SpaprMachineState *spapr, bool reset)
     SpaprPhbState *phb;
     char *buf;
 
-    fdt = g_malloc0(FDT_MAX_SIZE);
-    _FDT((fdt_create_empty_tree(fdt, FDT_MAX_SIZE)));
+    fdt = g_malloc0(space);
+    _FDT((fdt_create_empty_tree(fdt, space)));
 
     /* Root node */
     _FDT(fdt_setprop_string(fdt, 0, "device_type", "chrp"));
@@ -1723,19 +1718,13 @@ static void spapr_machine_reset(MachineState *machine)
      */
     fdt_addr = MIN(spapr->rma_size, RTAS_MAX_ADDR) - FDT_MAX_SIZE;
 
-    fdt = spapr_build_fdt(spapr, true);
+    fdt = spapr_build_fdt(spapr, true, FDT_MAX_SIZE);
 
     rc = fdt_pack(fdt);
 
     /* Should only fail if we've built a corrupted tree */
     assert(rc == 0);
 
-    if (fdt_totalsize(fdt) > FDT_MAX_SIZE) {
-        error_report("FDT too big ! 0x%x bytes (max is 0x%x)",
-                     fdt_totalsize(fdt), FDT_MAX_SIZE);
-        exit(1);
-    }
-
     /* Load the fdt */
     qemu_fdt_dumpdtb(fdt, fdt_totalsize(fdt));
     cpu_physical_memory_write(fdt_addr, fdt, fdt_totalsize(fdt));
-- 
2.23.0



^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [for-5.0 3/4] spapr: Fold h_cas_compose_response() into h_client_architecture_support()
  2019-11-29  5:33 [for-5.0 0/4] spapr: Improvements to CAS feature negotiation David Gibson
  2019-11-29  5:33 ` [for-5.0 1/4] spapr: Don't trigger a CAS reboot for XICS/XIVE mode changeover David Gibson
  2019-11-29  5:33 ` [for-5.0 2/4] spapr: Improve handling of fdt buffer size David Gibson
@ 2019-11-29  5:33 ` David Gibson
  2019-12-02  7:04   ` Cédric Le Goater
  2019-12-02  8:23   ` Greg Kurz
  2019-11-29  5:33 ` [for-5.0 4/4] spapr: Simplify ovec diff David Gibson
  2019-12-02  7:05 ` [for-5.0 0/4] spapr: Improvements to CAS feature negotiation Cédric Le Goater
  4 siblings, 2 replies; 16+ messages in thread
From: David Gibson @ 2019-11-29  5:33 UTC (permalink / raw)
  To: groug, clg; +Cc: lvivier, David Gibson, qemu-ppc, mdroth, qemu-devel

spapr_h_cas_compose_response() handles the last piece of the PAPR feature
negotiation process invoked via the ibm,client-architecture-support OF
call.  Its only caller is h_client_architecture_support() which handles
most of the rest of that process.

I believe it was place in a separate file originally to handle some fiddly
dependencies between functions, but mostly it's just confusing to have
the CAS process split into two pieces like this.  Now that compose response
is simplified (by just generating the whole device tree anew), it's cleaner
to just fold it into h_client_architecture_support().

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr.c         | 61 +-----------------------------------------
 hw/ppc/spapr_hcall.c   | 55 ++++++++++++++++++++++++++++++++++---
 include/hw/ppc/spapr.h |  4 +--
 3 files changed, 54 insertions(+), 66 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index d34e317f48..5187f5b0a5 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -76,7 +76,6 @@
 #include "hw/nmi.h"
 #include "hw/intc/intc.h"
 
-#include "qemu/cutils.h"
 #include "hw/ppc/spapr_cpu_core.h"
 #include "hw/mem/memory-device.h"
 #include "hw/ppc/spapr_tpm_proxy.h"
@@ -897,63 +896,6 @@ out:
     return ret;
 }
 
-static bool spapr_hotplugged_dev_before_cas(void)
-{
-    Object *drc_container, *obj;
-    ObjectProperty *prop;
-    ObjectPropertyIterator iter;
-
-    drc_container = container_get(object_get_root(), "/dr-connector");
-    object_property_iter_init(&iter, drc_container);
-    while ((prop = object_property_iter_next(&iter))) {
-        if (!strstart(prop->type, "link<", NULL)) {
-            continue;
-        }
-        obj = object_property_get_link(drc_container, prop->name, NULL);
-        if (spapr_drc_needed(obj)) {
-            return true;
-        }
-    }
-    return false;
-}
-
-static void *spapr_build_fdt(SpaprMachineState *spapr, bool reset,
-                             size_t space);
-
-int spapr_h_cas_compose_response(SpaprMachineState *spapr,
-                                 target_ulong addr, target_ulong size,
-                                 SpaprOptionVector *ov5_updates)
-{
-    void *fdt;
-    SpaprDeviceTreeUpdateHeader hdr = { .version_id = 1 };
-
-    if (spapr_hotplugged_dev_before_cas()) {
-        return 1;
-    }
-
-    if (size < sizeof(hdr)) {
-        error_report("SLOF provided insufficient CAS buffer "
-                     TARGET_FMT_lu " (min: %zu)", size, sizeof(hdr));
-        exit(EXIT_FAILURE);
-    }
-
-    size -= sizeof(hdr);
-
-    fdt = spapr_build_fdt(spapr, false, size);
-    _FDT((fdt_pack(fdt)));
-
-    cpu_physical_memory_write(addr, &hdr, sizeof(hdr));
-    cpu_physical_memory_write(addr + sizeof(hdr), fdt, fdt_totalsize(fdt));
-    trace_spapr_cas_continue(fdt_totalsize(fdt) + sizeof(hdr));
-
-    g_free(spapr->fdt_blob);
-    spapr->fdt_size = fdt_totalsize(fdt);
-    spapr->fdt_initial_size = spapr->fdt_size;
-    spapr->fdt_blob = fdt;
-
-    return 0;
-}
-
 static void spapr_dt_rtas(SpaprMachineState *spapr, void *fdt)
 {
     MachineState *ms = MACHINE(spapr);
@@ -1191,8 +1133,7 @@ static void spapr_dt_hypervisor(SpaprMachineState *spapr, void *fdt)
     }
 }
 
-static void *spapr_build_fdt(SpaprMachineState *spapr, bool reset,
-                             size_t space)
+void *spapr_build_fdt(SpaprMachineState *spapr, bool reset, size_t space)
 {
     MachineState *machine = MACHINE(spapr);
     MachineClass *mc = MACHINE_GET_CLASS(machine);
diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index 05a7ca275b..0f19be794c 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -1,4 +1,5 @@
 #include "qemu/osdep.h"
+#include "qemu/cutils.h"
 #include "qapi/error.h"
 #include "sysemu/hw_accel.h"
 #include "sysemu/runstate.h"
@@ -15,6 +16,7 @@
 #include "cpu-models.h"
 #include "trace.h"
 #include "kvm_ppc.h"
+#include "hw/ppc/fdt.h"
 #include "hw/ppc/spapr_ovec.h"
 #include "mmu-book3s-v3.h"
 #include "hw/mem/memory-device.h"
@@ -1638,6 +1640,26 @@ static uint32_t cas_check_pvr(SpaprMachineState *spapr, PowerPCCPU *cpu,
     return best_compat;
 }
 
+static bool spapr_hotplugged_dev_before_cas(void)
+{
+    Object *drc_container, *obj;
+    ObjectProperty *prop;
+    ObjectPropertyIterator iter;
+
+    drc_container = container_get(object_get_root(), "/dr-connector");
+    object_property_iter_init(&iter, drc_container);
+    while ((prop = object_property_iter_next(&iter))) {
+        if (!strstart(prop->type, "link<", NULL)) {
+            continue;
+        }
+        obj = object_property_get_link(drc_container, prop->name, NULL);
+        if (spapr_drc_needed(obj)) {
+            return true;
+        }
+    }
+    return false;
+}
+
 static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
                                                   SpaprMachineState *spapr,
                                                   target_ulong opcode,
@@ -1645,6 +1667,8 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
 {
     /* Working address in data buffer */
     target_ulong addr = ppc64_phys_to_real(args[0]);
+    target_ulong fdt_buf = args[1];
+    target_ulong fdt_bufsize = args[2];
     target_ulong ov_table;
     uint32_t cas_pvr;
     SpaprOptionVector *ov1_guest, *ov5_guest, *ov5_cas_old, *ov5_updates;
@@ -1788,16 +1812,41 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
 
     spapr_irq_update_active_intc(spapr);
 
+    if (spapr_hotplugged_dev_before_cas()) {
+        spapr->cas_reboot = true;
+    }
+
     if (!spapr->cas_reboot) {
+        void *fdt;
+        SpaprDeviceTreeUpdateHeader hdr = { .version_id = 1 };
+
         /* If spapr_machine_reset() did not set up a HPT but one is necessary
          * (because the guest isn't going to use radix) then set it up here. */
         if ((spapr->patb_entry & PATE1_GR) && !guest_radix) {
             /* legacy hash or new hash: */
             spapr_setup_hpt_and_vrma(spapr);
         }
-        spapr->cas_reboot =
-            (spapr_h_cas_compose_response(spapr, args[1], args[2],
-                                          ov5_updates) != 0);
+
+        if (fdt_bufsize < sizeof(hdr)) {
+            error_report("SLOF provided insufficient CAS buffer "
+                         TARGET_FMT_lu " (min: %zu)", fdt_bufsize, sizeof(hdr));
+            exit(EXIT_FAILURE);
+        }
+
+        fdt_bufsize -= sizeof(hdr);
+
+        fdt = spapr_build_fdt(spapr, false, fdt_bufsize);
+        _FDT((fdt_pack(fdt)));
+
+        cpu_physical_memory_write(fdt_buf, &hdr, sizeof(hdr));
+        cpu_physical_memory_write(fdt_buf + sizeof(hdr), fdt,
+                                  fdt_totalsize(fdt));
+        trace_spapr_cas_continue(fdt_totalsize(fdt) + sizeof(hdr));
+
+        g_free(spapr->fdt_blob);
+        spapr->fdt_size = fdt_totalsize(fdt);
+        spapr->fdt_initial_size = spapr->fdt_size;
+        spapr->fdt_blob = fdt;
     }
 
     spapr_ovec_cleanup(ov5_updates);
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index d5ab5ea7b2..61f005c6f6 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -766,11 +766,9 @@ struct SpaprEventLogEntry {
     QTAILQ_ENTRY(SpaprEventLogEntry) next;
 };
 
+void *spapr_build_fdt(SpaprMachineState *spapr, bool reset, size_t space);
 void spapr_events_init(SpaprMachineState *sm);
 void spapr_dt_events(SpaprMachineState *sm, void *fdt);
-int spapr_h_cas_compose_response(SpaprMachineState *sm,
-                                 target_ulong addr, target_ulong size,
-                                 SpaprOptionVector *ov5_updates);
 void close_htab_fd(SpaprMachineState *spapr);
 void spapr_setup_hpt_and_vrma(SpaprMachineState *spapr);
 void spapr_free_hpt(SpaprMachineState *spapr);
-- 
2.23.0



^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [for-5.0 4/4] spapr: Simplify ovec diff
  2019-11-29  5:33 [for-5.0 0/4] spapr: Improvements to CAS feature negotiation David Gibson
                   ` (2 preceding siblings ...)
  2019-11-29  5:33 ` [for-5.0 3/4] spapr: Fold h_cas_compose_response() into h_client_architecture_support() David Gibson
@ 2019-11-29  5:33 ` David Gibson
  2019-12-02  7:04   ` Cédric Le Goater
  2019-12-02  7:05 ` [for-5.0 0/4] spapr: Improvements to CAS feature negotiation Cédric Le Goater
  4 siblings, 1 reply; 16+ messages in thread
From: David Gibson @ 2019-11-29  5:33 UTC (permalink / raw)
  To: groug, clg; +Cc: lvivier, David Gibson, qemu-ppc, mdroth, qemu-devel

spapr_ovec_diff(ov, old, new) has somewhat complex semantics.  ov is set
to those bits which are in new but not old, and it returns as a boolean
whether or not there are any bits in old but not new.

It turns out that both callers only care about the second, not the first.
This is basically equivalent to a bitmap subset operation, which is easier
to understand and implement.  So replace spapr_ovec_diff() with
spapr_ovec_subset().

Cc: Mike Roth <mdroth@linux.vnet.ibm.com>

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr.c              | 14 +++-----------
 hw/ppc/spapr_hcall.c        |  8 ++------
 hw/ppc/spapr_ovec.c         | 30 ++++++++++--------------------
 include/hw/ppc/spapr_ovec.h |  4 +---
 4 files changed, 16 insertions(+), 40 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 5187f5b0a5..32e1cc1d3f 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1840,8 +1840,6 @@ static bool spapr_ov5_cas_needed(void *opaque)
 {
     SpaprMachineState *spapr = opaque;
     SpaprOptionVector *ov5_mask = spapr_ovec_new();
-    SpaprOptionVector *ov5_legacy = spapr_ovec_new();
-    SpaprOptionVector *ov5_removed = spapr_ovec_new();
     bool cas_needed;
 
     /* Prior to the introduction of SpaprOptionVector, we had two option
@@ -1873,17 +1871,11 @@ static bool spapr_ov5_cas_needed(void *opaque)
     spapr_ovec_set(ov5_mask, OV5_DRCONF_MEMORY);
     spapr_ovec_set(ov5_mask, OV5_DRMEM_V2);
 
-    /* spapr_ovec_diff returns true if bits were removed. we avoid using
-     * the mask itself since in the future it's possible "legacy" bits may be
-     * removed via machine options, which could generate a false positive
-     * that breaks migration.
-     */
-    spapr_ovec_intersect(ov5_legacy, spapr->ov5, ov5_mask);
-    cas_needed = spapr_ovec_diff(ov5_removed, spapr->ov5, ov5_legacy);
+    /* We need extra information if we have any bits outside the mask
+     * defined above */
+    cas_needed = !spapr_ovec_subset(spapr->ov5, ov5_mask);
 
     spapr_ovec_cleanup(ov5_mask);
-    spapr_ovec_cleanup(ov5_legacy);
-    spapr_ovec_cleanup(ov5_removed);
 
     return cas_needed;
 }
diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index 0f19be794c..f1799b1b70 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -1671,7 +1671,7 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
     target_ulong fdt_bufsize = args[2];
     target_ulong ov_table;
     uint32_t cas_pvr;
-    SpaprOptionVector *ov1_guest, *ov5_guest, *ov5_cas_old, *ov5_updates;
+    SpaprOptionVector *ov1_guest, *ov5_guest, *ov5_cas_old;
     bool guest_radix;
     Error *local_err = NULL;
     bool raw_mode_supported = false;
@@ -1770,9 +1770,7 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
     /* capabilities that have been added since CAS-generated guest reset.
      * if capabilities have since been removed, generate another reset
      */
-    ov5_updates = spapr_ovec_new();
-    spapr->cas_reboot = spapr_ovec_diff(ov5_updates,
-                                        ov5_cas_old, spapr->ov5_cas);
+    spapr->cas_reboot = !spapr_ovec_subset(ov5_cas_old, spapr->ov5_cas);
     spapr_ovec_cleanup(ov5_cas_old);
     /* Now that processing is finished, set the radix/hash bit for the
      * guest if it requested a valid mode; otherwise terminate the boot. */
@@ -1849,8 +1847,6 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
         spapr->fdt_blob = fdt;
     }
 
-    spapr_ovec_cleanup(ov5_updates);
-
     if (spapr->cas_reboot) {
         qemu_system_reset_request(SHUTDOWN_CAUSE_SUBSYSTEM_RESET);
     }
diff --git a/hw/ppc/spapr_ovec.c b/hw/ppc/spapr_ovec.c
index 811fadf143..0ff6d1aeae 100644
--- a/hw/ppc/spapr_ovec.c
+++ b/hw/ppc/spapr_ovec.c
@@ -76,31 +76,21 @@ void spapr_ovec_intersect(SpaprOptionVector *ov,
     bitmap_and(ov->bitmap, ov1->bitmap, ov2->bitmap, OV_MAXBITS);
 }
 
-/* returns true if options bits were removed, false otherwise */
-bool spapr_ovec_diff(SpaprOptionVector *ov,
-                     SpaprOptionVector *ov_old,
-                     SpaprOptionVector *ov_new)
+/* returns true if ov1 has a subset of bits in ov2 */
+bool spapr_ovec_subset(SpaprOptionVector *ov1, SpaprOptionVector *ov2)
 {
-    unsigned long *change_mask = bitmap_new(OV_MAXBITS);
-    unsigned long *removed_bits = bitmap_new(OV_MAXBITS);
-    bool bits_were_removed = false;
+    unsigned long *tmp = bitmap_new(OV_MAXBITS);
+    bool result;
 
-    g_assert(ov);
-    g_assert(ov_old);
-    g_assert(ov_new);
-
-    bitmap_xor(change_mask, ov_old->bitmap, ov_new->bitmap, OV_MAXBITS);
-    bitmap_and(ov->bitmap, ov_new->bitmap, change_mask, OV_MAXBITS);
-    bitmap_and(removed_bits, ov_old->bitmap, change_mask, OV_MAXBITS);
+    g_assert(ov1);
+    g_assert(ov2);
 
-    if (!bitmap_empty(removed_bits, OV_MAXBITS)) {
-        bits_were_removed = true;
-    }
+    bitmap_andnot(tmp, ov1->bitmap, ov2->bitmap, OV_MAXBITS);
+    result = bitmap_empty(tmp, OV_MAXBITS);
 
-    g_free(change_mask);
-    g_free(removed_bits);
+    g_free(tmp);
 
-    return bits_were_removed;
+    return result;
 }
 
 void spapr_ovec_cleanup(SpaprOptionVector *ov)
diff --git a/include/hw/ppc/spapr_ovec.h b/include/hw/ppc/spapr_ovec.h
index 7891e9caac..2bed517a2b 100644
--- a/include/hw/ppc/spapr_ovec.h
+++ b/include/hw/ppc/spapr_ovec.h
@@ -66,9 +66,7 @@ SpaprOptionVector *spapr_ovec_clone(SpaprOptionVector *ov_orig);
 void spapr_ovec_intersect(SpaprOptionVector *ov,
                           SpaprOptionVector *ov1,
                           SpaprOptionVector *ov2);
-bool spapr_ovec_diff(SpaprOptionVector *ov,
-                     SpaprOptionVector *ov_old,
-                     SpaprOptionVector *ov_new);
+bool spapr_ovec_subset(SpaprOptionVector *ov1, SpaprOptionVector *ov2);
 void spapr_ovec_cleanup(SpaprOptionVector *ov);
 void spapr_ovec_set(SpaprOptionVector *ov, long bitnr);
 void spapr_ovec_clear(SpaprOptionVector *ov, long bitnr);
-- 
2.23.0



^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [for-5.0 1/4] spapr: Don't trigger a CAS reboot for XICS/XIVE mode changeover
  2019-11-29  5:33 ` [for-5.0 1/4] spapr: Don't trigger a CAS reboot for XICS/XIVE mode changeover David Gibson
@ 2019-12-02  7:03   ` Cédric Le Goater
  2019-12-02  7:50   ` Greg Kurz
  1 sibling, 0 replies; 16+ messages in thread
From: Cédric Le Goater @ 2019-12-02  7:03 UTC (permalink / raw)
  To: David Gibson, groug; +Cc: lvivier, qemu-ppc, mdroth, qemu-devel

On 29/11/2019 06:33, David Gibson wrote:
> PAPR allows the interrupt controller used on a POWER9 machine (XICS or
> XIVE) to be selected by the guest operating system, by using the
> ibm,client-architecture-support (CAS) feature negotiation call.
> 
> Currently, if the guest selects an interrupt controller different from the
> one selected at initial boot, this causes the system to be reset with the
> new model and the boot starts again.  This means we run through the SLOF
> boot process twice, as well as any other bootloader (e.g. grub) in use
> before the OS calls CAS.  This can be confusing and/or inconvenient for
> users.
> 
> Thanks to two fairly recent changes, we no longer need this reboot.  1) we
> now completely regenerate the device tree when CAS is called (meaning we
> don't need special case updates for all the device tree changes caused by
> the interrupt controller mode change),  2) we now have explicit code paths
> to activate and deactivate the different interrupt controllers, rather than
> just implicitly calling those at machine reset time.
> 
> We can therefore eliminate the reboot for changing irq mode, simply by
> putting a called to spapr_irq_update_active_intc() before we call
> spapr_h_cas_compose_response() (which gives the updated device tree to the
> guest firmware and OS).
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>


Reviewed-by: Cedric Le Goater <clg@fr.ibm.com>

> ---
>  hw/ppc/spapr_hcall.c | 33 +++++++++++++--------------------
>  1 file changed, 13 insertions(+), 20 deletions(-)
> 
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index 140f05c1c6..05a7ca275b 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -1767,21 +1767,10 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
>      }
>      spapr->cas_pre_isa3_guest = !spapr_ovec_test(ov1_guest, OV1_PPC_3_00);
>      spapr_ovec_cleanup(ov1_guest);
> -    if (!spapr->cas_reboot) {
> -        /* If spapr_machine_reset() did not set up a HPT but one is necessary
> -         * (because the guest isn't going to use radix) then set it up here. */
> -        if ((spapr->patb_entry & PATE1_GR) && !guest_radix) {
> -            /* legacy hash or new hash: */
> -            spapr_setup_hpt_and_vrma(spapr);
> -        }
> -        spapr->cas_reboot =
> -            (spapr_h_cas_compose_response(spapr, args[1], args[2],
> -                                          ov5_updates) != 0);
> -    }
>  
>      /*
> -     * Ensure the guest asks for an interrupt mode we support; otherwise
> -     * terminate the boot.
> +     * Ensure the guest asks for an interrupt mode we support;
> +     * otherwise terminate the boot.
>       */
>      if (guest_xive) {
>          if (!spapr->irq->xive) {
> @@ -1797,14 +1786,18 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
>          }
>      }
>  
> -    /*
> -     * Generate a machine reset when we have an update of the
> -     * interrupt mode. Only required when the machine supports both
> -     * modes.
> -     */
> +    spapr_irq_update_active_intc(spapr);
> +
>      if (!spapr->cas_reboot) {
> -        spapr->cas_reboot = spapr_ovec_test(ov5_updates, OV5_XIVE_EXPLOIT)
> -            && spapr->irq->xics && spapr->irq->xive;
> +        /* If spapr_machine_reset() did not set up a HPT but one is necessary
> +         * (because the guest isn't going to use radix) then set it up here. */
> +        if ((spapr->patb_entry & PATE1_GR) && !guest_radix) {
> +            /* legacy hash or new hash: */
> +            spapr_setup_hpt_and_vrma(spapr);
> +        }
> +        spapr->cas_reboot =
> +            (spapr_h_cas_compose_response(spapr, args[1], args[2],
> +                                          ov5_updates) != 0);
>      }
>  
>      spapr_ovec_cleanup(ov5_updates);
> 



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [for-5.0 4/4] spapr: Simplify ovec diff
  2019-11-29  5:33 ` [for-5.0 4/4] spapr: Simplify ovec diff David Gibson
@ 2019-12-02  7:04   ` Cédric Le Goater
  0 siblings, 0 replies; 16+ messages in thread
From: Cédric Le Goater @ 2019-12-02  7:04 UTC (permalink / raw)
  To: David Gibson, groug; +Cc: lvivier, qemu-ppc, mdroth, qemu-devel

On 29/11/2019 06:33, David Gibson wrote:
> spapr_ovec_diff(ov, old, new) has somewhat complex semantics.  ov is set
> to those bits which are in new but not old, and it returns as a boolean
> whether or not there are any bits in old but not new.
> 
> It turns out that both callers only care about the second, not the first.
> This is basically equivalent to a bitmap subset operation, which is easier
> to understand and implement.  So replace spapr_ovec_diff() with
> spapr_ovec_subset().
> 
> Cc: Mike Roth <mdroth@linux.vnet.ibm.com>
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>


Reviewed-by: Cedric Le Goater <clg@fr.ibm.com>

> ---
>  hw/ppc/spapr.c              | 14 +++-----------
>  hw/ppc/spapr_hcall.c        |  8 ++------
>  hw/ppc/spapr_ovec.c         | 30 ++++++++++--------------------
>  include/hw/ppc/spapr_ovec.h |  4 +---
>  4 files changed, 16 insertions(+), 40 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 5187f5b0a5..32e1cc1d3f 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1840,8 +1840,6 @@ static bool spapr_ov5_cas_needed(void *opaque)
>  {
>      SpaprMachineState *spapr = opaque;
>      SpaprOptionVector *ov5_mask = spapr_ovec_new();
> -    SpaprOptionVector *ov5_legacy = spapr_ovec_new();
> -    SpaprOptionVector *ov5_removed = spapr_ovec_new();
>      bool cas_needed;
>  
>      /* Prior to the introduction of SpaprOptionVector, we had two option
> @@ -1873,17 +1871,11 @@ static bool spapr_ov5_cas_needed(void *opaque)
>      spapr_ovec_set(ov5_mask, OV5_DRCONF_MEMORY);
>      spapr_ovec_set(ov5_mask, OV5_DRMEM_V2);
>  
> -    /* spapr_ovec_diff returns true if bits were removed. we avoid using
> -     * the mask itself since in the future it's possible "legacy" bits may be
> -     * removed via machine options, which could generate a false positive
> -     * that breaks migration.
> -     */
> -    spapr_ovec_intersect(ov5_legacy, spapr->ov5, ov5_mask);
> -    cas_needed = spapr_ovec_diff(ov5_removed, spapr->ov5, ov5_legacy);
> +    /* We need extra information if we have any bits outside the mask
> +     * defined above */
> +    cas_needed = !spapr_ovec_subset(spapr->ov5, ov5_mask);
>  
>      spapr_ovec_cleanup(ov5_mask);
> -    spapr_ovec_cleanup(ov5_legacy);
> -    spapr_ovec_cleanup(ov5_removed);
>  
>      return cas_needed;
>  }
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index 0f19be794c..f1799b1b70 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -1671,7 +1671,7 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
>      target_ulong fdt_bufsize = args[2];
>      target_ulong ov_table;
>      uint32_t cas_pvr;
> -    SpaprOptionVector *ov1_guest, *ov5_guest, *ov5_cas_old, *ov5_updates;
> +    SpaprOptionVector *ov1_guest, *ov5_guest, *ov5_cas_old;
>      bool guest_radix;
>      Error *local_err = NULL;
>      bool raw_mode_supported = false;
> @@ -1770,9 +1770,7 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
>      /* capabilities that have been added since CAS-generated guest reset.
>       * if capabilities have since been removed, generate another reset
>       */
> -    ov5_updates = spapr_ovec_new();
> -    spapr->cas_reboot = spapr_ovec_diff(ov5_updates,
> -                                        ov5_cas_old, spapr->ov5_cas);
> +    spapr->cas_reboot = !spapr_ovec_subset(ov5_cas_old, spapr->ov5_cas);
>      spapr_ovec_cleanup(ov5_cas_old);
>      /* Now that processing is finished, set the radix/hash bit for the
>       * guest if it requested a valid mode; otherwise terminate the boot. */
> @@ -1849,8 +1847,6 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
>          spapr->fdt_blob = fdt;
>      }
>  
> -    spapr_ovec_cleanup(ov5_updates);
> -
>      if (spapr->cas_reboot) {
>          qemu_system_reset_request(SHUTDOWN_CAUSE_SUBSYSTEM_RESET);
>      }
> diff --git a/hw/ppc/spapr_ovec.c b/hw/ppc/spapr_ovec.c
> index 811fadf143..0ff6d1aeae 100644
> --- a/hw/ppc/spapr_ovec.c
> +++ b/hw/ppc/spapr_ovec.c
> @@ -76,31 +76,21 @@ void spapr_ovec_intersect(SpaprOptionVector *ov,
>      bitmap_and(ov->bitmap, ov1->bitmap, ov2->bitmap, OV_MAXBITS);
>  }
>  
> -/* returns true if options bits were removed, false otherwise */
> -bool spapr_ovec_diff(SpaprOptionVector *ov,
> -                     SpaprOptionVector *ov_old,
> -                     SpaprOptionVector *ov_new)
> +/* returns true if ov1 has a subset of bits in ov2 */
> +bool spapr_ovec_subset(SpaprOptionVector *ov1, SpaprOptionVector *ov2)
>  {
> -    unsigned long *change_mask = bitmap_new(OV_MAXBITS);
> -    unsigned long *removed_bits = bitmap_new(OV_MAXBITS);
> -    bool bits_were_removed = false;
> +    unsigned long *tmp = bitmap_new(OV_MAXBITS);
> +    bool result;
>  
> -    g_assert(ov);
> -    g_assert(ov_old);
> -    g_assert(ov_new);
> -
> -    bitmap_xor(change_mask, ov_old->bitmap, ov_new->bitmap, OV_MAXBITS);
> -    bitmap_and(ov->bitmap, ov_new->bitmap, change_mask, OV_MAXBITS);
> -    bitmap_and(removed_bits, ov_old->bitmap, change_mask, OV_MAXBITS);
> +    g_assert(ov1);
> +    g_assert(ov2);
>  
> -    if (!bitmap_empty(removed_bits, OV_MAXBITS)) {
> -        bits_were_removed = true;
> -    }
> +    bitmap_andnot(tmp, ov1->bitmap, ov2->bitmap, OV_MAXBITS);
> +    result = bitmap_empty(tmp, OV_MAXBITS);
>  
> -    g_free(change_mask);
> -    g_free(removed_bits);
> +    g_free(tmp);
>  
> -    return bits_were_removed;
> +    return result;
>  }
>  
>  void spapr_ovec_cleanup(SpaprOptionVector *ov)
> diff --git a/include/hw/ppc/spapr_ovec.h b/include/hw/ppc/spapr_ovec.h
> index 7891e9caac..2bed517a2b 100644
> --- a/include/hw/ppc/spapr_ovec.h
> +++ b/include/hw/ppc/spapr_ovec.h
> @@ -66,9 +66,7 @@ SpaprOptionVector *spapr_ovec_clone(SpaprOptionVector *ov_orig);
>  void spapr_ovec_intersect(SpaprOptionVector *ov,
>                            SpaprOptionVector *ov1,
>                            SpaprOptionVector *ov2);
> -bool spapr_ovec_diff(SpaprOptionVector *ov,
> -                     SpaprOptionVector *ov_old,
> -                     SpaprOptionVector *ov_new);
> +bool spapr_ovec_subset(SpaprOptionVector *ov1, SpaprOptionVector *ov2);
>  void spapr_ovec_cleanup(SpaprOptionVector *ov);
>  void spapr_ovec_set(SpaprOptionVector *ov, long bitnr);
>  void spapr_ovec_clear(SpaprOptionVector *ov, long bitnr);
> 



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [for-5.0 3/4] spapr: Fold h_cas_compose_response() into h_client_architecture_support()
  2019-11-29  5:33 ` [for-5.0 3/4] spapr: Fold h_cas_compose_response() into h_client_architecture_support() David Gibson
@ 2019-12-02  7:04   ` Cédric Le Goater
  2019-12-02  8:23   ` Greg Kurz
  1 sibling, 0 replies; 16+ messages in thread
From: Cédric Le Goater @ 2019-12-02  7:04 UTC (permalink / raw)
  To: David Gibson, groug; +Cc: lvivier, qemu-ppc, mdroth, qemu-devel

On 29/11/2019 06:33, David Gibson wrote:
> spapr_h_cas_compose_response() handles the last piece of the PAPR feature
> negotiation process invoked via the ibm,client-architecture-support OF
> call.  Its only caller is h_client_architecture_support() which handles
> most of the rest of that process.
> 
> I believe it was place in a separate file originally to handle some fiddly
> dependencies between functions, but mostly it's just confusing to have
> the CAS process split into two pieces like this.  Now that compose response
> is simplified (by just generating the whole device tree anew), it's cleaner
> to just fold it into h_client_architecture_support().
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

Reviewed-by: Cedric Le Goater <clg@fr.ibm.com>

> ---
>  hw/ppc/spapr.c         | 61 +-----------------------------------------
>  hw/ppc/spapr_hcall.c   | 55 ++++++++++++++++++++++++++++++++++---
>  include/hw/ppc/spapr.h |  4 +--
>  3 files changed, 54 insertions(+), 66 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index d34e317f48..5187f5b0a5 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -76,7 +76,6 @@
>  #include "hw/nmi.h"
>  #include "hw/intc/intc.h"
>  
> -#include "qemu/cutils.h"
>  #include "hw/ppc/spapr_cpu_core.h"
>  #include "hw/mem/memory-device.h"
>  #include "hw/ppc/spapr_tpm_proxy.h"
> @@ -897,63 +896,6 @@ out:
>      return ret;
>  }
>  
> -static bool spapr_hotplugged_dev_before_cas(void)
> -{
> -    Object *drc_container, *obj;
> -    ObjectProperty *prop;
> -    ObjectPropertyIterator iter;
> -
> -    drc_container = container_get(object_get_root(), "/dr-connector");
> -    object_property_iter_init(&iter, drc_container);
> -    while ((prop = object_property_iter_next(&iter))) {
> -        if (!strstart(prop->type, "link<", NULL)) {
> -            continue;
> -        }
> -        obj = object_property_get_link(drc_container, prop->name, NULL);
> -        if (spapr_drc_needed(obj)) {
> -            return true;
> -        }
> -    }
> -    return false;
> -}
> -
> -static void *spapr_build_fdt(SpaprMachineState *spapr, bool reset,
> -                             size_t space);
> -
> -int spapr_h_cas_compose_response(SpaprMachineState *spapr,
> -                                 target_ulong addr, target_ulong size,
> -                                 SpaprOptionVector *ov5_updates)
> -{
> -    void *fdt;
> -    SpaprDeviceTreeUpdateHeader hdr = { .version_id = 1 };
> -
> -    if (spapr_hotplugged_dev_before_cas()) {
> -        return 1;
> -    }
> -
> -    if (size < sizeof(hdr)) {
> -        error_report("SLOF provided insufficient CAS buffer "
> -                     TARGET_FMT_lu " (min: %zu)", size, sizeof(hdr));
> -        exit(EXIT_FAILURE);
> -    }
> -
> -    size -= sizeof(hdr);
> -
> -    fdt = spapr_build_fdt(spapr, false, size);
> -    _FDT((fdt_pack(fdt)));
> -
> -    cpu_physical_memory_write(addr, &hdr, sizeof(hdr));
> -    cpu_physical_memory_write(addr + sizeof(hdr), fdt, fdt_totalsize(fdt));
> -    trace_spapr_cas_continue(fdt_totalsize(fdt) + sizeof(hdr));
> -
> -    g_free(spapr->fdt_blob);
> -    spapr->fdt_size = fdt_totalsize(fdt);
> -    spapr->fdt_initial_size = spapr->fdt_size;
> -    spapr->fdt_blob = fdt;
> -
> -    return 0;
> -}
> -
>  static void spapr_dt_rtas(SpaprMachineState *spapr, void *fdt)
>  {
>      MachineState *ms = MACHINE(spapr);
> @@ -1191,8 +1133,7 @@ static void spapr_dt_hypervisor(SpaprMachineState *spapr, void *fdt)
>      }
>  }
>  
> -static void *spapr_build_fdt(SpaprMachineState *spapr, bool reset,
> -                             size_t space)
> +void *spapr_build_fdt(SpaprMachineState *spapr, bool reset, size_t space)
>  {
>      MachineState *machine = MACHINE(spapr);
>      MachineClass *mc = MACHINE_GET_CLASS(machine);
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index 05a7ca275b..0f19be794c 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -1,4 +1,5 @@
>  #include "qemu/osdep.h"
> +#include "qemu/cutils.h"
>  #include "qapi/error.h"
>  #include "sysemu/hw_accel.h"
>  #include "sysemu/runstate.h"
> @@ -15,6 +16,7 @@
>  #include "cpu-models.h"
>  #include "trace.h"
>  #include "kvm_ppc.h"
> +#include "hw/ppc/fdt.h"
>  #include "hw/ppc/spapr_ovec.h"
>  #include "mmu-book3s-v3.h"
>  #include "hw/mem/memory-device.h"
> @@ -1638,6 +1640,26 @@ static uint32_t cas_check_pvr(SpaprMachineState *spapr, PowerPCCPU *cpu,
>      return best_compat;
>  }
>  
> +static bool spapr_hotplugged_dev_before_cas(void)
> +{
> +    Object *drc_container, *obj;
> +    ObjectProperty *prop;
> +    ObjectPropertyIterator iter;
> +
> +    drc_container = container_get(object_get_root(), "/dr-connector");
> +    object_property_iter_init(&iter, drc_container);
> +    while ((prop = object_property_iter_next(&iter))) {
> +        if (!strstart(prop->type, "link<", NULL)) {
> +            continue;
> +        }
> +        obj = object_property_get_link(drc_container, prop->name, NULL);
> +        if (spapr_drc_needed(obj)) {
> +            return true;
> +        }
> +    }
> +    return false;
> +}
> +
>  static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
>                                                    SpaprMachineState *spapr,
>                                                    target_ulong opcode,
> @@ -1645,6 +1667,8 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
>  {
>      /* Working address in data buffer */
>      target_ulong addr = ppc64_phys_to_real(args[0]);
> +    target_ulong fdt_buf = args[1];
> +    target_ulong fdt_bufsize = args[2];
>      target_ulong ov_table;
>      uint32_t cas_pvr;
>      SpaprOptionVector *ov1_guest, *ov5_guest, *ov5_cas_old, *ov5_updates;
> @@ -1788,16 +1812,41 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
>  
>      spapr_irq_update_active_intc(spapr);
>  
> +    if (spapr_hotplugged_dev_before_cas()) {
> +        spapr->cas_reboot = true;
> +    }
> +
>      if (!spapr->cas_reboot) {
> +        void *fdt;
> +        SpaprDeviceTreeUpdateHeader hdr = { .version_id = 1 };
> +
>          /* If spapr_machine_reset() did not set up a HPT but one is necessary
>           * (because the guest isn't going to use radix) then set it up here. */
>          if ((spapr->patb_entry & PATE1_GR) && !guest_radix) {
>              /* legacy hash or new hash: */
>              spapr_setup_hpt_and_vrma(spapr);
>          }
> -        spapr->cas_reboot =
> -            (spapr_h_cas_compose_response(spapr, args[1], args[2],
> -                                          ov5_updates) != 0);
> +
> +        if (fdt_bufsize < sizeof(hdr)) {
> +            error_report("SLOF provided insufficient CAS buffer "
> +                         TARGET_FMT_lu " (min: %zu)", fdt_bufsize, sizeof(hdr));
> +            exit(EXIT_FAILURE);
> +        }
> +
> +        fdt_bufsize -= sizeof(hdr);
> +
> +        fdt = spapr_build_fdt(spapr, false, fdt_bufsize);
> +        _FDT((fdt_pack(fdt)));
> +
> +        cpu_physical_memory_write(fdt_buf, &hdr, sizeof(hdr));
> +        cpu_physical_memory_write(fdt_buf + sizeof(hdr), fdt,
> +                                  fdt_totalsize(fdt));
> +        trace_spapr_cas_continue(fdt_totalsize(fdt) + sizeof(hdr));
> +
> +        g_free(spapr->fdt_blob);
> +        spapr->fdt_size = fdt_totalsize(fdt);
> +        spapr->fdt_initial_size = spapr->fdt_size;
> +        spapr->fdt_blob = fdt;
>      }
>  
>      spapr_ovec_cleanup(ov5_updates);
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index d5ab5ea7b2..61f005c6f6 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -766,11 +766,9 @@ struct SpaprEventLogEntry {
>      QTAILQ_ENTRY(SpaprEventLogEntry) next;
>  };
>  
> +void *spapr_build_fdt(SpaprMachineState *spapr, bool reset, size_t space);
>  void spapr_events_init(SpaprMachineState *sm);
>  void spapr_dt_events(SpaprMachineState *sm, void *fdt);
> -int spapr_h_cas_compose_response(SpaprMachineState *sm,
> -                                 target_ulong addr, target_ulong size,
> -                                 SpaprOptionVector *ov5_updates);
>  void close_htab_fd(SpaprMachineState *spapr);
>  void spapr_setup_hpt_and_vrma(SpaprMachineState *spapr);
>  void spapr_free_hpt(SpaprMachineState *spapr);
> 



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [for-5.0 2/4] spapr: Improve handling of fdt buffer size
  2019-11-29  5:33 ` [for-5.0 2/4] spapr: Improve handling of fdt buffer size David Gibson
@ 2019-12-02  7:04   ` Cédric Le Goater
  2019-12-02  7:52   ` Greg Kurz
  1 sibling, 0 replies; 16+ messages in thread
From: Cédric Le Goater @ 2019-12-02  7:04 UTC (permalink / raw)
  To: David Gibson, groug; +Cc: lvivier, qemu-ppc, mdroth, qemu-devel

On 29/11/2019 06:33, David Gibson wrote:
> Previously, spapr_build_fdt() constructed the device tree in a fixed
> buffer of size FDT_MAX_SIZE.  This is a bit inflexible, but more
> importantly it's awkward for the case where we use it during CAS.  In
> that case the guest firmware supplies a buffer and we have to
> awkwardly check that what we generated fits into it afterwards, after
> doing a lot of size checks during spapr_build_fdt().
> 
> Simplify this by having spapr_build_fdt() take a 'space' parameter.
> For the CAS case, we pass in the buffer size provided by SLOF, for the
> machine init case, we continue to pass FDT_MAX_SIZE.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

Reviewed-by: Cedric Le Goater <clg@fr.ibm.com>


> ---
>  hw/ppc/spapr.c | 33 +++++++++++----------------------
>  1 file changed, 11 insertions(+), 22 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index d9c9a2bcee..d34e317f48 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -917,7 +917,8 @@ static bool spapr_hotplugged_dev_before_cas(void)
>      return false;
>  }
>  
> -static void *spapr_build_fdt(SpaprMachineState *spapr, bool reset);
> +static void *spapr_build_fdt(SpaprMachineState *spapr, bool reset,
> +                             size_t space);
>  
>  int spapr_h_cas_compose_response(SpaprMachineState *spapr,
>                                   target_ulong addr, target_ulong size,
> @@ -930,24 +931,17 @@ int spapr_h_cas_compose_response(SpaprMachineState *spapr,
>          return 1;
>      }
>  
> -    if (size < sizeof(hdr) || size > FW_MAX_SIZE) {
> -        error_report("SLOF provided an unexpected CAS buffer size "
> -                     TARGET_FMT_lu " (min: %zu, max: %u)",
> -                     size, sizeof(hdr), FW_MAX_SIZE);
> +    if (size < sizeof(hdr)) {
> +        error_report("SLOF provided insufficient CAS buffer "
> +                     TARGET_FMT_lu " (min: %zu)", size, sizeof(hdr));
>          exit(EXIT_FAILURE);
>      }
>  
>      size -= sizeof(hdr);
>  
> -    fdt = spapr_build_fdt(spapr, false);
> +    fdt = spapr_build_fdt(spapr, false, size);
>      _FDT((fdt_pack(fdt)));
>  
> -    if (fdt_totalsize(fdt) + sizeof(hdr) > size) {
> -        g_free(fdt);
> -        trace_spapr_cas_failed(size);
> -        return -1;
> -    }
> -
>      cpu_physical_memory_write(addr, &hdr, sizeof(hdr));
>      cpu_physical_memory_write(addr + sizeof(hdr), fdt, fdt_totalsize(fdt));
>      trace_spapr_cas_continue(fdt_totalsize(fdt) + sizeof(hdr));
> @@ -1197,7 +1191,8 @@ static void spapr_dt_hypervisor(SpaprMachineState *spapr, void *fdt)
>      }
>  }
>  
> -static void *spapr_build_fdt(SpaprMachineState *spapr, bool reset)
> +static void *spapr_build_fdt(SpaprMachineState *spapr, bool reset,
> +                             size_t space)
>  {
>      MachineState *machine = MACHINE(spapr);
>      MachineClass *mc = MACHINE_GET_CLASS(machine);
> @@ -1207,8 +1202,8 @@ static void *spapr_build_fdt(SpaprMachineState *spapr, bool reset)
>      SpaprPhbState *phb;
>      char *buf;
>  
> -    fdt = g_malloc0(FDT_MAX_SIZE);
> -    _FDT((fdt_create_empty_tree(fdt, FDT_MAX_SIZE)));
> +    fdt = g_malloc0(space);
> +    _FDT((fdt_create_empty_tree(fdt, space)));
>  
>      /* Root node */
>      _FDT(fdt_setprop_string(fdt, 0, "device_type", "chrp"));
> @@ -1723,19 +1718,13 @@ static void spapr_machine_reset(MachineState *machine)
>       */
>      fdt_addr = MIN(spapr->rma_size, RTAS_MAX_ADDR) - FDT_MAX_SIZE;
>  
> -    fdt = spapr_build_fdt(spapr, true);
> +    fdt = spapr_build_fdt(spapr, true, FDT_MAX_SIZE);
>  
>      rc = fdt_pack(fdt);
>  
>      /* Should only fail if we've built a corrupted tree */
>      assert(rc == 0);
>  
> -    if (fdt_totalsize(fdt) > FDT_MAX_SIZE) {
> -        error_report("FDT too big ! 0x%x bytes (max is 0x%x)",
> -                     fdt_totalsize(fdt), FDT_MAX_SIZE);
> -        exit(1);
> -    }
> -
>      /* Load the fdt */
>      qemu_fdt_dumpdtb(fdt, fdt_totalsize(fdt));
>      cpu_physical_memory_write(fdt_addr, fdt, fdt_totalsize(fdt));
> 



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [for-5.0 0/4] spapr: Improvements to CAS feature negotiation
  2019-11-29  5:33 [for-5.0 0/4] spapr: Improvements to CAS feature negotiation David Gibson
                   ` (3 preceding siblings ...)
  2019-11-29  5:33 ` [for-5.0 4/4] spapr: Simplify ovec diff David Gibson
@ 2019-12-02  7:05 ` Cédric Le Goater
  2019-12-03  5:00   ` David Gibson
  4 siblings, 1 reply; 16+ messages in thread
From: Cédric Le Goater @ 2019-12-02  7:05 UTC (permalink / raw)
  To: David Gibson, groug; +Cc: lvivier, qemu-ppc, mdroth, qemu-devel

On 29/11/2019 06:33, David Gibson wrote:
> This series contains several cleanups to the handling of the
> ibm,client-architecture-support firmware call used for boot time
> feature negotiation between the guest OS and the firmware &
> hypervisor.
> 
> Mostly it's just internal polish, but one significant user visible
> change is that we no longer generate an extra CAS reboot to switch
> between XICS and XIVE interrupt modes (by far the most common cause of
> CAS reboots in practice).


I love it. thanks for removing this extra reboot.

C. 


> 
> David Gibson (4):
>   spapr: Don't trigger a CAS reboot for XICS/XIVE mode changeover
>   spapr: Improve handling of fdt buffer size
>   spapr: Fold h_cas_compose_response() into
>     h_client_architecture_support()
>   spapr: Simplify ovec diff
> 
>  hw/ppc/spapr.c              | 92 +++----------------------------------
>  hw/ppc/spapr_hcall.c        | 90 +++++++++++++++++++++++++-----------
>  hw/ppc/spapr_ovec.c         | 30 ++++--------
>  include/hw/ppc/spapr.h      |  4 +-
>  include/hw/ppc/spapr_ovec.h |  4 +-
>  5 files changed, 83 insertions(+), 137 deletions(-)
> 



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [for-5.0 1/4] spapr: Don't trigger a CAS reboot for XICS/XIVE mode changeover
  2019-11-29  5:33 ` [for-5.0 1/4] spapr: Don't trigger a CAS reboot for XICS/XIVE mode changeover David Gibson
  2019-12-02  7:03   ` Cédric Le Goater
@ 2019-12-02  7:50   ` Greg Kurz
  2019-12-02  7:59     ` David Gibson
  1 sibling, 1 reply; 16+ messages in thread
From: Greg Kurz @ 2019-12-02  7:50 UTC (permalink / raw)
  To: David Gibson; +Cc: lvivier, qemu-devel, qemu-ppc, clg, mdroth

On Fri, 29 Nov 2019 16:33:53 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> PAPR allows the interrupt controller used on a POWER9 machine (XICS or
> XIVE) to be selected by the guest operating system, by using the
> ibm,client-architecture-support (CAS) feature negotiation call.
> 
> Currently, if the guest selects an interrupt controller different from the
> one selected at initial boot, this causes the system to be reset with the
> new model and the boot starts again.  This means we run through the SLOF
> boot process twice, as well as any other bootloader (e.g. grub) in use
> before the OS calls CAS.  This can be confusing and/or inconvenient for
> users.
> 
> Thanks to two fairly recent changes, we no longer need this reboot.  1) we
> now completely regenerate the device tree when CAS is called (meaning we
> don't need special case updates for all the device tree changes caused by
> the interrupt controller mode change),  2) we now have explicit code paths
> to activate and deactivate the different interrupt controllers, rather than
> just implicitly calling those at machine reset time.
> 
> We can therefore eliminate the reboot for changing irq mode, simply by
> putting a called to spapr_irq_update_active_intc() before we call

putting a call

> spapr_h_cas_compose_response() (which gives the updated device tree to the
> guest firmware and OS).
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---

Convenient indeed ! :)

Reviewed-by: Greg Kurz <groug@kaod.org>

>  hw/ppc/spapr_hcall.c | 33 +++++++++++++--------------------
>  1 file changed, 13 insertions(+), 20 deletions(-)
> 
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index 140f05c1c6..05a7ca275b 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -1767,21 +1767,10 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
>      }
>      spapr->cas_pre_isa3_guest = !spapr_ovec_test(ov1_guest, OV1_PPC_3_00);
>      spapr_ovec_cleanup(ov1_guest);
> -    if (!spapr->cas_reboot) {
> -        /* If spapr_machine_reset() did not set up a HPT but one is necessary
> -         * (because the guest isn't going to use radix) then set it up here. */
> -        if ((spapr->patb_entry & PATE1_GR) && !guest_radix) {
> -            /* legacy hash or new hash: */
> -            spapr_setup_hpt_and_vrma(spapr);
> -        }
> -        spapr->cas_reboot =
> -            (spapr_h_cas_compose_response(spapr, args[1], args[2],
> -                                          ov5_updates) != 0);
> -    }
>  
>      /*
> -     * Ensure the guest asks for an interrupt mode we support; otherwise
> -     * terminate the boot.
> +     * Ensure the guest asks for an interrupt mode we support;
> +     * otherwise terminate the boot.
>       */
>      if (guest_xive) {
>          if (!spapr->irq->xive) {
> @@ -1797,14 +1786,18 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
>          }
>      }
>  
> -    /*
> -     * Generate a machine reset when we have an update of the
> -     * interrupt mode. Only required when the machine supports both
> -     * modes.
> -     */
> +    spapr_irq_update_active_intc(spapr);
> +
>      if (!spapr->cas_reboot) {
> -        spapr->cas_reboot = spapr_ovec_test(ov5_updates, OV5_XIVE_EXPLOIT)
> -            && spapr->irq->xics && spapr->irq->xive;
> +        /* If spapr_machine_reset() did not set up a HPT but one is necessary
> +         * (because the guest isn't going to use radix) then set it up here. */
> +        if ((spapr->patb_entry & PATE1_GR) && !guest_radix) {
> +            /* legacy hash or new hash: */
> +            spapr_setup_hpt_and_vrma(spapr);
> +        }
> +        spapr->cas_reboot =
> +            (spapr_h_cas_compose_response(spapr, args[1], args[2],
> +                                          ov5_updates) != 0);
>      }
>  
>      spapr_ovec_cleanup(ov5_updates);



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [for-5.0 2/4] spapr: Improve handling of fdt buffer size
  2019-11-29  5:33 ` [for-5.0 2/4] spapr: Improve handling of fdt buffer size David Gibson
  2019-12-02  7:04   ` Cédric Le Goater
@ 2019-12-02  7:52   ` Greg Kurz
  1 sibling, 0 replies; 16+ messages in thread
From: Greg Kurz @ 2019-12-02  7:52 UTC (permalink / raw)
  To: David Gibson; +Cc: lvivier, qemu-devel, qemu-ppc, clg, mdroth

On Fri, 29 Nov 2019 16:33:54 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> Previously, spapr_build_fdt() constructed the device tree in a fixed
> buffer of size FDT_MAX_SIZE.  This is a bit inflexible, but more
> importantly it's awkward for the case where we use it during CAS.  In
> that case the guest firmware supplies a buffer and we have to
> awkwardly check that what we generated fits into it afterwards, after
> doing a lot of size checks during spapr_build_fdt().
> 
> Simplify this by having spapr_build_fdt() take a 'space' parameter.
> For the CAS case, we pass in the buffer size provided by SLOF, for the
> machine init case, we continue to pass FDT_MAX_SIZE.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---

Reviewed-by: Greg Kurz <groug@kaod.org>

>  hw/ppc/spapr.c | 33 +++++++++++----------------------
>  1 file changed, 11 insertions(+), 22 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index d9c9a2bcee..d34e317f48 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -917,7 +917,8 @@ static bool spapr_hotplugged_dev_before_cas(void)
>      return false;
>  }
>  
> -static void *spapr_build_fdt(SpaprMachineState *spapr, bool reset);
> +static void *spapr_build_fdt(SpaprMachineState *spapr, bool reset,
> +                             size_t space);
>  
>  int spapr_h_cas_compose_response(SpaprMachineState *spapr,
>                                   target_ulong addr, target_ulong size,
> @@ -930,24 +931,17 @@ int spapr_h_cas_compose_response(SpaprMachineState *spapr,
>          return 1;
>      }
>  
> -    if (size < sizeof(hdr) || size > FW_MAX_SIZE) {
> -        error_report("SLOF provided an unexpected CAS buffer size "
> -                     TARGET_FMT_lu " (min: %zu, max: %u)",
> -                     size, sizeof(hdr), FW_MAX_SIZE);
> +    if (size < sizeof(hdr)) {
> +        error_report("SLOF provided insufficient CAS buffer "
> +                     TARGET_FMT_lu " (min: %zu)", size, sizeof(hdr));
>          exit(EXIT_FAILURE);
>      }
>  
>      size -= sizeof(hdr);
>  
> -    fdt = spapr_build_fdt(spapr, false);
> +    fdt = spapr_build_fdt(spapr, false, size);
>      _FDT((fdt_pack(fdt)));
>  
> -    if (fdt_totalsize(fdt) + sizeof(hdr) > size) {
> -        g_free(fdt);
> -        trace_spapr_cas_failed(size);
> -        return -1;
> -    }
> -
>      cpu_physical_memory_write(addr, &hdr, sizeof(hdr));
>      cpu_physical_memory_write(addr + sizeof(hdr), fdt, fdt_totalsize(fdt));
>      trace_spapr_cas_continue(fdt_totalsize(fdt) + sizeof(hdr));
> @@ -1197,7 +1191,8 @@ static void spapr_dt_hypervisor(SpaprMachineState *spapr, void *fdt)
>      }
>  }
>  
> -static void *spapr_build_fdt(SpaprMachineState *spapr, bool reset)
> +static void *spapr_build_fdt(SpaprMachineState *spapr, bool reset,
> +                             size_t space)
>  {
>      MachineState *machine = MACHINE(spapr);
>      MachineClass *mc = MACHINE_GET_CLASS(machine);
> @@ -1207,8 +1202,8 @@ static void *spapr_build_fdt(SpaprMachineState *spapr, bool reset)
>      SpaprPhbState *phb;
>      char *buf;
>  
> -    fdt = g_malloc0(FDT_MAX_SIZE);
> -    _FDT((fdt_create_empty_tree(fdt, FDT_MAX_SIZE)));
> +    fdt = g_malloc0(space);
> +    _FDT((fdt_create_empty_tree(fdt, space)));
>  
>      /* Root node */
>      _FDT(fdt_setprop_string(fdt, 0, "device_type", "chrp"));
> @@ -1723,19 +1718,13 @@ static void spapr_machine_reset(MachineState *machine)
>       */
>      fdt_addr = MIN(spapr->rma_size, RTAS_MAX_ADDR) - FDT_MAX_SIZE;
>  
> -    fdt = spapr_build_fdt(spapr, true);
> +    fdt = spapr_build_fdt(spapr, true, FDT_MAX_SIZE);
>  
>      rc = fdt_pack(fdt);
>  
>      /* Should only fail if we've built a corrupted tree */
>      assert(rc == 0);
>  
> -    if (fdt_totalsize(fdt) > FDT_MAX_SIZE) {
> -        error_report("FDT too big ! 0x%x bytes (max is 0x%x)",
> -                     fdt_totalsize(fdt), FDT_MAX_SIZE);
> -        exit(1);
> -    }
> -
>      /* Load the fdt */
>      qemu_fdt_dumpdtb(fdt, fdt_totalsize(fdt));
>      cpu_physical_memory_write(fdt_addr, fdt, fdt_totalsize(fdt));



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [for-5.0 1/4] spapr: Don't trigger a CAS reboot for XICS/XIVE mode changeover
  2019-12-02  7:50   ` Greg Kurz
@ 2019-12-02  7:59     ` David Gibson
  0 siblings, 0 replies; 16+ messages in thread
From: David Gibson @ 2019-12-02  7:59 UTC (permalink / raw)
  To: Greg Kurz; +Cc: lvivier, qemu-devel, qemu-ppc, clg, mdroth

[-- Attachment #1: Type: text/plain, Size: 4471 bytes --]

On Mon, Dec 02, 2019 at 08:50:09AM +0100, Greg Kurz wrote:
> On Fri, 29 Nov 2019 16:33:53 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > PAPR allows the interrupt controller used on a POWER9 machine (XICS or
> > XIVE) to be selected by the guest operating system, by using the
> > ibm,client-architecture-support (CAS) feature negotiation call.
> > 
> > Currently, if the guest selects an interrupt controller different from the
> > one selected at initial boot, this causes the system to be reset with the
> > new model and the boot starts again.  This means we run through the SLOF
> > boot process twice, as well as any other bootloader (e.g. grub) in use
> > before the OS calls CAS.  This can be confusing and/or inconvenient for
> > users.
> > 
> > Thanks to two fairly recent changes, we no longer need this reboot.  1) we
> > now completely regenerate the device tree when CAS is called (meaning we
> > don't need special case updates for all the device tree changes caused by
> > the interrupt controller mode change),  2) we now have explicit code paths
> > to activate and deactivate the different interrupt controllers, rather than
> > just implicitly calling those at machine reset time.
> > 
> > We can therefore eliminate the reboot for changing irq mode, simply by
> > putting a called to spapr_irq_update_active_intc() before we call
> 
> putting a call

Fixed, thanks.

> > spapr_h_cas_compose_response() (which gives the updated device tree to the
> > guest firmware and OS).
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> 
> Convenient indeed ! :)
> 
> Reviewed-by: Greg Kurz <groug@kaod.org>
> 
> >  hw/ppc/spapr_hcall.c | 33 +++++++++++++--------------------
> >  1 file changed, 13 insertions(+), 20 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> > index 140f05c1c6..05a7ca275b 100644
> > --- a/hw/ppc/spapr_hcall.c
> > +++ b/hw/ppc/spapr_hcall.c
> > @@ -1767,21 +1767,10 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
> >      }
> >      spapr->cas_pre_isa3_guest = !spapr_ovec_test(ov1_guest, OV1_PPC_3_00);
> >      spapr_ovec_cleanup(ov1_guest);
> > -    if (!spapr->cas_reboot) {
> > -        /* If spapr_machine_reset() did not set up a HPT but one is necessary
> > -         * (because the guest isn't going to use radix) then set it up here. */
> > -        if ((spapr->patb_entry & PATE1_GR) && !guest_radix) {
> > -            /* legacy hash or new hash: */
> > -            spapr_setup_hpt_and_vrma(spapr);
> > -        }
> > -        spapr->cas_reboot =
> > -            (spapr_h_cas_compose_response(spapr, args[1], args[2],
> > -                                          ov5_updates) != 0);
> > -    }
> >  
> >      /*
> > -     * Ensure the guest asks for an interrupt mode we support; otherwise
> > -     * terminate the boot.
> > +     * Ensure the guest asks for an interrupt mode we support;
> > +     * otherwise terminate the boot.
> >       */
> >      if (guest_xive) {
> >          if (!spapr->irq->xive) {
> > @@ -1797,14 +1786,18 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
> >          }
> >      }
> >  
> > -    /*
> > -     * Generate a machine reset when we have an update of the
> > -     * interrupt mode. Only required when the machine supports both
> > -     * modes.
> > -     */
> > +    spapr_irq_update_active_intc(spapr);
> > +
> >      if (!spapr->cas_reboot) {
> > -        spapr->cas_reboot = spapr_ovec_test(ov5_updates, OV5_XIVE_EXPLOIT)
> > -            && spapr->irq->xics && spapr->irq->xive;
> > +        /* If spapr_machine_reset() did not set up a HPT but one is necessary
> > +         * (because the guest isn't going to use radix) then set it up here. */
> > +        if ((spapr->patb_entry & PATE1_GR) && !guest_radix) {
> > +            /* legacy hash or new hash: */
> > +            spapr_setup_hpt_and_vrma(spapr);
> > +        }
> > +        spapr->cas_reboot =
> > +            (spapr_h_cas_compose_response(spapr, args[1], args[2],
> > +                                          ov5_updates) != 0);
> >      }
> >  
> >      spapr_ovec_cleanup(ov5_updates);
> 

-- 
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 --]

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [for-5.0 3/4] spapr: Fold h_cas_compose_response() into h_client_architecture_support()
  2019-11-29  5:33 ` [for-5.0 3/4] spapr: Fold h_cas_compose_response() into h_client_architecture_support() David Gibson
  2019-12-02  7:04   ` Cédric Le Goater
@ 2019-12-02  8:23   ` Greg Kurz
  2019-12-02  8:40     ` David Gibson
  1 sibling, 1 reply; 16+ messages in thread
From: Greg Kurz @ 2019-12-02  8:23 UTC (permalink / raw)
  To: David Gibson; +Cc: lvivier, qemu-devel, qemu-ppc, clg, mdroth

On Fri, 29 Nov 2019 16:33:55 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> spapr_h_cas_compose_response() handles the last piece of the PAPR feature
> negotiation process invoked via the ibm,client-architecture-support OF
> call.  Its only caller is h_client_architecture_support() which handles
> most of the rest of that process.
> 
> I believe it was place in a separate file originally to handle some fiddly

it was placed

> dependencies between functions, but mostly it's just confusing to have
> the CAS process split into two pieces like this.  Now that compose response
> is simplified (by just generating the whole device tree anew), it's cleaner
> to just fold it into h_client_architecture_support().
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---

Reviewed-by: Greg Kurz <groug@kaod.org>

>  hw/ppc/spapr.c         | 61 +-----------------------------------------
>  hw/ppc/spapr_hcall.c   | 55 ++++++++++++++++++++++++++++++++++---
>  include/hw/ppc/spapr.h |  4 +--
>  3 files changed, 54 insertions(+), 66 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index d34e317f48..5187f5b0a5 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -76,7 +76,6 @@
>  #include "hw/nmi.h"
>  #include "hw/intc/intc.h"
>  
> -#include "qemu/cutils.h"
>  #include "hw/ppc/spapr_cpu_core.h"
>  #include "hw/mem/memory-device.h"
>  #include "hw/ppc/spapr_tpm_proxy.h"
> @@ -897,63 +896,6 @@ out:
>      return ret;
>  }
>  
> -static bool spapr_hotplugged_dev_before_cas(void)
> -{
> -    Object *drc_container, *obj;
> -    ObjectProperty *prop;
> -    ObjectPropertyIterator iter;
> -
> -    drc_container = container_get(object_get_root(), "/dr-connector");
> -    object_property_iter_init(&iter, drc_container);
> -    while ((prop = object_property_iter_next(&iter))) {
> -        if (!strstart(prop->type, "link<", NULL)) {
> -            continue;
> -        }
> -        obj = object_property_get_link(drc_container, prop->name, NULL);
> -        if (spapr_drc_needed(obj)) {
> -            return true;
> -        }
> -    }
> -    return false;
> -}
> -
> -static void *spapr_build_fdt(SpaprMachineState *spapr, bool reset,
> -                             size_t space);
> -
> -int spapr_h_cas_compose_response(SpaprMachineState *spapr,
> -                                 target_ulong addr, target_ulong size,
> -                                 SpaprOptionVector *ov5_updates)
> -{
> -    void *fdt;
> -    SpaprDeviceTreeUpdateHeader hdr = { .version_id = 1 };
> -
> -    if (spapr_hotplugged_dev_before_cas()) {
> -        return 1;
> -    }
> -
> -    if (size < sizeof(hdr)) {
> -        error_report("SLOF provided insufficient CAS buffer "
> -                     TARGET_FMT_lu " (min: %zu)", size, sizeof(hdr));
> -        exit(EXIT_FAILURE);
> -    }
> -
> -    size -= sizeof(hdr);
> -
> -    fdt = spapr_build_fdt(spapr, false, size);
> -    _FDT((fdt_pack(fdt)));
> -
> -    cpu_physical_memory_write(addr, &hdr, sizeof(hdr));
> -    cpu_physical_memory_write(addr + sizeof(hdr), fdt, fdt_totalsize(fdt));
> -    trace_spapr_cas_continue(fdt_totalsize(fdt) + sizeof(hdr));
> -
> -    g_free(spapr->fdt_blob);
> -    spapr->fdt_size = fdt_totalsize(fdt);
> -    spapr->fdt_initial_size = spapr->fdt_size;
> -    spapr->fdt_blob = fdt;
> -
> -    return 0;
> -}
> -
>  static void spapr_dt_rtas(SpaprMachineState *spapr, void *fdt)
>  {
>      MachineState *ms = MACHINE(spapr);
> @@ -1191,8 +1133,7 @@ static void spapr_dt_hypervisor(SpaprMachineState *spapr, void *fdt)
>      }
>  }
>  
> -static void *spapr_build_fdt(SpaprMachineState *spapr, bool reset,
> -                             size_t space)
> +void *spapr_build_fdt(SpaprMachineState *spapr, bool reset, size_t space)
>  {
>      MachineState *machine = MACHINE(spapr);
>      MachineClass *mc = MACHINE_GET_CLASS(machine);
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index 05a7ca275b..0f19be794c 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -1,4 +1,5 @@
>  #include "qemu/osdep.h"
> +#include "qemu/cutils.h"
>  #include "qapi/error.h"
>  #include "sysemu/hw_accel.h"
>  #include "sysemu/runstate.h"
> @@ -15,6 +16,7 @@
>  #include "cpu-models.h"
>  #include "trace.h"
>  #include "kvm_ppc.h"
> +#include "hw/ppc/fdt.h"
>  #include "hw/ppc/spapr_ovec.h"
>  #include "mmu-book3s-v3.h"
>  #include "hw/mem/memory-device.h"
> @@ -1638,6 +1640,26 @@ static uint32_t cas_check_pvr(SpaprMachineState *spapr, PowerPCCPU *cpu,
>      return best_compat;
>  }
>  
> +static bool spapr_hotplugged_dev_before_cas(void)
> +{
> +    Object *drc_container, *obj;
> +    ObjectProperty *prop;
> +    ObjectPropertyIterator iter;
> +
> +    drc_container = container_get(object_get_root(), "/dr-connector");
> +    object_property_iter_init(&iter, drc_container);
> +    while ((prop = object_property_iter_next(&iter))) {
> +        if (!strstart(prop->type, "link<", NULL)) {
> +            continue;
> +        }
> +        obj = object_property_get_link(drc_container, prop->name, NULL);
> +        if (spapr_drc_needed(obj)) {
> +            return true;
> +        }
> +    }
> +    return false;
> +}
> +
>  static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
>                                                    SpaprMachineState *spapr,
>                                                    target_ulong opcode,
> @@ -1645,6 +1667,8 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
>  {
>      /* Working address in data buffer */
>      target_ulong addr = ppc64_phys_to_real(args[0]);
> +    target_ulong fdt_buf = args[1];
> +    target_ulong fdt_bufsize = args[2];
>      target_ulong ov_table;
>      uint32_t cas_pvr;
>      SpaprOptionVector *ov1_guest, *ov5_guest, *ov5_cas_old, *ov5_updates;
> @@ -1788,16 +1812,41 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
>  
>      spapr_irq_update_active_intc(spapr);
>  
> +    if (spapr_hotplugged_dev_before_cas()) {
> +        spapr->cas_reboot = true;
> +    }
> +
>      if (!spapr->cas_reboot) {
> +        void *fdt;
> +        SpaprDeviceTreeUpdateHeader hdr = { .version_id = 1 };
> +
>          /* If spapr_machine_reset() did not set up a HPT but one is necessary
>           * (because the guest isn't going to use radix) then set it up here. */
>          if ((spapr->patb_entry & PATE1_GR) && !guest_radix) {
>              /* legacy hash or new hash: */
>              spapr_setup_hpt_and_vrma(spapr);
>          }
> -        spapr->cas_reboot =
> -            (spapr_h_cas_compose_response(spapr, args[1], args[2],
> -                                          ov5_updates) != 0);
> +
> +        if (fdt_bufsize < sizeof(hdr)) {
> +            error_report("SLOF provided insufficient CAS buffer "
> +                         TARGET_FMT_lu " (min: %zu)", fdt_bufsize, sizeof(hdr));
> +            exit(EXIT_FAILURE);
> +        }
> +
> +        fdt_bufsize -= sizeof(hdr);
> +
> +        fdt = spapr_build_fdt(spapr, false, fdt_bufsize);
> +        _FDT((fdt_pack(fdt)));
> +
> +        cpu_physical_memory_write(fdt_buf, &hdr, sizeof(hdr));
> +        cpu_physical_memory_write(fdt_buf + sizeof(hdr), fdt,
> +                                  fdt_totalsize(fdt));
> +        trace_spapr_cas_continue(fdt_totalsize(fdt) + sizeof(hdr));
> +
> +        g_free(spapr->fdt_blob);
> +        spapr->fdt_size = fdt_totalsize(fdt);
> +        spapr->fdt_initial_size = spapr->fdt_size;
> +        spapr->fdt_blob = fdt;
>      }
>  
>      spapr_ovec_cleanup(ov5_updates);
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index d5ab5ea7b2..61f005c6f6 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -766,11 +766,9 @@ struct SpaprEventLogEntry {
>      QTAILQ_ENTRY(SpaprEventLogEntry) next;
>  };
>  
> +void *spapr_build_fdt(SpaprMachineState *spapr, bool reset, size_t space);
>  void spapr_events_init(SpaprMachineState *sm);
>  void spapr_dt_events(SpaprMachineState *sm, void *fdt);
> -int spapr_h_cas_compose_response(SpaprMachineState *sm,
> -                                 target_ulong addr, target_ulong size,
> -                                 SpaprOptionVector *ov5_updates);
>  void close_htab_fd(SpaprMachineState *spapr);
>  void spapr_setup_hpt_and_vrma(SpaprMachineState *spapr);
>  void spapr_free_hpt(SpaprMachineState *spapr);



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [for-5.0 3/4] spapr: Fold h_cas_compose_response() into h_client_architecture_support()
  2019-12-02  8:23   ` Greg Kurz
@ 2019-12-02  8:40     ` David Gibson
  0 siblings, 0 replies; 16+ messages in thread
From: David Gibson @ 2019-12-02  8:40 UTC (permalink / raw)
  To: Greg Kurz; +Cc: lvivier, qemu-devel, qemu-ppc, clg, mdroth

[-- Attachment #1: Type: text/plain, Size: 9352 bytes --]

On Mon, Dec 02, 2019 at 09:23:35AM +0100, Greg Kurz wrote:
> On Fri, 29 Nov 2019 16:33:55 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > spapr_h_cas_compose_response() handles the last piece of the PAPR feature
> > negotiation process invoked via the ibm,client-architecture-support OF
> > call.  Its only caller is h_client_architecture_support() which handles
> > most of the rest of that process.
> > 
> > I believe it was place in a separate file originally to handle some fiddly
> 
> it was placed

fixed, thanks.

> 
> > dependencies between functions, but mostly it's just confusing to have
> > the CAS process split into two pieces like this.  Now that compose response
> > is simplified (by just generating the whole device tree anew), it's cleaner
> > to just fold it into h_client_architecture_support().
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> 
> Reviewed-by: Greg Kurz <groug@kaod.org>
> 
> >  hw/ppc/spapr.c         | 61 +-----------------------------------------
> >  hw/ppc/spapr_hcall.c   | 55 ++++++++++++++++++++++++++++++++++---
> >  include/hw/ppc/spapr.h |  4 +--
> >  3 files changed, 54 insertions(+), 66 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index d34e317f48..5187f5b0a5 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -76,7 +76,6 @@
> >  #include "hw/nmi.h"
> >  #include "hw/intc/intc.h"
> >  
> > -#include "qemu/cutils.h"
> >  #include "hw/ppc/spapr_cpu_core.h"
> >  #include "hw/mem/memory-device.h"
> >  #include "hw/ppc/spapr_tpm_proxy.h"
> > @@ -897,63 +896,6 @@ out:
> >      return ret;
> >  }
> >  
> > -static bool spapr_hotplugged_dev_before_cas(void)
> > -{
> > -    Object *drc_container, *obj;
> > -    ObjectProperty *prop;
> > -    ObjectPropertyIterator iter;
> > -
> > -    drc_container = container_get(object_get_root(), "/dr-connector");
> > -    object_property_iter_init(&iter, drc_container);
> > -    while ((prop = object_property_iter_next(&iter))) {
> > -        if (!strstart(prop->type, "link<", NULL)) {
> > -            continue;
> > -        }
> > -        obj = object_property_get_link(drc_container, prop->name, NULL);
> > -        if (spapr_drc_needed(obj)) {
> > -            return true;
> > -        }
> > -    }
> > -    return false;
> > -}
> > -
> > -static void *spapr_build_fdt(SpaprMachineState *spapr, bool reset,
> > -                             size_t space);
> > -
> > -int spapr_h_cas_compose_response(SpaprMachineState *spapr,
> > -                                 target_ulong addr, target_ulong size,
> > -                                 SpaprOptionVector *ov5_updates)
> > -{
> > -    void *fdt;
> > -    SpaprDeviceTreeUpdateHeader hdr = { .version_id = 1 };
> > -
> > -    if (spapr_hotplugged_dev_before_cas()) {
> > -        return 1;
> > -    }
> > -
> > -    if (size < sizeof(hdr)) {
> > -        error_report("SLOF provided insufficient CAS buffer "
> > -                     TARGET_FMT_lu " (min: %zu)", size, sizeof(hdr));
> > -        exit(EXIT_FAILURE);
> > -    }
> > -
> > -    size -= sizeof(hdr);
> > -
> > -    fdt = spapr_build_fdt(spapr, false, size);
> > -    _FDT((fdt_pack(fdt)));
> > -
> > -    cpu_physical_memory_write(addr, &hdr, sizeof(hdr));
> > -    cpu_physical_memory_write(addr + sizeof(hdr), fdt, fdt_totalsize(fdt));
> > -    trace_spapr_cas_continue(fdt_totalsize(fdt) + sizeof(hdr));
> > -
> > -    g_free(spapr->fdt_blob);
> > -    spapr->fdt_size = fdt_totalsize(fdt);
> > -    spapr->fdt_initial_size = spapr->fdt_size;
> > -    spapr->fdt_blob = fdt;
> > -
> > -    return 0;
> > -}
> > -
> >  static void spapr_dt_rtas(SpaprMachineState *spapr, void *fdt)
> >  {
> >      MachineState *ms = MACHINE(spapr);
> > @@ -1191,8 +1133,7 @@ static void spapr_dt_hypervisor(SpaprMachineState *spapr, void *fdt)
> >      }
> >  }
> >  
> > -static void *spapr_build_fdt(SpaprMachineState *spapr, bool reset,
> > -                             size_t space)
> > +void *spapr_build_fdt(SpaprMachineState *spapr, bool reset, size_t space)
> >  {
> >      MachineState *machine = MACHINE(spapr);
> >      MachineClass *mc = MACHINE_GET_CLASS(machine);
> > diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> > index 05a7ca275b..0f19be794c 100644
> > --- a/hw/ppc/spapr_hcall.c
> > +++ b/hw/ppc/spapr_hcall.c
> > @@ -1,4 +1,5 @@
> >  #include "qemu/osdep.h"
> > +#include "qemu/cutils.h"
> >  #include "qapi/error.h"
> >  #include "sysemu/hw_accel.h"
> >  #include "sysemu/runstate.h"
> > @@ -15,6 +16,7 @@
> >  #include "cpu-models.h"
> >  #include "trace.h"
> >  #include "kvm_ppc.h"
> > +#include "hw/ppc/fdt.h"
> >  #include "hw/ppc/spapr_ovec.h"
> >  #include "mmu-book3s-v3.h"
> >  #include "hw/mem/memory-device.h"
> > @@ -1638,6 +1640,26 @@ static uint32_t cas_check_pvr(SpaprMachineState *spapr, PowerPCCPU *cpu,
> >      return best_compat;
> >  }
> >  
> > +static bool spapr_hotplugged_dev_before_cas(void)
> > +{
> > +    Object *drc_container, *obj;
> > +    ObjectProperty *prop;
> > +    ObjectPropertyIterator iter;
> > +
> > +    drc_container = container_get(object_get_root(), "/dr-connector");
> > +    object_property_iter_init(&iter, drc_container);
> > +    while ((prop = object_property_iter_next(&iter))) {
> > +        if (!strstart(prop->type, "link<", NULL)) {
> > +            continue;
> > +        }
> > +        obj = object_property_get_link(drc_container, prop->name, NULL);
> > +        if (spapr_drc_needed(obj)) {
> > +            return true;
> > +        }
> > +    }
> > +    return false;
> > +}
> > +
> >  static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
> >                                                    SpaprMachineState *spapr,
> >                                                    target_ulong opcode,
> > @@ -1645,6 +1667,8 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
> >  {
> >      /* Working address in data buffer */
> >      target_ulong addr = ppc64_phys_to_real(args[0]);
> > +    target_ulong fdt_buf = args[1];
> > +    target_ulong fdt_bufsize = args[2];
> >      target_ulong ov_table;
> >      uint32_t cas_pvr;
> >      SpaprOptionVector *ov1_guest, *ov5_guest, *ov5_cas_old, *ov5_updates;
> > @@ -1788,16 +1812,41 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
> >  
> >      spapr_irq_update_active_intc(spapr);
> >  
> > +    if (spapr_hotplugged_dev_before_cas()) {
> > +        spapr->cas_reboot = true;
> > +    }
> > +
> >      if (!spapr->cas_reboot) {
> > +        void *fdt;
> > +        SpaprDeviceTreeUpdateHeader hdr = { .version_id = 1 };
> > +
> >          /* If spapr_machine_reset() did not set up a HPT but one is necessary
> >           * (because the guest isn't going to use radix) then set it up here. */
> >          if ((spapr->patb_entry & PATE1_GR) && !guest_radix) {
> >              /* legacy hash or new hash: */
> >              spapr_setup_hpt_and_vrma(spapr);
> >          }
> > -        spapr->cas_reboot =
> > -            (spapr_h_cas_compose_response(spapr, args[1], args[2],
> > -                                          ov5_updates) != 0);
> > +
> > +        if (fdt_bufsize < sizeof(hdr)) {
> > +            error_report("SLOF provided insufficient CAS buffer "
> > +                         TARGET_FMT_lu " (min: %zu)", fdt_bufsize, sizeof(hdr));
> > +            exit(EXIT_FAILURE);
> > +        }
> > +
> > +        fdt_bufsize -= sizeof(hdr);
> > +
> > +        fdt = spapr_build_fdt(spapr, false, fdt_bufsize);
> > +        _FDT((fdt_pack(fdt)));
> > +
> > +        cpu_physical_memory_write(fdt_buf, &hdr, sizeof(hdr));
> > +        cpu_physical_memory_write(fdt_buf + sizeof(hdr), fdt,
> > +                                  fdt_totalsize(fdt));
> > +        trace_spapr_cas_continue(fdt_totalsize(fdt) + sizeof(hdr));
> > +
> > +        g_free(spapr->fdt_blob);
> > +        spapr->fdt_size = fdt_totalsize(fdt);
> > +        spapr->fdt_initial_size = spapr->fdt_size;
> > +        spapr->fdt_blob = fdt;
> >      }
> >  
> >      spapr_ovec_cleanup(ov5_updates);
> > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> > index d5ab5ea7b2..61f005c6f6 100644
> > --- a/include/hw/ppc/spapr.h
> > +++ b/include/hw/ppc/spapr.h
> > @@ -766,11 +766,9 @@ struct SpaprEventLogEntry {
> >      QTAILQ_ENTRY(SpaprEventLogEntry) next;
> >  };
> >  
> > +void *spapr_build_fdt(SpaprMachineState *spapr, bool reset, size_t space);
> >  void spapr_events_init(SpaprMachineState *sm);
> >  void spapr_dt_events(SpaprMachineState *sm, void *fdt);
> > -int spapr_h_cas_compose_response(SpaprMachineState *sm,
> > -                                 target_ulong addr, target_ulong size,
> > -                                 SpaprOptionVector *ov5_updates);
> >  void close_htab_fd(SpaprMachineState *spapr);
> >  void spapr_setup_hpt_and_vrma(SpaprMachineState *spapr);
> >  void spapr_free_hpt(SpaprMachineState *spapr);
> 

-- 
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 --]

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [for-5.0 0/4] spapr: Improvements to CAS feature negotiation
  2019-12-02  7:05 ` [for-5.0 0/4] spapr: Improvements to CAS feature negotiation Cédric Le Goater
@ 2019-12-03  5:00   ` David Gibson
  0 siblings, 0 replies; 16+ messages in thread
From: David Gibson @ 2019-12-03  5:00 UTC (permalink / raw)
  To: Cédric Le Goater; +Cc: lvivier, qemu-devel, qemu-ppc, groug, mdroth

[-- Attachment #1: Type: text/plain, Size: 1561 bytes --]

On Mon, Dec 02, 2019 at 08:05:13AM +0100, Cédric Le Goater wrote:
> On 29/11/2019 06:33, David Gibson wrote:
> > This series contains several cleanups to the handling of the
> > ibm,client-architecture-support firmware call used for boot time
> > feature negotiation between the guest OS and the firmware &
> > hypervisor.
> > 
> > Mostly it's just internal polish, but one significant user visible
> > change is that we no longer generate an extra CAS reboot to switch
> > between XICS and XIVE interrupt modes (by far the most common cause of
> > CAS reboots in practice).
> 
> 
> I love it. thanks for removing this extra reboot.

Glad you like it.  I've folded this into ppc-for-5.0 now.

> 
> C. 
> 
> 
> > 
> > David Gibson (4):
> >   spapr: Don't trigger a CAS reboot for XICS/XIVE mode changeover
> >   spapr: Improve handling of fdt buffer size
> >   spapr: Fold h_cas_compose_response() into
> >     h_client_architecture_support()
> >   spapr: Simplify ovec diff
> > 
> >  hw/ppc/spapr.c              | 92 +++----------------------------------
> >  hw/ppc/spapr_hcall.c        | 90 +++++++++++++++++++++++++-----------
> >  hw/ppc/spapr_ovec.c         | 30 ++++--------
> >  include/hw/ppc/spapr.h      |  4 +-
> >  include/hw/ppc/spapr_ovec.h |  4 +-
> >  5 files changed, 83 insertions(+), 137 deletions(-)
> > 
> 

-- 
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 --]

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2019-12-03  5:04 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-29  5:33 [for-5.0 0/4] spapr: Improvements to CAS feature negotiation David Gibson
2019-11-29  5:33 ` [for-5.0 1/4] spapr: Don't trigger a CAS reboot for XICS/XIVE mode changeover David Gibson
2019-12-02  7:03   ` Cédric Le Goater
2019-12-02  7:50   ` Greg Kurz
2019-12-02  7:59     ` David Gibson
2019-11-29  5:33 ` [for-5.0 2/4] spapr: Improve handling of fdt buffer size David Gibson
2019-12-02  7:04   ` Cédric Le Goater
2019-12-02  7:52   ` Greg Kurz
2019-11-29  5:33 ` [for-5.0 3/4] spapr: Fold h_cas_compose_response() into h_client_architecture_support() David Gibson
2019-12-02  7:04   ` Cédric Le Goater
2019-12-02  8:23   ` Greg Kurz
2019-12-02  8:40     ` David Gibson
2019-11-29  5:33 ` [for-5.0 4/4] spapr: Simplify ovec diff David Gibson
2019-12-02  7:04   ` Cédric Le Goater
2019-12-02  7:05 ` [for-5.0 0/4] spapr: Improvements to CAS feature negotiation Cédric Le Goater
2019-12-03  5:00   ` David Gibson

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).