[Qemu-devel] [QEMU-PPC] [RFC 0/3] target/ppc: Rework spapr_caps

[Qemu-devel] [QEMU-PPC] [RFC 0/3] target/ppc: Rework spapr_caps

[Suraj Jitindar Singh]

The following patch series reworks the implementation of spapr_caps
to allow for a increased number of possible values in the internal
representation.

It also adds 3 new tristate capabilities.

A new H-Call is implemented which a guest will use to query the 
requirement for and availability of workarounds for certain cpu 
behaviours.

Suraj Jitindar Singh (3):
  hw/ppc/spapr_caps: Rework spapr_caps to use uint8 internal
    representation
  hw/spapr/spapr_caps: Add new caps
    safe_[cache/bounds_check/indirect_branch]
  target/ppc: Add H-Call H_GET_CPU_CHARACTERISTICS

 hw/ppc/spapr.c            |  25 ++-
 hw/ppc/spapr_caps.c       | 555 ++++++++++++++++++++++++++++++++++------------
 hw/ppc/spapr_hcall.c      |  81 +++++++
 include/hw/ppc/spapr.h    |  57 +++--
 linux-headers/linux/kvm.h |   3 +
 target/ppc/kvm.c          |  28 +++
 target/ppc/kvm_ppc.h      |  18 ++
 7 files changed, 595 insertions(+), 172 deletions(-)

-- 
2.13.6

[Qemu-devel] [QEMU-PPC] [RFC 1/3] hw/ppc/spapr_caps: Rework spapr_caps to use uint8 internal representation

[Suraj Jitindar Singh]

Currently spapr_caps are tied to boolean values (on or off). This patch
reworks the caps so that they can have any value between 0 and 127,
inclusive. This allows more capabilities with various values to be
represented in the same way internally. Capabilities are numbered in
ascending order. The internal representation of capability values is an
array of uint8s in the sPAPRMachineState, indexed by capability number.
Note: The MSB (0x80) of a capability is reserved to track whether the
      capability was set from the command line.

Capabilities can have their own name, description, options, getter and
setter functions, type and allow functions. They also each have their own
section in the migration stream. Capabilities are only migrated if they
were explictly set on the command line, with the assumption that
otherwise the default will match.

On migration we ensure that the capability value on the destination
is greater than or equal to the capability value from the source. So
long at this remains the case then the migration is considered
compatible and allowed to continue.

This patch implements generic getter and setter functions for boolean
capabilities. It also converts the existings cap-htm, cap-vsx and
cap-dfp capabilities to this new format.
---
 hw/ppc/spapr.c         |  19 +--
 hw/ppc/spapr_caps.c    | 335 ++++++++++++++++++++++++++++---------------------
 include/hw/ppc/spapr.h |  41 +++---
 3 files changed, 222 insertions(+), 173 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index d1acfe8858..7fa45729ba 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -320,7 +320,7 @@ static void spapr_populate_pa_features(sPAPRMachineState *spapr,
          */
         pa_features[3] |= 0x20;
     }
-    if (spapr_has_cap(spapr, SPAPR_CAP_HTM) && pa_size > 24) {
+    if (spapr_get_cap(spapr, SPAPR_CAP_HTM) && pa_size > 24) {
         pa_features[24] |= 0x80;    /* Transactional memory support */
     }
     if (legacy_guest && pa_size > 40) {
@@ -563,7 +563,7 @@ static void spapr_populate_cpu_dt(CPUState *cs, void *fdt, int offset,
      *
      * Only CPUs for which we create core types in spapr_cpu_core.c
      * are possible, and all of those have VMX */
-    if (spapr_has_cap(spapr, SPAPR_CAP_VSX)) {
+    if (spapr_get_cap(spapr, SPAPR_CAP_VSX)) {
         _FDT((fdt_setprop_cell(fdt, offset, "ibm,vmx", 2)));
     } else {
         _FDT((fdt_setprop_cell(fdt, offset, "ibm,vmx", 1)));
@@ -572,7 +572,7 @@ static void spapr_populate_cpu_dt(CPUState *cs, void *fdt, int offset,
     /* Advertise DFP (Decimal Floating Point) if available
      *   0 / no property == no DFP
      *   1               == DFP available */
-    if (spapr_has_cap(spapr, SPAPR_CAP_DFP)) {
+    if (spapr_get_cap(spapr, SPAPR_CAP_DFP)) {
         _FDT((fdt_setprop_cell(fdt, offset, "ibm,dfp", 1)));
     }
 
@@ -1762,7 +1762,9 @@ static const VMStateDescription vmstate_spapr = {
         &vmstate_spapr_ov5_cas,
         &vmstate_spapr_patb_entry,
         &vmstate_spapr_pending_events,
-        &vmstate_spapr_caps,
+        &vmstate_spapr_cap_htm,
+        &vmstate_spapr_cap_vsx,
+        &vmstate_spapr_cap_dfp,
         NULL
     }
 };
@@ -2323,8 +2325,6 @@ static void spapr_machine_init(MachineState *machine)
     char *filename;
     Error *resize_hpt_err = NULL;
 
-    spapr_caps_validate(spapr, &error_fatal);
-
     msi_nonbroken = true;
 
     QLIST_INIT(&spapr->phbs);
@@ -3834,7 +3834,9 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
      */
     mc->numa_mem_align_shift = 28;
 
-    smc->default_caps = spapr_caps(SPAPR_CAP_VSX | SPAPR_CAP_DFP);
+    smc->default_caps.caps[SPAPR_CAP_HTM] = SPAPR_CAP_OFF;
+    smc->default_caps.caps[SPAPR_CAP_VSX] = SPAPR_CAP_ON;
+    smc->default_caps.caps[SPAPR_CAP_DFP] = SPAPR_CAP_ON;
     spapr_caps_add_properties(smc, &error_abort);
 }
 
@@ -3916,8 +3918,7 @@ static void spapr_machine_2_11_class_options(MachineClass *mc)
     sPAPRMachineClass *smc = SPAPR_MACHINE_CLASS(mc);
 
     spapr_machine_2_12_class_options(mc);
-    smc->default_caps = spapr_caps(SPAPR_CAP_HTM | SPAPR_CAP_VSX
-                                   | SPAPR_CAP_DFP);
+    smc->default_caps.caps[SPAPR_CAP_HTM] = SPAPR_CAP_ON;
     SET_MACHINE_COMPAT(mc, SPAPR_COMPAT_2_11);
 }
 
diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
index 9d070a306c..af40f2e469 100644
--- a/hw/ppc/spapr_caps.c
+++ b/hw/ppc/spapr_caps.c
@@ -35,33 +35,71 @@
 typedef struct sPAPRCapabilityInfo {
     const char *name;
     const char *description;
-    uint64_t flag;
+    const char *options;
+    int index;
 
+    /* Getter and Setter Function Pointers */
+    ObjectPropertyAccessor *get;
+    ObjectPropertyAccessor *set;
+    const char *type;
     /* Make sure the virtual hardware can support this capability */
-    void (*allow)(sPAPRMachineState *spapr, Error **errp);
-
-    /* If possible, tell the virtual hardware not to allow the cap to
-     * be used at all */
-    void (*disallow)(sPAPRMachineState *spapr, Error **errp);
+    void (*allow)(sPAPRMachineState *spapr, uint8_t val, Error **errp);
 } sPAPRCapabilityInfo;
 
-static void cap_htm_allow(sPAPRMachineState *spapr, Error **errp)
+static void spapr_cap_get_bool(Object *obj, Visitor *v, const char *name,
+                               void *opaque, Error **errp)
+{
+    sPAPRCapabilityInfo *cap = opaque;
+    sPAPRMachineState *spapr = SPAPR_MACHINE(obj);
+    bool value = spapr_get_cap(spapr, cap->index) == SPAPR_CAP_ON;
+
+    visit_type_bool(v, name, &value, errp);
+}
+
+static void spapr_cap_set_bool(Object *obj, Visitor *v, const char *name,
+                               void *opaque, Error **errp)
 {
+    sPAPRCapabilityInfo *cap = opaque;
+    sPAPRMachineState *spapr = SPAPR_MACHINE(obj);
+    bool value;
+    Error *local_err = NULL;
+
+    visit_type_bool(v, name, &value, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
+
+    spapr->cmd_line_caps.caps[cap->index] = (value ? SPAPR_CAP_ON :
+                                                     SPAPR_CAP_OFF) |
+                                             SPAPR_CAP_CMD_LINE;
+}
+
+static void cap_htm_allow(sPAPRMachineState *spapr, uint8_t val, Error **errp)
+{
+    if (!val) {
+        /* TODO: We don't support disabling htm yet */
+        return;
+    }
     if (tcg_enabled()) {
         error_setg(errp,
-                   "No Transactional Memory support in TCG, try cap-htm=off");
+                   "No Transactional Memory support in TCG, try cap-htm=0");
     } else if (kvm_enabled() && !kvmppc_has_cap_htm()) {
         error_setg(errp,
-"KVM implementation does not support Transactional Memory, try cap-htm=off"
+"KVM implementation does not support Transactional Memory, try cap-htm=0"
             );
     }
 }
 
-static void cap_vsx_allow(sPAPRMachineState *spapr, Error **errp)
+static void cap_vsx_allow(sPAPRMachineState *spapr, uint8_t val, Error **errp)
 {
     PowerPCCPU *cpu = POWERPC_CPU(first_cpu);
     CPUPPCState *env = &cpu->env;
 
+    if (!val) {
+        /* TODO: We don't support disabling vsx yet */
+        return;
+    }
     /* Allowable CPUs in spapr_cpu_core.c should already have gotten
      * rid of anything that doesn't do VMX */
     g_assert(env->insns_flags & PPC_ALTIVEC);
@@ -70,37 +108,51 @@ static void cap_vsx_allow(sPAPRMachineState *spapr, Error **errp)
     }
 }
 
-static void cap_dfp_allow(sPAPRMachineState *spapr, Error **errp)
+static void cap_dfp_allow(sPAPRMachineState *spapr, uint8_t val, Error **errp)
 {
     PowerPCCPU *cpu = POWERPC_CPU(first_cpu);
     CPUPPCState *env = &cpu->env;
 
+    if (!val) {
+        /* TODO: We don't support disabling dfp yet */
+        return;
+    }
     if (!(env->insns_flags2 & PPC2_DFP)) {
         error_setg(errp, "DFP support not available, try cap-dfp=off");
     }
 }
 
-static sPAPRCapabilityInfo capability_table[] = {
-    {
+
+sPAPRCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
+    [SPAPR_CAP_HTM] = {
         .name = "htm",
         .description = "Allow Hardware Transactional Memory (HTM)",
-        .flag = SPAPR_CAP_HTM,
+        .options = "",
+        .index = SPAPR_CAP_HTM,
+        .get = spapr_cap_get_bool,
+        .set = spapr_cap_set_bool,
+        .type = "bool",
         .allow = cap_htm_allow,
-        /* TODO: add cap_htm_disallow */
     },
-    {
+    [SPAPR_CAP_VSX] = {
         .name = "vsx",
         .description = "Allow Vector Scalar Extensions (VSX)",
-        .flag = SPAPR_CAP_VSX,
+        .options = "",
+        .index = SPAPR_CAP_VSX,
+        .get = spapr_cap_get_bool,
+        .set = spapr_cap_set_bool,
+        .type = "bool",
         .allow = cap_vsx_allow,
-        /* TODO: add cap_vsx_disallow */
     },
-    {
+    [SPAPR_CAP_DFP] = {
         .name = "dfp",
         .description = "Allow Decimal Floating Point (DFP)",
-        .flag = SPAPR_CAP_DFP,
+        .options = "",
+        .index = SPAPR_CAP_DFP,
+        .get = spapr_cap_get_bool,
+        .set = spapr_cap_set_bool,
+        .type = "bool",
         .allow = cap_dfp_allow,
-        /* TODO: add cap_dfp_disallow */
     },
 };
 
@@ -115,201 +167,192 @@ static sPAPRCapabilities default_caps_with_cpu(sPAPRMachineState *spapr,
 
     if (!ppc_check_compat(cpu, CPU_POWERPC_LOGICAL_2_07,
                           0, spapr->max_compat_pvr)) {
-        caps.mask &= ~SPAPR_CAP_HTM;
+        caps.caps[SPAPR_CAP_HTM] = SPAPR_CAP_OFF;
     }
 
     if (!ppc_check_compat(cpu, CPU_POWERPC_LOGICAL_2_06,
                           0, spapr->max_compat_pvr)) {
-        caps.mask &= ~SPAPR_CAP_VSX;
-        caps.mask &= ~SPAPR_CAP_DFP;
+        caps.caps[SPAPR_CAP_VSX] = SPAPR_CAP_OFF;
+        caps.caps[SPAPR_CAP_DFP] = SPAPR_CAP_OFF;
     }
 
     return caps;
 }
 
-static bool spapr_caps_needed(void *opaque)
-{
-    sPAPRMachineState *spapr = opaque;
-
-    return (spapr->forced_caps.mask != 0) || (spapr->forbidden_caps.mask != 0);
-}
-
 /* This has to be called from the top-level spapr post_load, not the
  * caps specific one.  Otherwise it wouldn't be called when the source
  * caps are all defaults, which could still conflict with overridden
  * caps on the destination */
 int spapr_caps_post_migration(sPAPRMachineState *spapr)
 {
-    uint64_t allcaps = 0;
     int i;
     bool ok = true;
     sPAPRCapabilities dstcaps = spapr->effective_caps;
     sPAPRCapabilities srccaps;
 
     srccaps = default_caps_with_cpu(spapr, first_cpu);
-    srccaps.mask |= spapr->mig_forced_caps.mask;
-    srccaps.mask &= ~spapr->mig_forbidden_caps.mask;
+    for (i = 0; i < SPAPR_CAP_NUM; i++) {
+        if (spapr->mig_caps.caps[i] & SPAPR_CAP_CMD_LINE) {
+            srccaps.caps[i] = spapr->mig_caps.caps[i] & ~SPAPR_CAP_CMD_LINE;
+        }
+    }
 
-    for (i = 0; i < ARRAY_SIZE(capability_table); i++) {
+    for (i = 0; i < SPAPR_CAP_NUM; i++) {
         sPAPRCapabilityInfo *info = &capability_table[i];
 
-        allcaps |= info->flag;
-
-        if ((srccaps.mask & info->flag) && !(dstcaps.mask & info->flag)) {
-            error_report("cap-%s=on in incoming stream, but off in destination",
-                         info->name);
+        if (srccaps.caps[i] > dstcaps.caps[i]) {
+            error_report("cap-%s higher level (%d) in incoming stream than on destination (%d)",
+                         info->name, srccaps.caps[i], dstcaps.caps[i]);
             ok = false;
         }
 
-        if (!(srccaps.mask & info->flag) && (dstcaps.mask & info->flag)) {
-            warn_report("cap-%s=off in incoming stream, but on in destination",
-                         info->name);
+        if (srccaps.caps[i] < dstcaps.caps[i]) {
+            warn_report("cap-%s lower level (%d) in incoming stream than on destination (%d)",
+                         info->name, srccaps.caps[i], dstcaps.caps[i]);
         }
     }
 
-    if (spapr->mig_forced_caps.mask & ~allcaps) {
-        error_report(
-            "Unknown capabilities 0x%"PRIx64" enabled in incoming stream",
-            spapr->mig_forced_caps.mask & ~allcaps);
-        ok = false;
-    }
-    if (spapr->mig_forbidden_caps.mask & ~allcaps) {
-        warn_report(
-            "Unknown capabilities 0x%"PRIx64" disabled in incoming stream",
-            spapr->mig_forbidden_caps.mask & ~allcaps);
-    }
-
     return ok ? 0 : -EINVAL;
 }
 
-static int spapr_caps_pre_save(void *opaque)
+static bool spapr_cap_htm_needed(void *opaque)
 {
     sPAPRMachineState *spapr = opaque;
 
-    spapr->mig_forced_caps = spapr->forced_caps;
-    spapr->mig_forbidden_caps = spapr->forbidden_caps;
+    return !!(spapr->cmd_line_caps.caps[SPAPR_CAP_HTM] & SPAPR_CAP_CMD_LINE);
+}
+
+static int spapr_cap_htm_pre_save(void *opaque)
+{
+    sPAPRMachineState *spapr = opaque;
+
+    spapr->mig_caps.caps[SPAPR_CAP_HTM] =
+        spapr->cmd_line_caps.caps[SPAPR_CAP_HTM];
     return 0;
 }
 
-static int spapr_caps_pre_load(void *opaque)
+static int spapr_cap_htm_pre_load(void *opaque)
 {
     sPAPRMachineState *spapr = opaque;
 
-    spapr->mig_forced_caps = spapr_caps(0);
-    spapr->mig_forbidden_caps = spapr_caps(0);
+    spapr->mig_caps.caps[SPAPR_CAP_HTM] = 0;
     return 0;
 }
 
-const VMStateDescription vmstate_spapr_caps = {
-    .name = "spapr/caps",
+const VMStateDescription vmstate_spapr_cap_htm = {
+    .name = "spapr/cap_htm",
     .version_id = 1,
     .minimum_version_id = 1,
-    .needed = spapr_caps_needed,
-    .pre_save = spapr_caps_pre_save,
-    .pre_load = spapr_caps_pre_load,
+    .needed = spapr_cap_htm_needed,
+    .pre_save = spapr_cap_htm_pre_save,
+    .pre_load = spapr_cap_htm_pre_load,
     .fields = (VMStateField[]) {
-        VMSTATE_UINT64(mig_forced_caps.mask, sPAPRMachineState),
-        VMSTATE_UINT64(mig_forbidden_caps.mask, sPAPRMachineState),
+        VMSTATE_UINT8(mig_caps.caps[SPAPR_CAP_HTM], sPAPRMachineState),
         VMSTATE_END_OF_LIST()
     },
 };
 
-void spapr_caps_reset(sPAPRMachineState *spapr)
+static bool spapr_cap_vsx_needed(void *opaque)
 {
-    Error *local_err = NULL;
-    sPAPRCapabilities caps;
-    int i;
+    sPAPRMachineState *spapr = opaque;
 
-    /* First compute the actual set of caps we're running with.. */
-    caps = default_caps_with_cpu(spapr, first_cpu);
+    return !!(spapr->cmd_line_caps.caps[SPAPR_CAP_VSX] & SPAPR_CAP_CMD_LINE);
+}
 
-    /* Remove unnecessary forced/forbidden bits (this will help us
-     * with migration) */
-    spapr->forced_caps.mask &= ~caps.mask;
-    spapr->forbidden_caps.mask &= caps.mask;
+static int spapr_cap_vsx_pre_save(void *opaque)
+{
+    sPAPRMachineState *spapr = opaque;
+
+    spapr->mig_caps.caps[SPAPR_CAP_VSX] =
+        spapr->cmd_line_caps.caps[SPAPR_CAP_VSX];
+    return 0;
+}
 
-    caps.mask |= spapr->forced_caps.mask;
-    caps.mask &= ~spapr->forbidden_caps.mask;
+static int spapr_cap_vsx_pre_load(void *opaque)
+{
+    sPAPRMachineState *spapr = opaque;
 
-    spapr->effective_caps = caps;
+    spapr->mig_caps.caps[SPAPR_CAP_VSX] = 0;
+    return 0;
+}
 
-    /* .. then apply those caps to the virtual hardware */
+const VMStateDescription vmstate_spapr_cap_vsx = {
+    .name = "spapr/cap_vsx",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = spapr_cap_vsx_needed,
+    .pre_save = spapr_cap_vsx_pre_save,
+    .pre_load = spapr_cap_vsx_pre_load,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT8(mig_caps.caps[SPAPR_CAP_VSX], sPAPRMachineState),
+        VMSTATE_END_OF_LIST()
+    },
+};
 
-    for (i = 0; i < ARRAY_SIZE(capability_table); i++) {
-        sPAPRCapabilityInfo *info = &capability_table[i];
+static bool spapr_cap_dfp_needed(void *opaque)
+{
+    sPAPRMachineState *spapr = opaque;
 
-        if (spapr->effective_caps.mask & info->flag) {
-            /* Failure to allow a cap is fatal - if the guest doesn't
-             * have it, we'll be supplying an incorrect environment */
-            if (info->allow) {
-                info->allow(spapr, &error_fatal);
-            }
-        } else {
-            /* Failure to enforce a cap is only a warning.  The guest
-             * shouldn't be using it, since it's not advertised, so it
-             * doesn't get to complain about weird behaviour if it
-             * goes ahead anyway */
-            if (info->disallow) {
-                info->disallow(spapr, &local_err);
-            }
-            if (local_err) {
-                warn_report_err(local_err);
-                local_err = NULL;
-            }
-        }
-    }
+    return !!(spapr->cmd_line_caps.caps[SPAPR_CAP_DFP] & SPAPR_CAP_CMD_LINE);
 }
 
-static void spapr_cap_get(Object *obj, Visitor *v, const char *name,
-                          void *opaque, Error **errp)
+static int spapr_cap_dfp_pre_save(void *opaque)
 {
-    sPAPRCapabilityInfo *cap = opaque;
-    sPAPRMachineState *spapr = SPAPR_MACHINE(obj);
-    bool value = spapr_has_cap(spapr, cap->flag);
-
-    /* TODO: Could this get called before effective_caps is finalized
-     * in spapr_caps_reset()? */
+    sPAPRMachineState *spapr = opaque;
 
-    visit_type_bool(v, name, &value, errp);
+    spapr->mig_caps.caps[SPAPR_CAP_DFP] =
+        spapr->cmd_line_caps.caps[SPAPR_CAP_DFP];
+    return 0;
 }
 
-static void spapr_cap_set(Object *obj, Visitor *v, const char *name,
-                          void *opaque, Error **errp)
+static int spapr_cap_dfp_pre_load(void *opaque)
 {
-    sPAPRCapabilityInfo *cap = opaque;
-    sPAPRMachineState *spapr = SPAPR_MACHINE(obj);
-    bool value;
-    Error *local_err = NULL;
-
-    visit_type_bool(v, name, &value, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
-        return;
-    }
+    sPAPRMachineState *spapr = opaque;
 
-    if (value) {
-        spapr->forced_caps.mask |= cap->flag;
-    } else {
-        spapr->forbidden_caps.mask |= cap->flag;
-    }
+    spapr->mig_caps.caps[SPAPR_CAP_DFP] = 0;
+    return 0;
 }
 
-void spapr_caps_validate(sPAPRMachineState *spapr, Error **errp)
+const VMStateDescription vmstate_spapr_cap_dfp = {
+    .name = "spapr/cap_dfp",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = spapr_cap_dfp_needed,
+    .pre_save = spapr_cap_dfp_pre_save,
+    .pre_load = spapr_cap_dfp_pre_load,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT8(mig_caps.caps[SPAPR_CAP_DFP], sPAPRMachineState),
+        VMSTATE_END_OF_LIST()
+    },
+};
+
+void spapr_caps_reset(sPAPRMachineState *spapr)
 {
-    uint64_t allcaps = 0;
+    sPAPRCapabilities caps;
     int i;
 
-    for (i = 0; i < ARRAY_SIZE(capability_table); i++) {
-        g_assert((allcaps & capability_table[i].flag) == 0);
-        allcaps |= capability_table[i].flag;
+    /* First compute the actual set of caps we're running with.. */
+    caps = default_caps_with_cpu(spapr, first_cpu);
+
+    for (i = 0; i < SPAPR_CAP_NUM; i++) {
+        /* Check if set from command line and override default if so */
+        if (spapr->cmd_line_caps.caps[i] & SPAPR_CAP_CMD_LINE) {
+            caps.caps[i] = spapr->cmd_line_caps.caps[i] & ~SPAPR_CAP_CMD_LINE;
+        }
     }
 
-    g_assert((spapr->forced_caps.mask & ~allcaps) == 0);
-    g_assert((spapr->forbidden_caps.mask & ~allcaps) == 0);
+    spapr->effective_caps = caps;
 
-    if (spapr->forced_caps.mask & spapr->forbidden_caps.mask) {
-        error_setg(errp, "Some sPAPR capabilities set both on and off");
-        return;
+    /* .. then apply those caps to the virtual hardware */
+
+    for (i = 0; i < SPAPR_CAP_NUM; i++) {
+        sPAPRCapabilityInfo *info = &capability_table[i];
+
+        /*
+         * If the allow function can't set the desired level and think's it's
+         * fatal, it should cause that.
+         */
+        info->allow(spapr, spapr->effective_caps.caps[i], &error_fatal);
     }
 }
 
@@ -322,17 +365,19 @@ void spapr_caps_add_properties(sPAPRMachineClass *smc, Error **errp)
     for (i = 0; i < ARRAY_SIZE(capability_table); i++) {
         sPAPRCapabilityInfo *cap = &capability_table[i];
         const char *name = g_strdup_printf("cap-%s", cap->name);
+        char *desc;
 
-        object_class_property_add(klass, name, "bool",
-                                  spapr_cap_get, spapr_cap_set, NULL,
-                                  cap, &local_err);
+        object_class_property_add(klass, name, cap->type,
+                                  cap->get, cap->set,
+                                  NULL, cap, &local_err);
         if (local_err) {
             error_propagate(errp, local_err);
             return;
         }
 
-        object_class_property_set_description(klass, name, cap->description,
-                                              &local_err);
+        desc = g_strdup_printf("%s%s", cap->description, cap->options);
+        object_class_property_set_description(klass, name, desc, &local_err);
+        g_free(desc);
         if (local_err) {
             error_propagate(errp, local_err);
             return;
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 26ac17e641..2804fbbf12 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -57,17 +57,26 @@ typedef enum {
 /* These bits go in the migration stream, so they can't be reassigned */
 
 /* Hardware Transactional Memory */
-#define SPAPR_CAP_HTM               0x0000000000000001ULL
-
+#define SPAPR_CAP_HTM                   0x00
 /* Vector Scalar Extensions */
-#define SPAPR_CAP_VSX               0x0000000000000002ULL
-
+#define SPAPR_CAP_VSX                   0x01
 /* Decimal Floating Point */
-#define SPAPR_CAP_DFP               0x0000000000000004ULL
+#define SPAPR_CAP_DFP                   0x02
+/* Num Caps */
+#define SPAPR_CAP_NUM                   (SPAPR_CAP_DFP + 1)
+
+/*
+ * Capability Values
+ * NOTE: All execpt the immediately following MUST be less than 128 (0x80)
+ */
+#define SPAPR_CAP_CMD_LINE              0x80
+/* Bool Caps */
+#define SPAPR_CAP_OFF                   0x00
+#define SPAPR_CAP_ON                    0x01
 
 typedef struct sPAPRCapabilities sPAPRCapabilities;
 struct sPAPRCapabilities {
-    uint64_t mask;
+    uint8_t caps[SPAPR_CAP_NUM];
 };
 
 /**
@@ -149,9 +158,8 @@ struct sPAPRMachineState {
 
     const char *icp_type;
 
-    sPAPRCapabilities forced_caps, forbidden_caps;
-    sPAPRCapabilities mig_forced_caps, mig_forbidden_caps;
-    sPAPRCapabilities effective_caps;
+    sPAPRCapabilities cmd_line_caps, effective_caps;
+    sPAPRCapabilities mig_caps;
 };
 
 #define H_SUCCESS         0
@@ -752,21 +760,16 @@ qemu_irq spapr_qirq(sPAPRMachineState *spapr, int irq);
 /*
  * Handling of optional capabilities
  */
-extern const VMStateDescription vmstate_spapr_caps;
-
-static inline sPAPRCapabilities spapr_caps(uint64_t mask)
-{
-    sPAPRCapabilities caps = { mask };
-    return caps;
-}
+extern const VMStateDescription vmstate_spapr_cap_htm;
+extern const VMStateDescription vmstate_spapr_cap_vsx;
+extern const VMStateDescription vmstate_spapr_cap_dfp;
 
-static inline bool spapr_has_cap(sPAPRMachineState *spapr, uint64_t cap)
+static inline uint8_t spapr_get_cap(sPAPRMachineState *spapr, int cap)
 {
-    return !!(spapr->effective_caps.mask & cap);
+    return spapr->effective_caps.caps[cap];
 }
 
 void spapr_caps_reset(sPAPRMachineState *spapr);
-void spapr_caps_validate(sPAPRMachineState *spapr, Error **errp);
 void spapr_caps_add_properties(sPAPRMachineClass *smc, Error **errp);
 int spapr_caps_post_migration(sPAPRMachineState *spapr);
 
-- 
2.13.6

Re: [Qemu-devel] [Qemu-ppc] [QEMU-PPC] [RFC 1/3] hw/ppc/spapr_caps: Rework spapr_caps to use uint8 internal representation

[Murilo Opsfelder Araújo]

On 01/09/2018 07:21 AM, Suraj Jitindar Singh wrote:
> Currently spapr_caps are tied to boolean values (on or off). This patch
> reworks the caps so that they can have any value between 0 and 127,
> inclusive. This allows more capabilities with various values to be
> represented in the same way internally. Capabilities are numbered in
> ascending order. The internal representation of capability values is an
> array of uint8s in the sPAPRMachineState, indexed by capability number.
> Note: The MSB (0x80) of a capability is reserved to track whether the
>       capability was set from the command line.
> 
> Capabilities can have their own name, description, options, getter and
> setter functions, type and allow functions. They also each have their own
> section in the migration stream. Capabilities are only migrated if they
> were explictly set on the command line, with the assumption that
> otherwise the default will match.
> 
> On migration we ensure that the capability value on the destination
> is greater than or equal to the capability value from the source. So
> long at this remains the case then the migration is considered
> compatible and allowed to continue.
> 
> This patch implements generic getter and setter functions for boolean
> capabilities. It also converts the existings cap-htm, cap-vsx and
> cap-dfp capabilities to this new format.

Hi, Suraj.

I've got the impression that this patch does a lot of things. What about
splitting this patch into the following?

- rename spapr_has_cap() -> spapr_get_cap()
- introduce each spapr_cap_[gs]et_bool() in separate patches
- make use of spapr_cap[gs]et_bool()
- convert capabilities internal representation to uint8
- add each new capability separately

Perhaps it can be broken into even smaller changes.

Cheers
Murilo

Re: [Qemu-devel] [Qemu-ppc] [QEMU-PPC] [RFC 1/3] hw/ppc/spapr_caps: Rework spapr_caps to use uint8 internal representation

[Suraj Jitindar Singh]

On Tue, 2018-01-09 at 09:13 -0200, Murilo Opsfelder Araújo wrote:
> On 01/09/2018 07:21 AM, Suraj Jitindar Singh wrote:
> > Currently spapr_caps are tied to boolean values (on or off). This
> > patch
> > reworks the caps so that they can have any value between 0 and 127,
> > inclusive. This allows more capabilities with various values to be
> > represented in the same way internally. Capabilities are numbered
> > in
> > ascending order. The internal representation of capability values
> > is an
> > array of uint8s in the sPAPRMachineState, indexed by capability
> > number.
> > Note: The MSB (0x80) of a capability is reserved to track whether
> > the
> >       capability was set from the command line.
> > 
> > Capabilities can have their own name, description, options, getter
> > and
> > setter functions, type and allow functions. They also each have
> > their own
> > section in the migration stream. Capabilities are only migrated if
> > they
> > were explictly set on the command line, with the assumption that
> > otherwise the default will match.
> > 
> > On migration we ensure that the capability value on the destination
> > is greater than or equal to the capability value from the source.
> > So
> > long at this remains the case then the migration is considered
> > compatible and allowed to continue.
> > 
> > This patch implements generic getter and setter functions for
> > boolean
> > capabilities. It also converts the existings cap-htm, cap-vsx and
> > cap-dfp capabilities to this new format.
> 
> Hi, Suraj.
> 
> I've got the impression that this patch does a lot of things. What
> about
> splitting this patch into the following?
> 
> - rename spapr_has_cap() -> spapr_get_cap()
> - introduce each spapr_cap_[gs]et_bool() in separate patches
> - make use of spapr_cap[gs]et_bool()
> - convert capabilities internal representation to uint8
> - add each new capability separately

Absolutely. This was an RFC to get the code out there for comment.
I'll try to split it up further as suggested for the actual patch
series :)

> 
> Perhaps it can be broken into even smaller changes.
> 
> Cheers
> Murilo
> 

Re: [Qemu-devel] [Qemu-ppc] [QEMU-PPC] [RFC 1/3] hw/ppc/spapr_caps: Rework spapr_caps to use uint8 internal representation

[Andrea Bolognani]

On Tue, 2018-01-09 at 20:21 +1100, Suraj Jitindar Singh wrote:
[...]
> +static void cap_htm_allow(sPAPRMachineState *spapr, uint8_t val, Error **errp)
> +{
> +    if (!val) {
> +        /* TODO: We don't support disabling htm yet */
> +        return;
> +    }
>      if (tcg_enabled()) {
>          error_setg(errp,
> -                   "No Transactional Memory support in TCG, try cap-htm=off");
> +                   "No Transactional Memory support in TCG, try cap-htm=0");
>      } else if (kvm_enabled() && !kvmppc_has_cap_htm()) {
>          error_setg(errp,
> -"KVM implementation does not support Transactional Memory, try cap-htm=off"
> +"KVM implementation does not support Transactional Memory, try cap-htm=0"
>              );
>      }
>  }

Changing the command-line interface from off/on to 0/1 seems
unnecessary, given that broken/workaround/fixed are used for the
capabilities you introduce later in the series. off/on look much
better IMHO.

[...]
> -static bool spapr_caps_needed(void *opaque)
> -{
> -    sPAPRMachineState *spapr = opaque;
> -
> -    return (spapr->forced_caps.mask != 0) || (spapr->forbidden_caps.mask != 0);
> -}
> -
>  /* This has to be called from the top-level spapr post_load, not the
>   * caps specific one.  Otherwise it wouldn't be called when the source
>   * caps are all defaults, which could still conflict with overridden
>   * caps on the destination */
>  int spapr_caps_post_migration(sPAPRMachineState *spapr)
>  {
> -    uint64_t allcaps = 0;
>      int i;
>      bool ok = true;
>      sPAPRCapabilities dstcaps = spapr->effective_caps;
>      sPAPRCapabilities srccaps;
>  
>      srccaps = default_caps_with_cpu(spapr, first_cpu);
> -    srccaps.mask |= spapr->mig_forced_caps.mask;
> -    srccaps.mask &= ~spapr->mig_forbidden_caps.mask;
> +    for (i = 0; i < SPAPR_CAP_NUM; i++) {
> +        if (spapr->mig_caps.caps[i] & SPAPR_CAP_CMD_LINE) {
> +            srccaps.caps[i] = spapr->mig_caps.caps[i] & ~SPAPR_CAP_CMD_LINE;
> +        }
> +    }
>  
> -    for (i = 0; i < ARRAY_SIZE(capability_table); i++) {
> +    for (i = 0; i < SPAPR_CAP_NUM; i++) {
>          sPAPRCapabilityInfo *info = &capability_table[i];
>  
> -        allcaps |= info->flag;
> -
> -        if ((srccaps.mask & info->flag) && !(dstcaps.mask & info->flag)) {
> -            error_report("cap-%s=on in incoming stream, but off in destination",
> -                         info->name);
> +        if (srccaps.caps[i] > dstcaps.caps[i]) {
> +            error_report("cap-%s higher level (%d) in incoming stream than on destination (%d)",
> +                         info->name, srccaps.caps[i], dstcaps.caps[i]);
>              ok = false;
>          }
>  
> -        if (!(srccaps.mask & info->flag) && (dstcaps.mask & info->flag)) {
> -            warn_report("cap-%s=off in incoming stream, but on in destination",
> -                         info->name);
> +        if (srccaps.caps[i] < dstcaps.caps[i]) {
> +            warn_report("cap-%s lower level (%d) in incoming stream than on destination (%d)",
> +                         info->name, srccaps.caps[i], dstcaps.caps[i]);
>          }
>      }

These numeric comparisons make me feel very uneasy :)

What if we need to add more possible values down the line? Should
there be at least some room between existing values to avoid painting
ourselves in a corner? Eg. instead of using 0/1/2 use 20/40/60...

You clearly know more about the problem than I do, so feel free to
dismiss all of the above... I thought I would bring up my worries
just in case :)

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [Qemu-devel] [Qemu-ppc] [QEMU-PPC] [RFC 1/3] hw/ppc/spapr_caps: Rework spapr_caps to use uint8 internal representation

[Suraj Jitindar Singh]

On Tue, 2018-01-09 at 13:07 +0100, Andrea Bolognani wrote:
> On Tue, 2018-01-09 at 20:21 +1100, Suraj Jitindar Singh wrote:
> [...]
> > +static void cap_htm_allow(sPAPRMachineState *spapr, uint8_t val,
> > Error **errp)
> > +{
> > +    if (!val) {
> > +        /* TODO: We don't support disabling htm yet */
> > +        return;
> > +    }
> >      if (tcg_enabled()) {
> >          error_setg(errp,
> > -                   "No Transactional Memory support in TCG, try
> > cap-htm=off");
> > +                   "No Transactional Memory support in TCG, try
> > cap-htm=0");
> >      } else if (kvm_enabled() && !kvmppc_has_cap_htm()) {
> >          error_setg(errp,
> > -"KVM implementation does not support Transactional Memory, try
> > cap-htm=off"
> > +"KVM implementation does not support Transactional Memory, try
> > cap-htm=0"
> >              );
> >      }
> >  }
> 
> Changing the command-line interface from off/on to 0/1 seems
> unnecessary, given that broken/workaround/fixed are used for the
> capabilities you introduce later in the series. off/on look much
> better IMHO.

These are booleans so they have to be "on"/"off" anyway... 0/1 doesn't
work. My bad :/ I'll fix the message.

> 
> [...]
> > -static bool spapr_caps_needed(void *opaque)
> > -{
> > -    sPAPRMachineState *spapr = opaque;
> > -
> > -    return (spapr->forced_caps.mask != 0) || (spapr-
> > >forbidden_caps.mask != 0);
> > -}
> > -
> >  /* This has to be called from the top-level spapr post_load, not
> > the
> >   * caps specific one.  Otherwise it wouldn't be called when the
> > source
> >   * caps are all defaults, which could still conflict with
> > overridden
> >   * caps on the destination */
> >  int spapr_caps_post_migration(sPAPRMachineState *spapr)
> >  {
> > -    uint64_t allcaps = 0;
> >      int i;
> >      bool ok = true;
> >      sPAPRCapabilities dstcaps = spapr->effective_caps;
> >      sPAPRCapabilities srccaps;
> >  
> >      srccaps = default_caps_with_cpu(spapr, first_cpu);
> > -    srccaps.mask |= spapr->mig_forced_caps.mask;
> > -    srccaps.mask &= ~spapr->mig_forbidden_caps.mask;
> > +    for (i = 0; i < SPAPR_CAP_NUM; i++) {
> > +        if (spapr->mig_caps.caps[i] & SPAPR_CAP_CMD_LINE) {
> > +            srccaps.caps[i] = spapr->mig_caps.caps[i] &
> > ~SPAPR_CAP_CMD_LINE;
> > +        }
> > +    }
> >  
> > -    for (i = 0; i < ARRAY_SIZE(capability_table); i++) {
> > +    for (i = 0; i < SPAPR_CAP_NUM; i++) {
> >          sPAPRCapabilityInfo *info = &capability_table[i];
> >  
> > -        allcaps |= info->flag;
> > -
> > -        if ((srccaps.mask & info->flag) && !(dstcaps.mask & info-
> > >flag)) {
> > -            error_report("cap-%s=on in incoming stream, but off in
> > destination",
> > -                         info->name);
> > +        if (srccaps.caps[i] > dstcaps.caps[i]) {
> > +            error_report("cap-%s higher level (%d) in incoming
> > stream than on destination (%d)",
> > +                         info->name, srccaps.caps[i],
> > dstcaps.caps[i]);
> >              ok = false;
> >          }
> >  
> > -        if (!(srccaps.mask & info->flag) && (dstcaps.mask & info-
> > >flag)) {
> > -            warn_report("cap-%s=off in incoming stream, but on in
> > destination",
> > -                         info->name);
> > +        if (srccaps.caps[i] < dstcaps.caps[i]) {
> > +            warn_report("cap-%s lower level (%d) in incoming
> > stream than on destination (%d)",
> > +                         info->name, srccaps.caps[i],
> > dstcaps.caps[i]);
> >          }
> >      }
> 
> These numeric comparisons make me feel very uneasy :)
> 
> What if we need to add more possible values down the line? Should
> there be at least some room between existing values to avoid painting
> ourselves in a corner? Eg. instead of using 0/1/2 use 20/40/60...
> 
> You clearly know more about the problem than I do, so feel free to
> dismiss all of the above... I thought I would bring up my worries
> just in case :)

For these capabilities I think we're ok to keep it as 0/1/2. In the
event we need a bigger range another capability can be added with other
possible values which was the whole point of introducing this generic
framework. The basic idea is the receiving side must always support a
higher "level" than the source.

With these new capabilities it's more likely we'll have to add an
entirly new one than require more possible values. :)

It could even be possible to have a per capability comparison function
to confirm compatibility in future. But again thats an exercise for
when/if more complex capabilities are added.

> 

Re: [Qemu-devel] [Qemu-ppc] [QEMU-PPC] [RFC 1/3] hw/ppc/spapr_caps: Rework spapr_caps to use uint8 internal representation

[David Gibson]

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

On Wed, Jan 10, 2018 at 11:19:33AM +1100, Suraj Jitindar Singh wrote:
> On Tue, 2018-01-09 at 13:07 +0100, Andrea Bolognani wrote:
> > On Tue, 2018-01-09 at 20:21 +1100, Suraj Jitindar Singh wrote:
> > [...]
> > > +static void cap_htm_allow(sPAPRMachineState *spapr, uint8_t val,
> > > Error **errp)
> > > +{
> > > +    if (!val) {
> > > +        /* TODO: We don't support disabling htm yet */
> > > +        return;
> > > +    }
> > >      if (tcg_enabled()) {
> > >          error_setg(errp,
> > > -                   "No Transactional Memory support in TCG, try
> > > cap-htm=off");
> > > +                   "No Transactional Memory support in TCG, try
> > > cap-htm=0");
> > >      } else if (kvm_enabled() && !kvmppc_has_cap_htm()) {
> > >          error_setg(errp,
> > > -"KVM implementation does not support Transactional Memory, try
> > > cap-htm=off"
> > > +"KVM implementation does not support Transactional Memory, try
> > > cap-htm=0"
> > >              );
> > >      }
> > >  }
> > 
> > Changing the command-line interface from off/on to 0/1 seems
> > unnecessary, given that broken/workaround/fixed are used for the
> > capabilities you introduce later in the series. off/on look much
> > better IMHO.
> 
> These are booleans so they have to be "on"/"off" anyway... 0/1 doesn't
> work. My bad :/ I'll fix the message.
> 
> > 
> > [...]
> > > -static bool spapr_caps_needed(void *opaque)
> > > -{
> > > -    sPAPRMachineState *spapr = opaque;
> > > -
> > > -    return (spapr->forced_caps.mask != 0) || (spapr-
> > > >forbidden_caps.mask != 0);
> > > -}
> > > -
> > >  /* This has to be called from the top-level spapr post_load, not
> > > the
> > >   * caps specific one.  Otherwise it wouldn't be called when the
> > > source
> > >   * caps are all defaults, which could still conflict with
> > > overridden
> > >   * caps on the destination */
> > >  int spapr_caps_post_migration(sPAPRMachineState *spapr)
> > >  {
> > > -    uint64_t allcaps = 0;
> > >      int i;
> > >      bool ok = true;
> > >      sPAPRCapabilities dstcaps = spapr->effective_caps;
> > >      sPAPRCapabilities srccaps;
> > >  
> > >      srccaps = default_caps_with_cpu(spapr, first_cpu);
> > > -    srccaps.mask |= spapr->mig_forced_caps.mask;
> > > -    srccaps.mask &= ~spapr->mig_forbidden_caps.mask;
> > > +    for (i = 0; i < SPAPR_CAP_NUM; i++) {
> > > +        if (spapr->mig_caps.caps[i] & SPAPR_CAP_CMD_LINE) {
> > > +            srccaps.caps[i] = spapr->mig_caps.caps[i] &
> > > ~SPAPR_CAP_CMD_LINE;
> > > +        }
> > > +    }
> > >  
> > > -    for (i = 0; i < ARRAY_SIZE(capability_table); i++) {
> > > +    for (i = 0; i < SPAPR_CAP_NUM; i++) {
> > >          sPAPRCapabilityInfo *info = &capability_table[i];
> > >  
> > > -        allcaps |= info->flag;
> > > -
> > > -        if ((srccaps.mask & info->flag) && !(dstcaps.mask & info-
> > > >flag)) {
> > > -            error_report("cap-%s=on in incoming stream, but off in
> > > destination",
> > > -                         info->name);
> > > +        if (srccaps.caps[i] > dstcaps.caps[i]) {
> > > +            error_report("cap-%s higher level (%d) in incoming
> > > stream than on destination (%d)",
> > > +                         info->name, srccaps.caps[i],
> > > dstcaps.caps[i]);
> > >              ok = false;
> > >          }
> > >  
> > > -        if (!(srccaps.mask & info->flag) && (dstcaps.mask & info-
> > > >flag)) {
> > > -            warn_report("cap-%s=off in incoming stream, but on in
> > > destination",
> > > -                         info->name);
> > > +        if (srccaps.caps[i] < dstcaps.caps[i]) {
> > > +            warn_report("cap-%s lower level (%d) in incoming
> > > stream than on destination (%d)",
> > > +                         info->name, srccaps.caps[i],
> > > dstcaps.caps[i]);
> > >          }
> > >      }
> > 
> > These numeric comparisons make me feel very uneasy :)
> > 
> > What if we need to add more possible values down the line? Should
> > there be at least some room between existing values to avoid painting
> > ourselves in a corner? Eg. instead of using 0/1/2 use 20/40/60...
> > 
> > You clearly know more about the problem than I do, so feel free to
> > dismiss all of the above... I thought I would bring up my worries
> > just in case :)
> 
> For these capabilities I think we're ok to keep it as 0/1/2. In the
> event we need a bigger range another capability can be added with other
> possible values which was the whole point of introducing this generic
> framework. The basic idea is the receiving side must always support a
> higher "level" than the source.
> 
> With these new capabilities it's more likely we'll have to add an
> entirly new one than require more possible values. :)
> 
> It could even be possible to have a per capability comparison function
> to confirm compatibility in future. But again thats an exercise for
> when/if more complex capabilities are added.

I concur.  New capabilities which require a more detailed set of
values are reasonably likely.  New values for existing capabilities
are not - to make sense of the capabilities you really need to
understand the set of all possible values when they're defined.  So I
think 0..2 is ok.

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

Re: [Qemu-devel] [QEMU-PPC] [RFC 1/3] hw/ppc/spapr_caps: Rework spapr_caps to use uint8 internal representation

[David Gibson]

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

On Tue, Jan 09, 2018 at 08:21:01PM +1100, Suraj Jitindar Singh wrote:
> Currently spapr_caps are tied to boolean values (on or off). This patch
> reworks the caps so that they can have any value between 0 and 127,
> inclusive. This allows more capabilities with various values to be
> represented in the same way internally. Capabilities are numbered in
> ascending order. The internal representation of capability values is an
> array of uint8s in the sPAPRMachineState, indexed by capability number.
> Note: The MSB (0x80) of a capability is reserved to track whether the
>       capability was set from the command line.
> 
> Capabilities can have their own name, description, options, getter and
> setter functions, type and allow functions. They also each have their own
> section in the migration stream. Capabilities are only migrated if they
> were explictly set on the command line, with the assumption that
> otherwise the default will match.
> 
> On migration we ensure that the capability value on the destination
> is greater than or equal to the capability value from the source. So
> long at this remains the case then the migration is considered
> compatible and allowed to continue.
> 
> This patch implements generic getter and setter functions for boolean
> capabilities. It also converts the existings cap-htm, cap-vsx and
> cap-dfp capabilities to this new format.
> ---
>  hw/ppc/spapr.c         |  19 +--
>  hw/ppc/spapr_caps.c    | 335 ++++++++++++++++++++++++++++---------------------
>  include/hw/ppc/spapr.h |  41 +++---
>  3 files changed, 222 insertions(+), 173 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index d1acfe8858..7fa45729ba 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -320,7 +320,7 @@ static void spapr_populate_pa_features(sPAPRMachineState *spapr,
>           */
>          pa_features[3] |= 0x20;
>      }
> -    if (spapr_has_cap(spapr, SPAPR_CAP_HTM) && pa_size > 24) {
> +    if (spapr_get_cap(spapr, SPAPR_CAP_HTM) && pa_size > 24) {

I'd prefer to see explicit >0 or !=0 to emphasise that the returned
value is now an integer not a bool.

>          pa_features[24] |= 0x80;    /* Transactional memory support */
>      }
>      if (legacy_guest && pa_size > 40) {
> @@ -563,7 +563,7 @@ static void spapr_populate_cpu_dt(CPUState *cs, void *fdt, int offset,
>       *
>       * Only CPUs for which we create core types in spapr_cpu_core.c
>       * are possible, and all of those have VMX */
> -    if (spapr_has_cap(spapr, SPAPR_CAP_VSX)) {
> +    if (spapr_get_cap(spapr, SPAPR_CAP_VSX)) {
>          _FDT((fdt_setprop_cell(fdt, offset, "ibm,vmx", 2)));
>      } else {
>          _FDT((fdt_setprop_cell(fdt, offset, "ibm,vmx", 1)));
> @@ -572,7 +572,7 @@ static void spapr_populate_cpu_dt(CPUState *cs, void *fdt, int offset,
>      /* Advertise DFP (Decimal Floating Point) if available
>       *   0 / no property == no DFP
>       *   1               == DFP available */
> -    if (spapr_has_cap(spapr, SPAPR_CAP_DFP)) {
> +    if (spapr_get_cap(spapr, SPAPR_CAP_DFP)) {
>          _FDT((fdt_setprop_cell(fdt, offset, "ibm,dfp", 1)));
>      }
>  
> @@ -1762,7 +1762,9 @@ static const VMStateDescription vmstate_spapr = {
>          &vmstate_spapr_ov5_cas,
>          &vmstate_spapr_patb_entry,
>          &vmstate_spapr_pending_events,
> -        &vmstate_spapr_caps,
> +        &vmstate_spapr_cap_htm,
> +        &vmstate_spapr_cap_vsx,
> +        &vmstate_spapr_cap_dfp,
>          NULL
>      }
>  };
> @@ -2323,8 +2325,6 @@ static void spapr_machine_init(MachineState *machine)
>      char *filename;
>      Error *resize_hpt_err = NULL;
>  
> -    spapr_caps_validate(spapr, &error_fatal);
> -
>      msi_nonbroken = true;
>  
>      QLIST_INIT(&spapr->phbs);
> @@ -3834,7 +3834,9 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
>       */
>      mc->numa_mem_align_shift = 28;
>  
> -    smc->default_caps = spapr_caps(SPAPR_CAP_VSX | SPAPR_CAP_DFP);
> +    smc->default_caps.caps[SPAPR_CAP_HTM] = SPAPR_CAP_OFF;
> +    smc->default_caps.caps[SPAPR_CAP_VSX] = SPAPR_CAP_ON;
> +    smc->default_caps.caps[SPAPR_CAP_DFP] = SPAPR_CAP_ON;
>      spapr_caps_add_properties(smc, &error_abort);
>  }
>  
> @@ -3916,8 +3918,7 @@ static void spapr_machine_2_11_class_options(MachineClass *mc)
>      sPAPRMachineClass *smc = SPAPR_MACHINE_CLASS(mc);
>  
>      spapr_machine_2_12_class_options(mc);
> -    smc->default_caps = spapr_caps(SPAPR_CAP_HTM | SPAPR_CAP_VSX
> -                                   | SPAPR_CAP_DFP);
> +    smc->default_caps.caps[SPAPR_CAP_HTM] = SPAPR_CAP_ON;
>      SET_MACHINE_COMPAT(mc, SPAPR_COMPAT_2_11);
>  }
>  
> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
> index 9d070a306c..af40f2e469 100644
> --- a/hw/ppc/spapr_caps.c
> +++ b/hw/ppc/spapr_caps.c
> @@ -35,33 +35,71 @@
>  typedef struct sPAPRCapabilityInfo {
>      const char *name;
>      const char *description;
> -    uint64_t flag;
> +    const char *options;
> +    int index;
>  
> +    /* Getter and Setter Function Pointers */
> +    ObjectPropertyAccessor *get;
> +    ObjectPropertyAccessor *set;
> +    const char *type;
>      /* Make sure the virtual hardware can support this capability */
> -    void (*allow)(sPAPRMachineState *spapr, Error **errp);
> -
> -    /* If possible, tell the virtual hardware not to allow the cap to
> -     * be used at all */
> -    void (*disallow)(sPAPRMachineState *spapr, Error **errp);
> +    void (*allow)(sPAPRMachineState *spapr, uint8_t val, Error **errp);

I think we need a new name for this since it can both allow and
disallow.  Maybe 'apply'?

>  } sPAPRCapabilityInfo;
>  
> -static void cap_htm_allow(sPAPRMachineState *spapr, Error **errp)
> +static void spapr_cap_get_bool(Object *obj, Visitor *v, const char *name,
> +                               void *opaque, Error **errp)
> +{
> +    sPAPRCapabilityInfo *cap = opaque;
> +    sPAPRMachineState *spapr = SPAPR_MACHINE(obj);
> +    bool value = spapr_get_cap(spapr, cap->index) == SPAPR_CAP_ON;
> +
> +    visit_type_bool(v, name, &value, errp);
> +}
> +
> +static void spapr_cap_set_bool(Object *obj, Visitor *v, const char *name,
> +                               void *opaque, Error **errp)
>  {
> +    sPAPRCapabilityInfo *cap = opaque;
> +    sPAPRMachineState *spapr = SPAPR_MACHINE(obj);
> +    bool value;
> +    Error *local_err = NULL;
> +
> +    visit_type_bool(v, name, &value, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +
> +    spapr->cmd_line_caps.caps[cap->index] = (value ? SPAPR_CAP_ON :
> +                                                     SPAPR_CAP_OFF) |
> +                                             SPAPR_CAP_CMD_LINE;
> +}
> +
> +static void cap_htm_allow(sPAPRMachineState *spapr, uint8_t val, Error **errp)
> +{
> +    if (!val) {
> +        /* TODO: We don't support disabling htm yet */
> +        return;
> +    }

A downside of fusing allow and disallow is that we can't now apply the
rule that failure to allow is an error but failure to disallow is only
a warning in the common code.  Oh well.

>      if (tcg_enabled()) {
>          error_setg(errp,
> -                   "No Transactional Memory support in TCG, try cap-htm=off");
> +                   "No Transactional Memory support in TCG, try cap-htm=0");
>      } else if (kvm_enabled() && !kvmppc_has_cap_htm()) {
>          error_setg(errp,
> -"KVM implementation does not support Transactional Memory, try cap-htm=off"
> +"KVM implementation does not support Transactional Memory, try cap-htm=0"
>              );
>      }
>  }
>  
> -static void cap_vsx_allow(sPAPRMachineState *spapr, Error **errp)
> +static void cap_vsx_allow(sPAPRMachineState *spapr, uint8_t val, Error **errp)
>  {
>      PowerPCCPU *cpu = POWERPC_CPU(first_cpu);
>      CPUPPCState *env = &cpu->env;
>  
> +    if (!val) {
> +        /* TODO: We don't support disabling vsx yet */
> +        return;
> +    }
>      /* Allowable CPUs in spapr_cpu_core.c should already have gotten
>       * rid of anything that doesn't do VMX */
>      g_assert(env->insns_flags & PPC_ALTIVEC);
> @@ -70,37 +108,51 @@ static void cap_vsx_allow(sPAPRMachineState *spapr, Error **errp)
>      }
>  }
>  
> -static void cap_dfp_allow(sPAPRMachineState *spapr, Error **errp)
> +static void cap_dfp_allow(sPAPRMachineState *spapr, uint8_t val, Error **errp)
>  {
>      PowerPCCPU *cpu = POWERPC_CPU(first_cpu);
>      CPUPPCState *env = &cpu->env;
>  
> +    if (!val) {
> +        /* TODO: We don't support disabling dfp yet */
> +        return;
> +    }
>      if (!(env->insns_flags2 & PPC2_DFP)) {
>          error_setg(errp, "DFP support not available, try cap-dfp=off");
>      }
>  }
>  
> -static sPAPRCapabilityInfo capability_table[] = {
> -    {
> +
> +sPAPRCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
> +    [SPAPR_CAP_HTM] = {
>          .name = "htm",
>          .description = "Allow Hardware Transactional Memory (HTM)",
> -        .flag = SPAPR_CAP_HTM,
> +        .options = "",

It's not really clear to me what the options field is for.

> +        .index = SPAPR_CAP_HTM,
> +        .get = spapr_cap_get_bool,
> +        .set = spapr_cap_set_bool,
> +        .type = "bool",
>          .allow = cap_htm_allow,
> -        /* TODO: add cap_htm_disallow */
>      },
> -    {
> +    [SPAPR_CAP_VSX] = {
>          .name = "vsx",
>          .description = "Allow Vector Scalar Extensions (VSX)",
> -        .flag = SPAPR_CAP_VSX,
> +        .options = "",
> +        .index = SPAPR_CAP_VSX,
> +        .get = spapr_cap_get_bool,
> +        .set = spapr_cap_set_bool,
> +        .type = "bool",
>          .allow = cap_vsx_allow,
> -        /* TODO: add cap_vsx_disallow */
>      },
> -    {
> +    [SPAPR_CAP_DFP] = {
>          .name = "dfp",
>          .description = "Allow Decimal Floating Point (DFP)",
> -        .flag = SPAPR_CAP_DFP,
> +        .options = "",
> +        .index = SPAPR_CAP_DFP,
> +        .get = spapr_cap_get_bool,
> +        .set = spapr_cap_set_bool,
> +        .type = "bool",
>          .allow = cap_dfp_allow,
> -        /* TODO: add cap_dfp_disallow */
>      },
>  };
>  
> @@ -115,201 +167,192 @@ static sPAPRCapabilities default_caps_with_cpu(sPAPRMachineState *spapr,
>  
>      if (!ppc_check_compat(cpu, CPU_POWERPC_LOGICAL_2_07,
>                            0, spapr->max_compat_pvr)) {
> -        caps.mask &= ~SPAPR_CAP_HTM;
> +        caps.caps[SPAPR_CAP_HTM] = SPAPR_CAP_OFF;
>      }
>  
>      if (!ppc_check_compat(cpu, CPU_POWERPC_LOGICAL_2_06,
>                            0, spapr->max_compat_pvr)) {
> -        caps.mask &= ~SPAPR_CAP_VSX;
> -        caps.mask &= ~SPAPR_CAP_DFP;
> +        caps.caps[SPAPR_CAP_VSX] = SPAPR_CAP_OFF;
> +        caps.caps[SPAPR_CAP_DFP] = SPAPR_CAP_OFF;
>      }
>  
>      return caps;
>  }
>  
> -static bool spapr_caps_needed(void *opaque)
> -{
> -    sPAPRMachineState *spapr = opaque;
> -
> -    return (spapr->forced_caps.mask != 0) || (spapr->forbidden_caps.mask != 0);
> -}
> -
>  /* This has to be called from the top-level spapr post_load, not the
>   * caps specific one.  Otherwise it wouldn't be called when the source
>   * caps are all defaults, which could still conflict with overridden
>   * caps on the destination */
>  int spapr_caps_post_migration(sPAPRMachineState *spapr)
>  {
> -    uint64_t allcaps = 0;
>      int i;
>      bool ok = true;
>      sPAPRCapabilities dstcaps = spapr->effective_caps;
>      sPAPRCapabilities srccaps;
>  
>      srccaps = default_caps_with_cpu(spapr, first_cpu);
> -    srccaps.mask |= spapr->mig_forced_caps.mask;
> -    srccaps.mask &= ~spapr->mig_forbidden_caps.mask;
> +    for (i = 0; i < SPAPR_CAP_NUM; i++) {
> +        if (spapr->mig_caps.caps[i] & SPAPR_CAP_CMD_LINE) {
> +            srccaps.caps[i] = spapr->mig_caps.caps[i] & ~SPAPR_CAP_CMD_LINE;
> +        }
> +    }
>  
> -    for (i = 0; i < ARRAY_SIZE(capability_table); i++) {
> +    for (i = 0; i < SPAPR_CAP_NUM; i++) {
>          sPAPRCapabilityInfo *info = &capability_table[i];
>  
> -        allcaps |= info->flag;
> -
> -        if ((srccaps.mask & info->flag) && !(dstcaps.mask & info->flag)) {
> -            error_report("cap-%s=on in incoming stream, but off in destination",
> -                         info->name);
> +        if (srccaps.caps[i] > dstcaps.caps[i]) {
> +            error_report("cap-%s higher level (%d) in incoming stream than on destination (%d)",
> +                         info->name, srccaps.caps[i], dstcaps.caps[i]);
>              ok = false;
>          }
>  
> -        if (!(srccaps.mask & info->flag) && (dstcaps.mask & info->flag)) {
> -            warn_report("cap-%s=off in incoming stream, but on in destination",
> -                         info->name);
> +        if (srccaps.caps[i] < dstcaps.caps[i]) {
> +            warn_report("cap-%s lower level (%d) in incoming stream than on destination (%d)",
> +                         info->name, srccaps.caps[i], dstcaps.caps[i]);
>          }
>      }
>  
> -    if (spapr->mig_forced_caps.mask & ~allcaps) {
> -        error_report(
> -            "Unknown capabilities 0x%"PRIx64" enabled in incoming stream",
> -            spapr->mig_forced_caps.mask & ~allcaps);
> -        ok = false;
> -    }
> -    if (spapr->mig_forbidden_caps.mask & ~allcaps) {
> -        warn_report(
> -            "Unknown capabilities 0x%"PRIx64" disabled in incoming stream",
> -            spapr->mig_forbidden_caps.mask & ~allcaps);
> -    }
> -
>      return ok ? 0 : -EINVAL;
>  }
>  
> -static int spapr_caps_pre_save(void *opaque)
> +static bool spapr_cap_htm_needed(void *opaque)
>  {
>      sPAPRMachineState *spapr = opaque;
>  
> -    spapr->mig_forced_caps = spapr->forced_caps;
> -    spapr->mig_forbidden_caps = spapr->forbidden_caps;
> +    return !!(spapr->cmd_line_caps.caps[SPAPR_CAP_HTM] & SPAPR_CAP_CMD_LINE);
> +}
> +
> +static int spapr_cap_htm_pre_save(void *opaque)

Having to have a separate needed, pre_save and pre_load for each cap
will be a pain.  I hope we can find a way to do this in common.

> +{
> +    sPAPRMachineState *spapr = opaque;
> +
> +    spapr->mig_caps.caps[SPAPR_CAP_HTM] =
> +        spapr->cmd_line_caps.caps[SPAPR_CAP_HTM];
>      return 0;
>  }
>  
> -static int spapr_caps_pre_load(void *opaque)
> +static int spapr_cap_htm_pre_load(void *opaque)
>  {
>      sPAPRMachineState *spapr = opaque;
>  
> -    spapr->mig_forced_caps = spapr_caps(0);
> -    spapr->mig_forbidden_caps = spapr_caps(0);
> +    spapr->mig_caps.caps[SPAPR_CAP_HTM] = 0;
>      return 0;
>  }
>  
> -const VMStateDescription vmstate_spapr_caps = {
> -    .name = "spapr/caps",
> +const VMStateDescription vmstate_spapr_cap_htm = {
> +    .name = "spapr/cap_htm",

I'd suggest spapr/caps/htm - and a common vmstate_spapr_caps to hold
the subsections for convenience rather than putting them straight
under the master spapr one.

>      .version_id = 1,
>      .minimum_version_id = 1,
> -    .needed = spapr_caps_needed,
> -    .pre_save = spapr_caps_pre_save,
> -    .pre_load = spapr_caps_pre_load,
> +    .needed = spapr_cap_htm_needed,
> +    .pre_save = spapr_cap_htm_pre_save,
> +    .pre_load = spapr_cap_htm_pre_load,
>      .fields = (VMStateField[]) {
> -        VMSTATE_UINT64(mig_forced_caps.mask, sPAPRMachineState),
> -        VMSTATE_UINT64(mig_forbidden_caps.mask, sPAPRMachineState),
> +        VMSTATE_UINT8(mig_caps.caps[SPAPR_CAP_HTM], sPAPRMachineState),
>          VMSTATE_END_OF_LIST()
>      },
>  };
>  
> -void spapr_caps_reset(sPAPRMachineState *spapr)
> +static bool spapr_cap_vsx_needed(void *opaque)
>  {
> -    Error *local_err = NULL;
> -    sPAPRCapabilities caps;
> -    int i;
> +    sPAPRMachineState *spapr = opaque;
>  
> -    /* First compute the actual set of caps we're running with.. */
> -    caps = default_caps_with_cpu(spapr, first_cpu);
> +    return !!(spapr->cmd_line_caps.caps[SPAPR_CAP_VSX] & SPAPR_CAP_CMD_LINE);
> +}
>  
> -    /* Remove unnecessary forced/forbidden bits (this will help us
> -     * with migration) */
> -    spapr->forced_caps.mask &= ~caps.mask;
> -    spapr->forbidden_caps.mask &= caps.mask;

I don't think you have an equivalent of this in the new code, which
means that an explicitly set property could prevent migration, even if
it actually has the default value.

> +static int spapr_cap_vsx_pre_save(void *opaque)
> +{
> +    sPAPRMachineState *spapr = opaque;
> +
> +    spapr->mig_caps.caps[SPAPR_CAP_VSX] =
> +        spapr->cmd_line_caps.caps[SPAPR_CAP_VSX];
> +    return 0;
> +}
>  
> -    caps.mask |= spapr->forced_caps.mask;
> -    caps.mask &= ~spapr->forbidden_caps.mask;
> +static int spapr_cap_vsx_pre_load(void *opaque)
> +{
> +    sPAPRMachineState *spapr = opaque;
>  
> -    spapr->effective_caps = caps;
> +    spapr->mig_caps.caps[SPAPR_CAP_VSX] = 0;
> +    return 0;
> +}
>  
> -    /* .. then apply those caps to the virtual hardware */
> +const VMStateDescription vmstate_spapr_cap_vsx = {
> +    .name = "spapr/cap_vsx",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .needed = spapr_cap_vsx_needed,
> +    .pre_save = spapr_cap_vsx_pre_save,
> +    .pre_load = spapr_cap_vsx_pre_load,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT8(mig_caps.caps[SPAPR_CAP_VSX], sPAPRMachineState),
> +        VMSTATE_END_OF_LIST()
> +    },
> +};
>  
> -    for (i = 0; i < ARRAY_SIZE(capability_table); i++) {
> -        sPAPRCapabilityInfo *info = &capability_table[i];
> +static bool spapr_cap_dfp_needed(void *opaque)
> +{
> +    sPAPRMachineState *spapr = opaque;
>  
> -        if (spapr->effective_caps.mask & info->flag) {
> -            /* Failure to allow a cap is fatal - if the guest doesn't
> -             * have it, we'll be supplying an incorrect environment */
> -            if (info->allow) {
> -                info->allow(spapr, &error_fatal);
> -            }
> -        } else {
> -            /* Failure to enforce a cap is only a warning.  The guest
> -             * shouldn't be using it, since it's not advertised, so it
> -             * doesn't get to complain about weird behaviour if it
> -             * goes ahead anyway */
> -            if (info->disallow) {
> -                info->disallow(spapr, &local_err);
> -            }
> -            if (local_err) {
> -                warn_report_err(local_err);
> -                local_err = NULL;
> -            }
> -        }
> -    }
> +    return !!(spapr->cmd_line_caps.caps[SPAPR_CAP_DFP] & SPAPR_CAP_CMD_LINE);
>  }
>  
> -static void spapr_cap_get(Object *obj, Visitor *v, const char *name,
> -                          void *opaque, Error **errp)
> +static int spapr_cap_dfp_pre_save(void *opaque)
>  {
> -    sPAPRCapabilityInfo *cap = opaque;
> -    sPAPRMachineState *spapr = SPAPR_MACHINE(obj);
> -    bool value = spapr_has_cap(spapr, cap->flag);
> -
> -    /* TODO: Could this get called before effective_caps is finalized
> -     * in spapr_caps_reset()? */
> +    sPAPRMachineState *spapr = opaque;
>  
> -    visit_type_bool(v, name, &value, errp);
> +    spapr->mig_caps.caps[SPAPR_CAP_DFP] =
> +        spapr->cmd_line_caps.caps[SPAPR_CAP_DFP];
> +    return 0;
>  }
>  
> -static void spapr_cap_set(Object *obj, Visitor *v, const char *name,
> -                          void *opaque, Error **errp)
> +static int spapr_cap_dfp_pre_load(void *opaque)
>  {
> -    sPAPRCapabilityInfo *cap = opaque;
> -    sPAPRMachineState *spapr = SPAPR_MACHINE(obj);
> -    bool value;
> -    Error *local_err = NULL;
> -
> -    visit_type_bool(v, name, &value, &local_err);
> -    if (local_err) {
> -        error_propagate(errp, local_err);
> -        return;
> -    }
> +    sPAPRMachineState *spapr = opaque;
>  
> -    if (value) {
> -        spapr->forced_caps.mask |= cap->flag;
> -    } else {
> -        spapr->forbidden_caps.mask |= cap->flag;
> -    }
> +    spapr->mig_caps.caps[SPAPR_CAP_DFP] = 0;
> +    return 0;
>  }
>  
> -void spapr_caps_validate(sPAPRMachineState *spapr, Error **errp)
> +const VMStateDescription vmstate_spapr_cap_dfp = {
> +    .name = "spapr/cap_dfp",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .needed = spapr_cap_dfp_needed,
> +    .pre_save = spapr_cap_dfp_pre_save,
> +    .pre_load = spapr_cap_dfp_pre_load,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT8(mig_caps.caps[SPAPR_CAP_DFP], sPAPRMachineState),
> +        VMSTATE_END_OF_LIST()
> +    },
> +};
> +
> +void spapr_caps_reset(sPAPRMachineState *spapr)
>  {
> -    uint64_t allcaps = 0;
> +    sPAPRCapabilities caps;
>      int i;
>  
> -    for (i = 0; i < ARRAY_SIZE(capability_table); i++) {
> -        g_assert((allcaps & capability_table[i].flag) == 0);
> -        allcaps |= capability_table[i].flag;
> +    /* First compute the actual set of caps we're running with.. */
> +    caps = default_caps_with_cpu(spapr, first_cpu);
> +
> +    for (i = 0; i < SPAPR_CAP_NUM; i++) {
> +        /* Check if set from command line and override default if so */
> +        if (spapr->cmd_line_caps.caps[i] & SPAPR_CAP_CMD_LINE) {
> +            caps.caps[i] = spapr->cmd_line_caps.caps[i] & ~SPAPR_CAP_CMD_LINE;
> +        }
>      }
>  
> -    g_assert((spapr->forced_caps.mask & ~allcaps) == 0);
> -    g_assert((spapr->forbidden_caps.mask & ~allcaps) == 0);
> +    spapr->effective_caps = caps;
>  
> -    if (spapr->forced_caps.mask & spapr->forbidden_caps.mask) {
> -        error_setg(errp, "Some sPAPR capabilities set both on and off");
> -        return;
> +    /* .. then apply those caps to the virtual hardware */
> +
> +    for (i = 0; i < SPAPR_CAP_NUM; i++) {
> +        sPAPRCapabilityInfo *info = &capability_table[i];
> +
> +        /*
> +         * If the allow function can't set the desired level and think's it's

Nit, s/think's/thinks/

> +         * fatal, it should cause that.
> +         */
> +        info->allow(spapr, spapr->effective_caps.caps[i], &error_fatal);
>      }
>  }
>  
> @@ -322,17 +365,19 @@ void spapr_caps_add_properties(sPAPRMachineClass *smc, Error **errp)
>      for (i = 0; i < ARRAY_SIZE(capability_table); i++) {
>          sPAPRCapabilityInfo *cap = &capability_table[i];
>          const char *name = g_strdup_printf("cap-%s", cap->name);
> +        char *desc;
>  
> -        object_class_property_add(klass, name, "bool",
> -                                  spapr_cap_get, spapr_cap_set, NULL,
> -                                  cap, &local_err);
> +        object_class_property_add(klass, name, cap->type,
> +                                  cap->get, cap->set,
> +                                  NULL, cap, &local_err);
>          if (local_err) {
>              error_propagate(errp, local_err);
>              return;
>          }
>  
> -        object_class_property_set_description(klass, name, cap->description,
> -                                              &local_err);
> +        desc = g_strdup_printf("%s%s", cap->description, cap->options);
> +        object_class_property_set_description(klass, name, desc, &local_err);
> +        g_free(desc);
>          if (local_err) {
>              error_propagate(errp, local_err);
>              return;
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 26ac17e641..2804fbbf12 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -57,17 +57,26 @@ typedef enum {
>  /* These bits go in the migration stream, so they can't be reassigned */
>  
>  /* Hardware Transactional Memory */
> -#define SPAPR_CAP_HTM               0x0000000000000001ULL
> -
> +#define SPAPR_CAP_HTM                   0x00
>  /* Vector Scalar Extensions */
> -#define SPAPR_CAP_VSX               0x0000000000000002ULL
> -
> +#define SPAPR_CAP_VSX                   0x01
>  /* Decimal Floating Point */
> -#define SPAPR_CAP_DFP               0x0000000000000004ULL
> +#define SPAPR_CAP_DFP                   0x02
> +/* Num Caps */
> +#define SPAPR_CAP_NUM                   (SPAPR_CAP_DFP + 1)
> +
> +/*
> + * Capability Values
> + * NOTE: All execpt the immediately following MUST be less than 128 (0x80)
> + */
> +#define SPAPR_CAP_CMD_LINE              0x80
> +/* Bool Caps */
> +#define SPAPR_CAP_OFF                   0x00
> +#define SPAPR_CAP_ON                    0x01
>  
>  typedef struct sPAPRCapabilities sPAPRCapabilities;
>  struct sPAPRCapabilities {
> -    uint64_t mask;
> +    uint8_t caps[SPAPR_CAP_NUM];
>  };
>  
>  /**
> @@ -149,9 +158,8 @@ struct sPAPRMachineState {
>  
>      const char *icp_type;
>  
> -    sPAPRCapabilities forced_caps, forbidden_caps;
> -    sPAPRCapabilities mig_forced_caps, mig_forbidden_caps;
> -    sPAPRCapabilities effective_caps;
> +    sPAPRCapabilities cmd_line_caps, effective_caps;
> +    sPAPRCapabilities mig_caps;
>  };
>  
>  #define H_SUCCESS         0
> @@ -752,21 +760,16 @@ qemu_irq spapr_qirq(sPAPRMachineState *spapr, int irq);
>  /*
>   * Handling of optional capabilities
>   */
> -extern const VMStateDescription vmstate_spapr_caps;
> -
> -static inline sPAPRCapabilities spapr_caps(uint64_t mask)
> -{
> -    sPAPRCapabilities caps = { mask };
> -    return caps;
> -}
> +extern const VMStateDescription vmstate_spapr_cap_htm;
> +extern const VMStateDescription vmstate_spapr_cap_vsx;
> +extern const VMStateDescription vmstate_spapr_cap_dfp;
>  
> -static inline bool spapr_has_cap(sPAPRMachineState *spapr, uint64_t cap)
> +static inline uint8_t spapr_get_cap(sPAPRMachineState *spapr, int cap)
>  {
> -    return !!(spapr->effective_caps.mask & cap);
> +    return spapr->effective_caps.caps[cap];
>  }
>  
>  void spapr_caps_reset(sPAPRMachineState *spapr);
> -void spapr_caps_validate(sPAPRMachineState *spapr, Error **errp);
>  void spapr_caps_add_properties(sPAPRMachineClass *smc, Error **errp);
>  int spapr_caps_post_migration(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 --]

Re: [Qemu-devel] [QEMU-PPC] [RFC 1/3] hw/ppc/spapr_caps: Rework spapr_caps to use uint8 internal representation

[Suraj Jitindar Singh]

On Wed, 2018-01-10 at 15:13 +1100, David Gibson wrote:
> On Tue, Jan 09, 2018 at 08:21:01PM +1100, Suraj Jitindar Singh wrote:
> > Currently spapr_caps are tied to boolean values (on or off). This
> > patch
> > reworks the caps so that they can have any value between 0 and 127,
> > inclusive. This allows more capabilities with various values to be
> > represented in the same way internally. Capabilities are numbered
> > in
> > ascending order. The internal representation of capability values
> > is an
> > array of uint8s in the sPAPRMachineState, indexed by capability
> > number.
> > Note: The MSB (0x80) of a capability is reserved to track whether
> > the
> >       capability was set from the command line.
> > 
> > Capabilities can have their own name, description, options, getter
> > and
> > setter functions, type and allow functions. They also each have
> > their own
> > section in the migration stream. Capabilities are only migrated if
> > they
> > were explictly set on the command line, with the assumption that
> > otherwise the default will match.
> > 
> > On migration we ensure that the capability value on the destination
> > is greater than or equal to the capability value from the source.
> > So
> > long at this remains the case then the migration is considered
> > compatible and allowed to continue.
> > 
> > This patch implements generic getter and setter functions for
> > boolean
> > capabilities. It also converts the existings cap-htm, cap-vsx and
> > cap-dfp capabilities to this new format.
> > ---
> >  hw/ppc/spapr.c         |  19 +--
> >  hw/ppc/spapr_caps.c    | 335 ++++++++++++++++++++++++++++---------
> > ------------
> >  include/hw/ppc/spapr.h |  41 +++---
> >  3 files changed, 222 insertions(+), 173 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index d1acfe8858..7fa45729ba 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -320,7 +320,7 @@ static void
> > spapr_populate_pa_features(sPAPRMachineState *spapr,
> >           */
> >          pa_features[3] |= 0x20;
> >      }
> > -    if (spapr_has_cap(spapr, SPAPR_CAP_HTM) && pa_size > 24) {
> > +    if (spapr_get_cap(spapr, SPAPR_CAP_HTM) && pa_size > 24) {
> 
> I'd prefer to see explicit >0 or !=0 to emphasise that the returned
> value is now an integer not a bool.

Sure

> 
> >          pa_features[24] |= 0x80;    /* Transactional memory
> > support */
> >      }
> >      if (legacy_guest && pa_size > 40) {
> > @@ -563,7 +563,7 @@ static void spapr_populate_cpu_dt(CPUState *cs,
> > void *fdt, int offset,
> >       *
> >       * Only CPUs for which we create core types in
> > spapr_cpu_core.c
> >       * are possible, and all of those have VMX */
> > -    if (spapr_has_cap(spapr, SPAPR_CAP_VSX)) {
> > +    if (spapr_get_cap(spapr, SPAPR_CAP_VSX)) {
> >          _FDT((fdt_setprop_cell(fdt, offset, "ibm,vmx", 2)));
> >      } else {
> >          _FDT((fdt_setprop_cell(fdt, offset, "ibm,vmx", 1)));
> > @@ -572,7 +572,7 @@ static void spapr_populate_cpu_dt(CPUState *cs,
> > void *fdt, int offset,
> >      /* Advertise DFP (Decimal Floating Point) if available
> >       *   0 / no property == no DFP
> >       *   1               == DFP available */
> > -    if (spapr_has_cap(spapr, SPAPR_CAP_DFP)) {
> > +    if (spapr_get_cap(spapr, SPAPR_CAP_DFP)) {
> >          _FDT((fdt_setprop_cell(fdt, offset, "ibm,dfp", 1)));
> >      }
> >  
> > @@ -1762,7 +1762,9 @@ static const VMStateDescription vmstate_spapr
> > = {
> >          &vmstate_spapr_ov5_cas,
> >          &vmstate_spapr_patb_entry,
> >          &vmstate_spapr_pending_events,
> > -        &vmstate_spapr_caps,
> > +        &vmstate_spapr_cap_htm,
> > +        &vmstate_spapr_cap_vsx,
> > +        &vmstate_spapr_cap_dfp,
> >          NULL
> >      }
> >  };
> > @@ -2323,8 +2325,6 @@ static void spapr_machine_init(MachineState
> > *machine)
> >      char *filename;
> >      Error *resize_hpt_err = NULL;
> >  
> > -    spapr_caps_validate(spapr, &error_fatal);
> > -
> >      msi_nonbroken = true;
> >  
> >      QLIST_INIT(&spapr->phbs);
> > @@ -3834,7 +3834,9 @@ static void
> > spapr_machine_class_init(ObjectClass *oc, void *data)
> >       */
> >      mc->numa_mem_align_shift = 28;
> >  
> > -    smc->default_caps = spapr_caps(SPAPR_CAP_VSX | SPAPR_CAP_DFP);
> > +    smc->default_caps.caps[SPAPR_CAP_HTM] = SPAPR_CAP_OFF;
> > +    smc->default_caps.caps[SPAPR_CAP_VSX] = SPAPR_CAP_ON;
> > +    smc->default_caps.caps[SPAPR_CAP_DFP] = SPAPR_CAP_ON;
> >      spapr_caps_add_properties(smc, &error_abort);
> >  }
> >  
> > @@ -3916,8 +3918,7 @@ static void
> > spapr_machine_2_11_class_options(MachineClass *mc)
> >      sPAPRMachineClass *smc = SPAPR_MACHINE_CLASS(mc);
> >  
> >      spapr_machine_2_12_class_options(mc);
> > -    smc->default_caps = spapr_caps(SPAPR_CAP_HTM | SPAPR_CAP_VSX
> > -                                   | SPAPR_CAP_DFP);
> > +    smc->default_caps.caps[SPAPR_CAP_HTM] = SPAPR_CAP_ON;
> >      SET_MACHINE_COMPAT(mc, SPAPR_COMPAT_2_11);
> >  }
> >  
> > diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
> > index 9d070a306c..af40f2e469 100644
> > --- a/hw/ppc/spapr_caps.c
> > +++ b/hw/ppc/spapr_caps.c
> > @@ -35,33 +35,71 @@
> >  typedef struct sPAPRCapabilityInfo {
> >      const char *name;
> >      const char *description;
> > -    uint64_t flag;
> > +    const char *options;
> > +    int index;
> >  
> > +    /* Getter and Setter Function Pointers */
> > +    ObjectPropertyAccessor *get;
> > +    ObjectPropertyAccessor *set;
> > +    const char *type;
> >      /* Make sure the virtual hardware can support this capability
> > */
> > -    void (*allow)(sPAPRMachineState *spapr, Error **errp);
> > -
> > -    /* If possible, tell the virtual hardware not to allow the cap
> > to
> > -     * be used at all */
> > -    void (*disallow)(sPAPRMachineState *spapr, Error **errp);
> > +    void (*allow)(sPAPRMachineState *spapr, uint8_t val, Error
> > **errp);
> 
> I think we need a new name for this since it can both allow and
> disallow.  Maybe 'apply'?

Sure

> 
> >  } sPAPRCapabilityInfo;
> >  
> > -static void cap_htm_allow(sPAPRMachineState *spapr, Error **errp)
> > +static void spapr_cap_get_bool(Object *obj, Visitor *v, const char
> > *name,
> > +                               void *opaque, Error **errp)
> > +{
> > +    sPAPRCapabilityInfo *cap = opaque;
> > +    sPAPRMachineState *spapr = SPAPR_MACHINE(obj);
> > +    bool value = spapr_get_cap(spapr, cap->index) == SPAPR_CAP_ON;
> > +
> > +    visit_type_bool(v, name, &value, errp);
> > +}
> > +
> > +static void spapr_cap_set_bool(Object *obj, Visitor *v, const char
> > *name,
> > +                               void *opaque, Error **errp)
> >  {
> > +    sPAPRCapabilityInfo *cap = opaque;
> > +    sPAPRMachineState *spapr = SPAPR_MACHINE(obj);
> > +    bool value;
> > +    Error *local_err = NULL;
> > +
> > +    visit_type_bool(v, name, &value, &local_err);
> > +    if (local_err) {
> > +        error_propagate(errp, local_err);
> > +        return;
> > +    }
> > +
> > +    spapr->cmd_line_caps.caps[cap->index] = (value ? SPAPR_CAP_ON
> > :
> > +                                                     SPAPR_CAP_OFF
> > ) |
> > +                                             SPAPR_CAP_CMD_LINE;
> > +}
> > +
> > +static void cap_htm_allow(sPAPRMachineState *spapr, uint8_t val,
> > Error **errp)
> > +{
> > +    if (!val) {
> > +        /* TODO: We don't support disabling htm yet */
> > +        return;
> > +    }
> 
> A downside of fusing allow and disallow is that we can't now apply
> the
> rule that failure to allow is an error but failure to disallow is
> only
> a warning in the common code.  Oh well.

Yep, well it's up to the apply function now to either allow it or cause
an error if it thinks it's fatal.

> 
> >      if (tcg_enabled()) {
> >          error_setg(errp,
> > -                   "No Transactional Memory support in TCG, try
> > cap-htm=off");
> > +                   "No Transactional Memory support in TCG, try
> > cap-htm=0");
> >      } else if (kvm_enabled() && !kvmppc_has_cap_htm()) {
> >          error_setg(errp,
> > -"KVM implementation does not support Transactional Memory, try
> > cap-htm=off"
> > +"KVM implementation does not support Transactional Memory, try
> > cap-htm=0"
> >              );
> >      }
> >  }
> >  
> > -static void cap_vsx_allow(sPAPRMachineState *spapr, Error **errp)
> > +static void cap_vsx_allow(sPAPRMachineState *spapr, uint8_t val,
> > Error **errp)
> >  {
> >      PowerPCCPU *cpu = POWERPC_CPU(first_cpu);
> >      CPUPPCState *env = &cpu->env;
> >  
> > +    if (!val) {
> > +        /* TODO: We don't support disabling vsx yet */
> > +        return;
> > +    }
> >      /* Allowable CPUs in spapr_cpu_core.c should already have
> > gotten
> >       * rid of anything that doesn't do VMX */
> >      g_assert(env->insns_flags & PPC_ALTIVEC);
> > @@ -70,37 +108,51 @@ static void cap_vsx_allow(sPAPRMachineState
> > *spapr, Error **errp)
> >      }
> >  }
> >  
> > -static void cap_dfp_allow(sPAPRMachineState *spapr, Error **errp)
> > +static void cap_dfp_allow(sPAPRMachineState *spapr, uint8_t val,
> > Error **errp)
> >  {
> >      PowerPCCPU *cpu = POWERPC_CPU(first_cpu);
> >      CPUPPCState *env = &cpu->env;
> >  
> > +    if (!val) {
> > +        /* TODO: We don't support disabling dfp yet */
> > +        return;
> > +    }
> >      if (!(env->insns_flags2 & PPC2_DFP)) {
> >          error_setg(errp, "DFP support not available, try cap-
> > dfp=off");
> >      }
> >  }
> >  
> > -static sPAPRCapabilityInfo capability_table[] = {
> > -    {
> > +
> > +sPAPRCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
> > +    [SPAPR_CAP_HTM] = {
> >          .name = "htm",
> >          .description = "Allow Hardware Transactional Memory
> > (HTM)",
> > -        .flag = SPAPR_CAP_HTM,
> > +        .options = "",
> 
> It's not really clear to me what the options field is for.

So for string options it's for the allowed values which go in the
description.
See next patch :)

> 
> > +        .index = SPAPR_CAP_HTM,
> > +        .get = spapr_cap_get_bool,
> > +        .set = spapr_cap_set_bool,
> > +        .type = "bool",
> >          .allow = cap_htm_allow,
> > -        /* TODO: add cap_htm_disallow */
> >      },
> > -    {
> > +    [SPAPR_CAP_VSX] = {
> >          .name = "vsx",
> >          .description = "Allow Vector Scalar Extensions (VSX)",
> > -        .flag = SPAPR_CAP_VSX,
> > +        .options = "",
> > +        .index = SPAPR_CAP_VSX,
> > +        .get = spapr_cap_get_bool,
> > +        .set = spapr_cap_set_bool,
> > +        .type = "bool",
> >          .allow = cap_vsx_allow,
> > -        /* TODO: add cap_vsx_disallow */
> >      },
> > -    {
> > +    [SPAPR_CAP_DFP] = {
> >          .name = "dfp",
> >          .description = "Allow Decimal Floating Point (DFP)",
> > -        .flag = SPAPR_CAP_DFP,
> > +        .options = "",
> > +        .index = SPAPR_CAP_DFP,
> > +        .get = spapr_cap_get_bool,
> > +        .set = spapr_cap_set_bool,
> > +        .type = "bool",
> >          .allow = cap_dfp_allow,
> > -        /* TODO: add cap_dfp_disallow */
> >      },
> >  };
> >  
> > @@ -115,201 +167,192 @@ static sPAPRCapabilities
> > default_caps_with_cpu(sPAPRMachineState *spapr,
> >  
> >      if (!ppc_check_compat(cpu, CPU_POWERPC_LOGICAL_2_07,
> >                            0, spapr->max_compat_pvr)) {
> > -        caps.mask &= ~SPAPR_CAP_HTM;
> > +        caps.caps[SPAPR_CAP_HTM] = SPAPR_CAP_OFF;
> >      }
> >  
> >      if (!ppc_check_compat(cpu, CPU_POWERPC_LOGICAL_2_06,
> >                            0, spapr->max_compat_pvr)) {
> > -        caps.mask &= ~SPAPR_CAP_VSX;
> > -        caps.mask &= ~SPAPR_CAP_DFP;
> > +        caps.caps[SPAPR_CAP_VSX] = SPAPR_CAP_OFF;
> > +        caps.caps[SPAPR_CAP_DFP] = SPAPR_CAP_OFF;
> >      }
> >  
> >      return caps;
> >  }
> >  
> > -static bool spapr_caps_needed(void *opaque)
> > -{
> > -    sPAPRMachineState *spapr = opaque;
> > -
> > -    return (spapr->forced_caps.mask != 0) || (spapr-
> > >forbidden_caps.mask != 0);
> > -}
> > -
> >  /* This has to be called from the top-level spapr post_load, not
> > the
> >   * caps specific one.  Otherwise it wouldn't be called when the
> > source
> >   * caps are all defaults, which could still conflict with
> > overridden
> >   * caps on the destination */
> >  int spapr_caps_post_migration(sPAPRMachineState *spapr)
> >  {
> > -    uint64_t allcaps = 0;
> >      int i;
> >      bool ok = true;
> >      sPAPRCapabilities dstcaps = spapr->effective_caps;
> >      sPAPRCapabilities srccaps;
> >  
> >      srccaps = default_caps_with_cpu(spapr, first_cpu);
> > -    srccaps.mask |= spapr->mig_forced_caps.mask;
> > -    srccaps.mask &= ~spapr->mig_forbidden_caps.mask;
> > +    for (i = 0; i < SPAPR_CAP_NUM; i++) {
> > +        if (spapr->mig_caps.caps[i] & SPAPR_CAP_CMD_LINE) {
> > +            srccaps.caps[i] = spapr->mig_caps.caps[i] &
> > ~SPAPR_CAP_CMD_LINE;
> > +        }
> > +    }
> >  
> > -    for (i = 0; i < ARRAY_SIZE(capability_table); i++) {
> > +    for (i = 0; i < SPAPR_CAP_NUM; i++) {
> >          sPAPRCapabilityInfo *info = &capability_table[i];
> >  
> > -        allcaps |= info->flag;
> > -
> > -        if ((srccaps.mask & info->flag) && !(dstcaps.mask & info-
> > >flag)) {
> > -            error_report("cap-%s=on in incoming stream, but off in
> > destination",
> > -                         info->name);
> > +        if (srccaps.caps[i] > dstcaps.caps[i]) {
> > +            error_report("cap-%s higher level (%d) in incoming
> > stream than on destination (%d)",
> > +                         info->name, srccaps.caps[i],
> > dstcaps.caps[i]);
> >              ok = false;
> >          }
> >  
> > -        if (!(srccaps.mask & info->flag) && (dstcaps.mask & info-
> > >flag)) {
> > -            warn_report("cap-%s=off in incoming stream, but on in
> > destination",
> > -                         info->name);
> > +        if (srccaps.caps[i] < dstcaps.caps[i]) {
> > +            warn_report("cap-%s lower level (%d) in incoming
> > stream than on destination (%d)",
> > +                         info->name, srccaps.caps[i],
> > dstcaps.caps[i]);
> >          }
> >      }
> >  
> > -    if (spapr->mig_forced_caps.mask & ~allcaps) {
> > -        error_report(
> > -            "Unknown capabilities 0x%"PRIx64" enabled in incoming
> > stream",
> > -            spapr->mig_forced_caps.mask & ~allcaps);
> > -        ok = false;
> > -    }
> > -    if (spapr->mig_forbidden_caps.mask & ~allcaps) {
> > -        warn_report(
> > -            "Unknown capabilities 0x%"PRIx64" disabled in incoming
> > stream",
> > -            spapr->mig_forbidden_caps.mask & ~allcaps);
> > -    }
> > -
> >      return ok ? 0 : -EINVAL;
> >  }
> >  
> > -static int spapr_caps_pre_save(void *opaque)
> > +static bool spapr_cap_htm_needed(void *opaque)
> >  {
> >      sPAPRMachineState *spapr = opaque;
> >  
> > -    spapr->mig_forced_caps = spapr->forced_caps;
> > -    spapr->mig_forbidden_caps = spapr->forbidden_caps;
> > +    return !!(spapr->cmd_line_caps.caps[SPAPR_CAP_HTM] &
> > SPAPR_CAP_CMD_LINE);
> > +}
> > +
> > +static int spapr_cap_htm_pre_save(void *opaque)
> 
> Having to have a separate needed, pre_save and pre_load for each cap
> will be a pain.  I hope we can find a way to do this in common.

As discussed on IRC

> 
> > +{
> > +    sPAPRMachineState *spapr = opaque;
> > +
> > +    spapr->mig_caps.caps[SPAPR_CAP_HTM] =
> > +        spapr->cmd_line_caps.caps[SPAPR_CAP_HTM];
> >      return 0;
> >  }
> >  
> > -static int spapr_caps_pre_load(void *opaque)
> > +static int spapr_cap_htm_pre_load(void *opaque)
> >  {
> >      sPAPRMachineState *spapr = opaque;
> >  
> > -    spapr->mig_forced_caps = spapr_caps(0);
> > -    spapr->mig_forbidden_caps = spapr_caps(0);
> > +    spapr->mig_caps.caps[SPAPR_CAP_HTM] = 0;
> >      return 0;
> >  }
> >  
> > -const VMStateDescription vmstate_spapr_caps = {
> > -    .name = "spapr/caps",
> > +const VMStateDescription vmstate_spapr_cap_htm = {
> > +    .name = "spapr/cap_htm",
> 
> I'd suggest spapr/caps/htm - and a common vmstate_spapr_caps to hold
> the subsections for convenience rather than putting them straight
> under the master spapr one.

As discussed on IRC

> 
> >      .version_id = 1,
> >      .minimum_version_id = 1,
> > -    .needed = spapr_caps_needed,
> > -    .pre_save = spapr_caps_pre_save,
> > -    .pre_load = spapr_caps_pre_load,
> > +    .needed = spapr_cap_htm_needed,
> > +    .pre_save = spapr_cap_htm_pre_save,
> > +    .pre_load = spapr_cap_htm_pre_load,
> >      .fields = (VMStateField[]) {
> > -        VMSTATE_UINT64(mig_forced_caps.mask, sPAPRMachineState),
> > -        VMSTATE_UINT64(mig_forbidden_caps.mask,
> > sPAPRMachineState),
> > +        VMSTATE_UINT8(mig_caps.caps[SPAPR_CAP_HTM],
> > sPAPRMachineState),
> >          VMSTATE_END_OF_LIST()
> >      },
> >  };
> >  
> > -void spapr_caps_reset(sPAPRMachineState *spapr)
> > +static bool spapr_cap_vsx_needed(void *opaque)
> >  {
> > -    Error *local_err = NULL;
> > -    sPAPRCapabilities caps;
> > -    int i;
> > +    sPAPRMachineState *spapr = opaque;
> >  
> > -    /* First compute the actual set of caps we're running with..
> > */
> > -    caps = default_caps_with_cpu(spapr, first_cpu);
> > +    return !!(spapr->cmd_line_caps.caps[SPAPR_CAP_VSX] &
> > SPAPR_CAP_CMD_LINE);
> > +}
> >  
> > -    /* Remove unnecessary forced/forbidden bits (this will help us
> > -     * with migration) */
> > -    spapr->forced_caps.mask &= ~caps.mask;
> > -    spapr->forbidden_caps.mask &= caps.mask;
> 
> I don't think you have an equivalent of this in the new code, which
> means that an explicitly set property could prevent migration, even
> if
> it actually has the default value.

As discussed on IRC

> 
> > +static int spapr_cap_vsx_pre_save(void *opaque)
> > +{
> > +    sPAPRMachineState *spapr = opaque;
> > +
> > +    spapr->mig_caps.caps[SPAPR_CAP_VSX] =
> > +        spapr->cmd_line_caps.caps[SPAPR_CAP_VSX];
> > +    return 0;
> > +}
> >  
> > -    caps.mask |= spapr->forced_caps.mask;
> > -    caps.mask &= ~spapr->forbidden_caps.mask;
> > +static int spapr_cap_vsx_pre_load(void *opaque)
> > +{
> > +    sPAPRMachineState *spapr = opaque;
> >  
> > -    spapr->effective_caps = caps;
> > +    spapr->mig_caps.caps[SPAPR_CAP_VSX] = 0;
> > +    return 0;
> > +}
> >  
> > -    /* .. then apply those caps to the virtual hardware */
> > +const VMStateDescription vmstate_spapr_cap_vsx = {
> > +    .name = "spapr/cap_vsx",
> > +    .version_id = 1,
> > +    .minimum_version_id = 1,
> > +    .needed = spapr_cap_vsx_needed,
> > +    .pre_save = spapr_cap_vsx_pre_save,
> > +    .pre_load = spapr_cap_vsx_pre_load,
> > +    .fields = (VMStateField[]) {
> > +        VMSTATE_UINT8(mig_caps.caps[SPAPR_CAP_VSX],
> > sPAPRMachineState),
> > +        VMSTATE_END_OF_LIST()
> > +    },
> > +};
> >  
> > -    for (i = 0; i < ARRAY_SIZE(capability_table); i++) {
> > -        sPAPRCapabilityInfo *info = &capability_table[i];
> > +static bool spapr_cap_dfp_needed(void *opaque)
> > +{
> > +    sPAPRMachineState *spapr = opaque;
> >  
> > -        if (spapr->effective_caps.mask & info->flag) {
> > -            /* Failure to allow a cap is fatal - if the guest
> > doesn't
> > -             * have it, we'll be supplying an incorrect
> > environment */
> > -            if (info->allow) {
> > -                info->allow(spapr, &error_fatal);
> > -            }
> > -        } else {
> > -            /* Failure to enforce a cap is only a warning.  The
> > guest
> > -             * shouldn't be using it, since it's not advertised,
> > so it
> > -             * doesn't get to complain about weird behaviour if it
> > -             * goes ahead anyway */
> > -            if (info->disallow) {
> > -                info->disallow(spapr, &local_err);
> > -            }
> > -            if (local_err) {
> > -                warn_report_err(local_err);
> > -                local_err = NULL;
> > -            }
> > -        }
> > -    }
> > +    return !!(spapr->cmd_line_caps.caps[SPAPR_CAP_DFP] &
> > SPAPR_CAP_CMD_LINE);
> >  }
> >  
> > -static void spapr_cap_get(Object *obj, Visitor *v, const char
> > *name,
> > -                          void *opaque, Error **errp)
> > +static int spapr_cap_dfp_pre_save(void *opaque)
> >  {
> > -    sPAPRCapabilityInfo *cap = opaque;
> > -    sPAPRMachineState *spapr = SPAPR_MACHINE(obj);
> > -    bool value = spapr_has_cap(spapr, cap->flag);
> > -
> > -    /* TODO: Could this get called before effective_caps is
> > finalized
> > -     * in spapr_caps_reset()? */
> > +    sPAPRMachineState *spapr = opaque;
> >  
> > -    visit_type_bool(v, name, &value, errp);
> > +    spapr->mig_caps.caps[SPAPR_CAP_DFP] =
> > +        spapr->cmd_line_caps.caps[SPAPR_CAP_DFP];
> > +    return 0;
> >  }
> >  
> > -static void spapr_cap_set(Object *obj, Visitor *v, const char
> > *name,
> > -                          void *opaque, Error **errp)
> > +static int spapr_cap_dfp_pre_load(void *opaque)
> >  {
> > -    sPAPRCapabilityInfo *cap = opaque;
> > -    sPAPRMachineState *spapr = SPAPR_MACHINE(obj);
> > -    bool value;
> > -    Error *local_err = NULL;
> > -
> > -    visit_type_bool(v, name, &value, &local_err);
> > -    if (local_err) {
> > -        error_propagate(errp, local_err);
> > -        return;
> > -    }
> > +    sPAPRMachineState *spapr = opaque;
> >  
> > -    if (value) {
> > -        spapr->forced_caps.mask |= cap->flag;
> > -    } else {
> > -        spapr->forbidden_caps.mask |= cap->flag;
> > -    }
> > +    spapr->mig_caps.caps[SPAPR_CAP_DFP] = 0;
> > +    return 0;
> >  }
> >  
> > -void spapr_caps_validate(sPAPRMachineState *spapr, Error **errp)
> > +const VMStateDescription vmstate_spapr_cap_dfp = {
> > +    .name = "spapr/cap_dfp",
> > +    .version_id = 1,
> > +    .minimum_version_id = 1,
> > +    .needed = spapr_cap_dfp_needed,
> > +    .pre_save = spapr_cap_dfp_pre_save,
> > +    .pre_load = spapr_cap_dfp_pre_load,
> > +    .fields = (VMStateField[]) {
> > +        VMSTATE_UINT8(mig_caps.caps[SPAPR_CAP_DFP],
> > sPAPRMachineState),
> > +        VMSTATE_END_OF_LIST()
> > +    },
> > +};
> > +
> > +void spapr_caps_reset(sPAPRMachineState *spapr)
> >  {
> > -    uint64_t allcaps = 0;
> > +    sPAPRCapabilities caps;
> >      int i;
> >  
> > -    for (i = 0; i < ARRAY_SIZE(capability_table); i++) {
> > -        g_assert((allcaps & capability_table[i].flag) == 0);
> > -        allcaps |= capability_table[i].flag;
> > +    /* First compute the actual set of caps we're running with..
> > */
> > +    caps = default_caps_with_cpu(spapr, first_cpu);
> > +
> > +    for (i = 0; i < SPAPR_CAP_NUM; i++) {
> > +        /* Check if set from command line and override default if
> > so */
> > +        if (spapr->cmd_line_caps.caps[i] & SPAPR_CAP_CMD_LINE) {
> > +            caps.caps[i] = spapr->cmd_line_caps.caps[i] &
> > ~SPAPR_CAP_CMD_LINE;
> > +        }
> >      }
> >  
> > -    g_assert((spapr->forced_caps.mask & ~allcaps) == 0);
> > -    g_assert((spapr->forbidden_caps.mask & ~allcaps) == 0);
> > +    spapr->effective_caps = caps;
> >  
> > -    if (spapr->forced_caps.mask & spapr->forbidden_caps.mask) {
> > -        error_setg(errp, "Some sPAPR capabilities set both on and
> > off");
> > -        return;
> > +    /* .. then apply those caps to the virtual hardware */
> > +
> > +    for (i = 0; i < SPAPR_CAP_NUM; i++) {
> > +        sPAPRCapabilityInfo *info = &capability_table[i];
> > +
> > +        /*
> > +         * If the allow function can't set the desired level and
> > think's it's
> 
> Nit, s/think's/thinks/

yep

> 
> > +         * fatal, it should cause that.
> > +         */
> > +        info->allow(spapr, spapr->effective_caps.caps[i],
> > &error_fatal);
> >      }
> >  }
> >  
> > @@ -322,17 +365,19 @@ void
> > spapr_caps_add_properties(sPAPRMachineClass *smc, Error **errp)
> >      for (i = 0; i < ARRAY_SIZE(capability_table); i++) {
> >          sPAPRCapabilityInfo *cap = &capability_table[i];
> >          const char *name = g_strdup_printf("cap-%s", cap->name);
> > +        char *desc;
> >  
> > -        object_class_property_add(klass, name, "bool",
> > -                                  spapr_cap_get, spapr_cap_set,
> > NULL,
> > -                                  cap, &local_err);
> > +        object_class_property_add(klass, name, cap->type,
> > +                                  cap->get, cap->set,
> > +                                  NULL, cap, &local_err);
> >          if (local_err) {
> >              error_propagate(errp, local_err);
> >              return;
> >          }
> >  
> > -        object_class_property_set_description(klass, name, cap-
> > >description,
> > -                                              &local_err);
> > +        desc = g_strdup_printf("%s%s", cap->description, cap-
> > >options);
> > +        object_class_property_set_description(klass, name, desc,
> > &local_err);
> > +        g_free(desc);
> >          if (local_err) {
> >              error_propagate(errp, local_err);
> >              return;
> > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> > index 26ac17e641..2804fbbf12 100644
> > --- a/include/hw/ppc/spapr.h
> > +++ b/include/hw/ppc/spapr.h
> > @@ -57,17 +57,26 @@ typedef enum {
> >  /* These bits go in the migration stream, so they can't be
> > reassigned */
> >  
> >  /* Hardware Transactional Memory */
> > -#define SPAPR_CAP_HTM               0x0000000000000001ULL
> > -
> > +#define SPAPR_CAP_HTM                   0x00
> >  /* Vector Scalar Extensions */
> > -#define SPAPR_CAP_VSX               0x0000000000000002ULL
> > -
> > +#define SPAPR_CAP_VSX                   0x01
> >  /* Decimal Floating Point */
> > -#define SPAPR_CAP_DFP               0x0000000000000004ULL
> > +#define SPAPR_CAP_DFP                   0x02
> > +/* Num Caps */
> > +#define SPAPR_CAP_NUM                   (SPAPR_CAP_DFP + 1)
> > +
> > +/*
> > + * Capability Values
> > + * NOTE: All execpt the immediately following MUST be less than
> > 128 (0x80)
> > + */
> > +#define SPAPR_CAP_CMD_LINE              0x80
> > +/* Bool Caps */
> > +#define SPAPR_CAP_OFF                   0x00
> > +#define SPAPR_CAP_ON                    0x01
> >  
> >  typedef struct sPAPRCapabilities sPAPRCapabilities;
> >  struct sPAPRCapabilities {
> > -    uint64_t mask;
> > +    uint8_t caps[SPAPR_CAP_NUM];
> >  };
> >  
> >  /**
> > @@ -149,9 +158,8 @@ struct sPAPRMachineState {
> >  
> >      const char *icp_type;
> >  
> > -    sPAPRCapabilities forced_caps, forbidden_caps;
> > -    sPAPRCapabilities mig_forced_caps, mig_forbidden_caps;
> > -    sPAPRCapabilities effective_caps;
> > +    sPAPRCapabilities cmd_line_caps, effective_caps;
> > +    sPAPRCapabilities mig_caps;
> >  };
> >  
> >  #define H_SUCCESS         0
> > @@ -752,21 +760,16 @@ qemu_irq spapr_qirq(sPAPRMachineState *spapr,
> > int irq);
> >  /*
> >   * Handling of optional capabilities
> >   */
> > -extern const VMStateDescription vmstate_spapr_caps;
> > -
> > -static inline sPAPRCapabilities spapr_caps(uint64_t mask)
> > -{
> > -    sPAPRCapabilities caps = { mask };
> > -    return caps;
> > -}
> > +extern const VMStateDescription vmstate_spapr_cap_htm;
> > +extern const VMStateDescription vmstate_spapr_cap_vsx;
> > +extern const VMStateDescription vmstate_spapr_cap_dfp;
> >  
> > -static inline bool spapr_has_cap(sPAPRMachineState *spapr,
> > uint64_t cap)
> > +static inline uint8_t spapr_get_cap(sPAPRMachineState *spapr, int
> > cap)
> >  {
> > -    return !!(spapr->effective_caps.mask & cap);
> > +    return spapr->effective_caps.caps[cap];
> >  }
> >  
> >  void spapr_caps_reset(sPAPRMachineState *spapr);
> > -void spapr_caps_validate(sPAPRMachineState *spapr, Error **errp);
> >  void spapr_caps_add_properties(sPAPRMachineClass *smc, Error
> > **errp);
> >  int spapr_caps_post_migration(sPAPRMachineState *spapr);
> >  
> 
> 

[Qemu-devel] [QEMU-PPC] [RFC 2/3] hw/spapr/spapr_caps: Add new caps safe_[cache/bounds_check/indirect_branch]

[Suraj Jitindar Singh]

This patch adds three new capabilities:
cap-cfpc -> safe_cache
cap-sbbc -> safe_bounds_check
cap-ibs  -> safe_indirect_branch

Each capability is tristate with the possible values "broken",
"workaround" or "fixed". Add generic getter and setter functions for
this new capability type. Add these new capabilities to the capabilities
list. The maximum value for the capabilities is queried from kvm through
new kvm capabilities. The requested values are considered to be
compatible if kvm can support an equal or higher value for each
capability.

Discussion:
Currently these new capabilities default to broken to allow for
backwards compatibility, is this the best option?
---
 hw/ppc/spapr.c            |   6 ++
 hw/ppc/spapr_caps.c       | 224 ++++++++++++++++++++++++++++++++++++++++++++++
 include/hw/ppc/spapr.h    |  15 +++-
 linux-headers/linux/kvm.h |   3 +
 target/ppc/kvm.c          |  28 ++++++
 target/ppc/kvm_ppc.h      |  18 ++++
 6 files changed, 293 insertions(+), 1 deletion(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 7fa45729ba..d9700b0254 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1765,6 +1765,9 @@ static const VMStateDescription vmstate_spapr = {
         &vmstate_spapr_cap_htm,
         &vmstate_spapr_cap_vsx,
         &vmstate_spapr_cap_dfp,
+        &vmstate_spapr_cap_cfpc,
+        &vmstate_spapr_cap_sbbc,
+        &vmstate_spapr_cap_ibs,
         NULL
     }
 };
@@ -3837,6 +3840,9 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
     smc->default_caps.caps[SPAPR_CAP_HTM] = SPAPR_CAP_OFF;
     smc->default_caps.caps[SPAPR_CAP_VSX] = SPAPR_CAP_ON;
     smc->default_caps.caps[SPAPR_CAP_DFP] = SPAPR_CAP_ON;
+    smc->default_caps.caps[SPAPR_CAP_CFPC] = SPAPR_CAP_BROKEN;
+    smc->default_caps.caps[SPAPR_CAP_SBBC] = SPAPR_CAP_BROKEN;
+    smc->default_caps.caps[SPAPR_CAP_IBS] = SPAPR_CAP_BROKEN;
     spapr_caps_add_properties(smc, &error_abort);
 }
 
diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
index af40f2e469..e6910aa191 100644
--- a/hw/ppc/spapr_caps.c
+++ b/hw/ppc/spapr_caps.c
@@ -75,6 +75,64 @@ static void spapr_cap_set_bool(Object *obj, Visitor *v, const char *name,
                                              SPAPR_CAP_CMD_LINE;
 }
 
+static void spapr_cap_get_tristate(Object *obj, Visitor *v, const char *name,
+                                   void *opaque, Error **errp)
+{
+    sPAPRCapabilityInfo *cap = opaque;
+    sPAPRMachineState *spapr = SPAPR_MACHINE(obj);
+    char *val = NULL;
+    uint8_t value = spapr_get_cap(spapr, cap->index);
+
+    switch (value) {
+    case SPAPR_CAP_BROKEN:
+        val = g_strdup("broken");
+        break;
+    case SPAPR_CAP_WORKAROUND:
+        val = g_strdup("workaround");
+        break;
+    case SPAPR_CAP_FIXED:
+        val = g_strdup("fixed");
+        break;
+    default:
+        break;
+    }
+
+    visit_type_str(v, name, &val, errp);
+    g_free(val);
+}
+
+static void spapr_cap_set_tristate(Object *obj, Visitor *v, const char *name,
+                                   void *opaque, Error **errp)
+{
+    sPAPRCapabilityInfo *cap = opaque;
+    sPAPRMachineState *spapr = SPAPR_MACHINE(obj);
+    char *val;
+    Error *local_err = NULL;
+    uint8_t value;
+
+    visit_type_str(v, name, &val, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
+
+    if (!strcasecmp(val, "broken")) {
+        value = SPAPR_CAP_BROKEN;
+    } else if (!strcasecmp(val, "workaround")) {
+        value = SPAPR_CAP_WORKAROUND;
+    } else if (!strcasecmp(val, "fixed")) {
+        value = SPAPR_CAP_FIXED;
+    } else {
+        error_setg(errp, "Invalid capability mode \"%s\" for cap-%s", val,
+                   cap->name);
+        goto out;
+    }
+
+    spapr->cmd_line_caps.caps[cap->index] = value | SPAPR_CAP_CMD_LINE;
+out:
+    g_free(val);
+}
+
 static void cap_htm_allow(sPAPRMachineState *spapr, uint8_t val, Error **errp)
 {
     if (!val) {
@@ -122,6 +180,31 @@ static void cap_dfp_allow(sPAPRMachineState *spapr, uint8_t val, Error **errp)
     }
 }
 
+static void cap_safe_cache_allow(sPAPRMachineState *spapr, uint8_t val,
+                                 Error **errp)
+{
+    if (kvm_enabled() && (val > kvmppc_get_cap_safe_cache())) {
+        error_setg(errp, "Requested safe cache capability level not supported by kvm, try a different value for cap-cfpc");
+    }
+}
+
+static void cap_safe_bounds_check_allow(sPAPRMachineState *spapr, uint8_t val,
+                                        Error **errp)
+{
+    if (kvm_enabled() && (val > kvmppc_get_cap_safe_bounds_check())) {
+        error_setg(errp, "Requested safe bounds check capability level not supported by kvm, try a different value for cap-sbbc");
+    }
+}
+
+static void cap_safe_indirect_branch_allow(sPAPRMachineState *spapr,
+                                           uint8_t val, Error **errp)
+{
+    if (kvm_enabled() && (val > kvmppc_get_cap_safe_indirect_branch())) {
+        error_setg(errp, "Requested safe indirect branch capability level not supported by kvm, try a different value for cap-ibs");
+    }
+}
+
+#define VALUE_DESC_TRISTATE     " (broken, workaround, fixed)"
 
 sPAPRCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
     [SPAPR_CAP_HTM] = {
@@ -154,6 +237,36 @@ sPAPRCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
         .type = "bool",
         .allow = cap_dfp_allow,
     },
+    [SPAPR_CAP_CFPC] = {
+        .name = "cfpc",
+        .description = "Cache Flush on Privilege Change",
+        .options = VALUE_DESC_TRISTATE,
+        .index = SPAPR_CAP_CFPC,
+        .get = spapr_cap_get_tristate,
+        .set = spapr_cap_set_tristate,
+        .type = "string",
+        .allow = cap_safe_cache_allow,
+    },
+    [SPAPR_CAP_SBBC] = {
+        .name = "sbbc",
+        .description = "Speculation Barrier Bounds Checking",
+        .options = VALUE_DESC_TRISTATE,
+        .index = SPAPR_CAP_SBBC,
+        .get = spapr_cap_get_tristate,
+        .set = spapr_cap_set_tristate,
+        .type = "string",
+        .allow = cap_safe_bounds_check_allow,
+    },
+    [SPAPR_CAP_IBS] = {
+        .name = "ibs",
+        .description = "Indirect Branch Serialisation",
+        .options = VALUE_DESC_TRISTATE,
+        .index = SPAPR_CAP_IBS,
+        .get = spapr_cap_get_tristate,
+        .set = spapr_cap_set_tristate,
+        .type = "string",
+        .allow = cap_safe_indirect_branch_allow,
+    },
 };
 
 static sPAPRCapabilities default_caps_with_cpu(sPAPRMachineState *spapr,
@@ -326,6 +439,117 @@ const VMStateDescription vmstate_spapr_cap_dfp = {
     },
 };
 
+static bool spapr_cap_safe_cache_needed(void *opaque)
+{
+    sPAPRMachineState *spapr = opaque;
+
+    return !!(spapr->cmd_line_caps.caps[SPAPR_CAP_CFPC] & SPAPR_CAP_CMD_LINE);
+}
+
+static int spapr_cap_safe_cache_pre_save(void *opaque)
+{
+    sPAPRMachineState *spapr = opaque;
+
+    spapr->mig_caps.caps[SPAPR_CAP_CFPC] =
+        spapr->cmd_line_caps.caps[SPAPR_CAP_CFPC];
+    return 0;
+}
+
+static int spapr_cap_safe_cache_pre_load(void *opaque)
+{
+    sPAPRMachineState *spapr = opaque;
+
+    spapr->mig_caps.caps[SPAPR_CAP_CFPC] = 0;
+    return 0;
+}
+
+const VMStateDescription vmstate_spapr_cap_cfpc = {
+    .name = "spapr/cap_cfpc",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = spapr_cap_safe_cache_needed,
+    .pre_save = spapr_cap_safe_cache_pre_save,
+    .pre_load = spapr_cap_safe_cache_pre_load,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT8(mig_caps.caps[SPAPR_CAP_CFPC], sPAPRMachineState),
+        VMSTATE_END_OF_LIST()
+    },
+};
+
+static bool spapr_cap_safe_bounds_check_needed(void *opaque)
+{
+    sPAPRMachineState *spapr = opaque;
+
+    return !!(spapr->cmd_line_caps.caps[SPAPR_CAP_SBBC] & SPAPR_CAP_CMD_LINE);
+}
+
+static int spapr_cap_safe_bounds_check_pre_save(void *opaque)
+{
+    sPAPRMachineState *spapr = opaque;
+
+    spapr->mig_caps.caps[SPAPR_CAP_SBBC] =
+        spapr->cmd_line_caps.caps[SPAPR_CAP_SBBC];
+    return 0;
+}
+
+static int spapr_cap_safe_bounds_check_pre_load(void *opaque)
+{
+    sPAPRMachineState *spapr = opaque;
+
+    spapr->mig_caps.caps[SPAPR_CAP_SBBC] = 0;
+    return 0;
+}
+
+const VMStateDescription vmstate_spapr_cap_sbbc = {
+    .name = "spapr/cap_sbbc",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = spapr_cap_safe_bounds_check_needed,
+    .pre_save = spapr_cap_safe_bounds_check_pre_save,
+    .pre_load = spapr_cap_safe_bounds_check_pre_load,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT8(mig_caps.caps[SPAPR_CAP_SBBC], sPAPRMachineState),
+        VMSTATE_END_OF_LIST()
+    },
+};
+
+static bool spapr_cap_safe_indirect_branch_needed(void *opaque)
+{
+    sPAPRMachineState *spapr = opaque;
+
+    return !!(spapr->cmd_line_caps.caps[SPAPR_CAP_IBS] & SPAPR_CAP_CMD_LINE);
+}
+
+static int spapr_cap_safe_indirect_branch_pre_save(void *opaque)
+{
+    sPAPRMachineState *spapr = opaque;
+
+    spapr->mig_caps.caps[SPAPR_CAP_IBS] =
+        spapr->cmd_line_caps.caps[SPAPR_CAP_IBS];
+    return 0;
+}
+
+static int spapr_cap_safe_indirect_branch_pre_load(void *opaque)
+{
+    sPAPRMachineState *spapr = opaque;
+
+    spapr->mig_caps.caps[SPAPR_CAP_IBS] = 0;
+    return 0;
+}
+
+const VMStateDescription vmstate_spapr_cap_ibs = {
+    .name = "spapr/cap_ibs",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = spapr_cap_safe_indirect_branch_needed,
+    .pre_save = spapr_cap_safe_indirect_branch_pre_save,
+    .pre_load = spapr_cap_safe_indirect_branch_pre_load,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT8(mig_caps.caps[SPAPR_CAP_IBS], sPAPRMachineState),
+        VMSTATE_END_OF_LIST()
+    },
+};
+
 void spapr_caps_reset(sPAPRMachineState *spapr)
 {
     sPAPRCapabilities caps;
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 2804fbbf12..2db2f3e2e2 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -62,8 +62,14 @@ typedef enum {
 #define SPAPR_CAP_VSX                   0x01
 /* Decimal Floating Point */
 #define SPAPR_CAP_DFP                   0x02
+/* Cache Flush on Privilege Change */
+#define SPAPR_CAP_CFPC                  0x03
+/* Speculation Barrier Bounds Checking */
+#define SPAPR_CAP_SBBC                  0x04
+/* Indirect Branch Serialisation */
+#define SPAPR_CAP_IBS                   0x05
 /* Num Caps */
-#define SPAPR_CAP_NUM                   (SPAPR_CAP_DFP + 1)
+#define SPAPR_CAP_NUM                   (SPAPR_CAP_IBS + 1)
 
 /*
  * Capability Values
@@ -73,6 +79,10 @@ typedef enum {
 /* Bool Caps */
 #define SPAPR_CAP_OFF                   0x00
 #define SPAPR_CAP_ON                    0x01
+/* Broken | Workaround | Fixed Caps */
+#define SPAPR_CAP_BROKEN                0x00
+#define SPAPR_CAP_WORKAROUND            0x01
+#define SPAPR_CAP_FIXED                 0x02
 
 typedef struct sPAPRCapabilities sPAPRCapabilities;
 struct sPAPRCapabilities {
@@ -763,6 +773,9 @@ qemu_irq spapr_qirq(sPAPRMachineState *spapr, int irq);
 extern const VMStateDescription vmstate_spapr_cap_htm;
 extern const VMStateDescription vmstate_spapr_cap_vsx;
 extern const VMStateDescription vmstate_spapr_cap_dfp;
+extern const VMStateDescription vmstate_spapr_cap_cfpc;
+extern const VMStateDescription vmstate_spapr_cap_sbbc;
+extern const VMStateDescription vmstate_spapr_cap_ibs;
 
 static inline uint8_t spapr_get_cap(sPAPRMachineState *spapr, int cap)
 {
diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h
index ce6c2f11f4..0abe0a0abb 100644
--- a/linux-headers/linux/kvm.h
+++ b/linux-headers/linux/kvm.h
@@ -932,6 +932,9 @@ struct kvm_ppc_resize_hpt {
 #define KVM_CAP_HYPERV_SYNIC2 148
 #define KVM_CAP_HYPERV_VP_INDEX 149
 #define KVM_CAP_S390_AIS_MIGRATION 150
+#define KVM_CAP_PPC_SAFE_CACHE 151
+#define KVM_CAP_PPC_SAFE_BOUNDS_CHECK 152
+#define KVM_CAP_PPC_SAFE_INDIRECT_BRANCH 153
 
 #ifdef KVM_CAP_IRQ_ROUTING
 
diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
index 518dd06e98..818499237c 100644
--- a/target/ppc/kvm.c
+++ b/target/ppc/kvm.c
@@ -89,6 +89,9 @@ static int cap_mmu_radix;
 static int cap_mmu_hash_v3;
 static int cap_resize_hpt;
 static int cap_ppc_pvr_compat;
+static int cap_ppc_safe_cache;
+static int cap_ppc_safe_bounds_check;
+static int cap_ppc_safe_indirect_branch;
 
 static uint32_t debug_inst_opcode;
 
@@ -147,6 +150,16 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
     cap_mmu_radix = kvm_vm_check_extension(s, KVM_CAP_PPC_MMU_RADIX);
     cap_mmu_hash_v3 = kvm_vm_check_extension(s, KVM_CAP_PPC_MMU_HASH_V3);
     cap_resize_hpt = kvm_vm_check_extension(s, KVM_CAP_SPAPR_RESIZE_HPT);
+    cap_ppc_safe_cache = kvm_vm_check_extension(s, KVM_CAP_PPC_SAFE_CACHE);
+    cap_ppc_safe_cache = cap_ppc_safe_cache > 0 ? cap_ppc_safe_cache : 0;
+    cap_ppc_safe_bounds_check = kvm_vm_check_extension(s,
+                                KVM_CAP_PPC_SAFE_BOUNDS_CHECK);
+    cap_ppc_safe_bounds_check = cap_ppc_safe_bounds_check > 0 ?
+                                cap_ppc_safe_bounds_check : 0;
+    cap_ppc_safe_indirect_branch = kvm_vm_check_extension(s,
+                                   KVM_CAP_PPC_SAFE_INDIRECT_BRANCH);
+    cap_ppc_safe_indirect_branch = cap_ppc_safe_indirect_branch > 0 ?
+                                   cap_ppc_safe_indirect_branch : 0;
     /*
      * Note: setting it to false because there is not such capability
      * in KVM at this moment.
@@ -2456,6 +2469,21 @@ bool kvmppc_has_cap_mmu_hash_v3(void)
     return cap_mmu_hash_v3;
 }
 
+int kvmppc_get_cap_safe_cache(void)
+{
+    return cap_ppc_safe_cache;
+}
+
+int kvmppc_get_cap_safe_bounds_check(void)
+{
+    return cap_ppc_safe_bounds_check;
+}
+
+int kvmppc_get_cap_safe_indirect_branch(void)
+{
+    return cap_ppc_safe_indirect_branch;
+}
+
 PowerPCCPUClass *kvm_ppc_get_host_cpu_class(void)
 {
     uint32_t host_pvr = mfpvr();
diff --git a/target/ppc/kvm_ppc.h b/target/ppc/kvm_ppc.h
index ecb55493cc..39830baa77 100644
--- a/target/ppc/kvm_ppc.h
+++ b/target/ppc/kvm_ppc.h
@@ -59,6 +59,9 @@ bool kvmppc_has_cap_fixup_hcalls(void);
 bool kvmppc_has_cap_htm(void);
 bool kvmppc_has_cap_mmu_radix(void);
 bool kvmppc_has_cap_mmu_hash_v3(void);
+int kvmppc_get_cap_safe_cache(void);
+int kvmppc_get_cap_safe_bounds_check(void);
+int kvmppc_get_cap_safe_indirect_branch(void);
 int kvmppc_enable_hwrng(void);
 int kvmppc_put_books_sregs(PowerPCCPU *cpu);
 PowerPCCPUClass *kvm_ppc_get_host_cpu_class(void);
@@ -290,6 +293,21 @@ static inline bool kvmppc_has_cap_mmu_hash_v3(void)
     return false;
 }
 
+static inline int kvmppc_get_cap_safe_cache(void)
+{
+    return 0;
+}
+
+static inline int kvmppc_get_cap_safe_bounds_check(void)
+{
+    return 0;
+}
+
+static inline int kvmppc_get_cap_safe_indirect_branch(void)
+{
+    return 0;
+}
+
 static inline int kvmppc_enable_hwrng(void)
 {
     return -1;
-- 
2.13.6

Re: [Qemu-devel] [Qemu-ppc] [QEMU-PPC] [RFC 2/3] hw/spapr/spapr_caps: Add new caps safe_[cache/bounds_check/indirect_branch]

[Murilo Opsfelder Araújo]

On 01/09/2018 07:21 AM, Suraj Jitindar Singh wrote:
> This patch adds three new capabilities:
> cap-cfpc -> safe_cache
> cap-sbbc -> safe_bounds_check
> cap-ibs  -> safe_indirect_branch

Hi, Suraj.

What about splitting this into smaller patches, one per capability?

> Each capability is tristate with the possible values "broken",
> "workaround" or "fixed". Add generic getter and setter functions for
> this new capability type. Add these new capabilities to the capabilities
> list. The maximum value for the capabilities is queried from kvm through
> new kvm capabilities. The requested values are considered to be
> compatible if kvm can support an equal or higher value for each
> capability.
> 
> Discussion:
> Currently these new capabilities default to broken to allow for
> backwards compatibility, is this the best option?

This could be placed in the cover letter, not in the commit message.

Cheers
Murilo

Re: [Qemu-devel] [Qemu-ppc] [QEMU-PPC] [RFC 2/3] hw/spapr/spapr_caps: Add new caps safe_[cache/bounds_check/indirect_branch]

[Suraj Jitindar Singh]

On Tue, 2018-01-09 at 09:15 -0200, Murilo Opsfelder Araújo wrote:
> On 01/09/2018 07:21 AM, Suraj Jitindar Singh wrote:
> > This patch adds three new capabilities:
> > cap-cfpc -> safe_cache
> > cap-sbbc -> safe_bounds_check
> > cap-ibs  -> safe_indirect_branch
> 
> Hi, Suraj.
> 
> What about splitting this into smaller patches, one per capability?

Yep

> 
> > Each capability is tristate with the possible values "broken",
> > "workaround" or "fixed". Add generic getter and setter functions
> > for
> > this new capability type. Add these new capabilities to the
> > capabilities
> > list. The maximum value for the capabilities is queried from kvm
> > through
> > new kvm capabilities. The requested values are considered to be
> > compatible if kvm can support an equal or higher value for each
> > capability.
> > 
> > Discussion:
> > Currently these new capabilities default to broken to allow for
> > backwards compatibility, is this the best option?
> 
> This could be placed in the cover letter, not in the commit 

Only here because this is an RFC

> 
> Cheers
> Murilo
> 

Re: [Qemu-devel] [QEMU-PPC] [RFC 2/3] hw/spapr/spapr_caps: Add new caps safe_[cache/bounds_check/indirect_branch]

[joserz]

On Tue, Jan 09, 2018 at 08:21:02PM +1100, Suraj Jitindar Singh wrote:
> This patch adds three new capabilities:
> cap-cfpc -> safe_cache
> cap-sbbc -> safe_bounds_check
> cap-ibs  -> safe_indirect_branch
> 
> Each capability is tristate with the possible values "broken",
> "workaround" or "fixed". Add generic getter and setter functions for
> this new capability type. Add these new capabilities to the capabilities
> list. The maximum value for the capabilities is queried from kvm through
> new kvm capabilities. The requested values are considered to be
> compatible if kvm can support an equal or higher value for each
> capability.
> 
> Discussion:
> Currently these new capabilities default to broken to allow for
> backwards compatibility, is this the best option?
> ---
>  hw/ppc/spapr.c            |   6 ++
>  hw/ppc/spapr_caps.c       | 224 ++++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/ppc/spapr.h    |  15 +++-
>  linux-headers/linux/kvm.h |   3 +
>  target/ppc/kvm.c          |  28 ++++++
>  target/ppc/kvm_ppc.h      |  18 ++++
>  6 files changed, 293 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 7fa45729ba..d9700b0254 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1765,6 +1765,9 @@ static const VMStateDescription vmstate_spapr = {
>          &vmstate_spapr_cap_htm,
>          &vmstate_spapr_cap_vsx,
>          &vmstate_spapr_cap_dfp,
> +        &vmstate_spapr_cap_cfpc,
> +        &vmstate_spapr_cap_sbbc,
> +        &vmstate_spapr_cap_ibs,
>          NULL
>      }
>  };
> @@ -3837,6 +3840,9 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
>      smc->default_caps.caps[SPAPR_CAP_HTM] = SPAPR_CAP_OFF;
>      smc->default_caps.caps[SPAPR_CAP_VSX] = SPAPR_CAP_ON;
>      smc->default_caps.caps[SPAPR_CAP_DFP] = SPAPR_CAP_ON;
> +    smc->default_caps.caps[SPAPR_CAP_CFPC] = SPAPR_CAP_BROKEN;
> +    smc->default_caps.caps[SPAPR_CAP_SBBC] = SPAPR_CAP_BROKEN;
> +    smc->default_caps.caps[SPAPR_CAP_IBS] = SPAPR_CAP_BROKEN;
>      spapr_caps_add_properties(smc, &error_abort);
>  }
> 
> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
> index af40f2e469..e6910aa191 100644
> --- a/hw/ppc/spapr_caps.c
> +++ b/hw/ppc/spapr_caps.c
> @@ -75,6 +75,64 @@ static void spapr_cap_set_bool(Object *obj, Visitor *v, const char *name,
>                                               SPAPR_CAP_CMD_LINE;
>  }
> 
> +static void spapr_cap_get_tristate(Object *obj, Visitor *v, const char *name,
> +                                   void *opaque, Error **errp)
> +{
> +    sPAPRCapabilityInfo *cap = opaque;
> +    sPAPRMachineState *spapr = SPAPR_MACHINE(obj);
> +    char *val = NULL;
> +    uint8_t value = spapr_get_cap(spapr, cap->index);
> +
> +    switch (value) {
> +    case SPAPR_CAP_BROKEN:
> +        val = g_strdup("broken");
> +        break;
> +    case SPAPR_CAP_WORKAROUND:
> +        val = g_strdup("workaround");
> +        break;
> +    case SPAPR_CAP_FIXED:
> +        val = g_strdup("fixed");
> +        break;
> +    default:
> +        break;

Hello Suraj

Is default a possible case? If so, will val be handled correctly by
visit_type_str and g_free?

if not, maybe a throwing an error like you did in
spapr_cap_set_tristate could be better.

> +    }
> +
> +    visit_type_str(v, name, &val, errp);
> +    g_free(val);
> +}
> +
> +static void spapr_cap_set_tristate(Object *obj, Visitor *v, const char *name,
> +                                   void *opaque, Error **errp)
> +{
> +    sPAPRCapabilityInfo *cap = opaque;
> +    sPAPRMachineState *spapr = SPAPR_MACHINE(obj);
> +    char *val;
> +    Error *local_err = NULL;
> +    uint8_t value;
> +
> +    visit_type_str(v, name, &val, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +
> +    if (!strcasecmp(val, "broken")) {
> +        value = SPAPR_CAP_BROKEN;
> +    } else if (!strcasecmp(val, "workaround")) {
> +        value = SPAPR_CAP_WORKAROUND;
> +    } else if (!strcasecmp(val, "fixed")) {
> +        value = SPAPR_CAP_FIXED;
> +    } else {
> +        error_setg(errp, "Invalid capability mode \"%s\" for cap-%s", val,
> +                   cap->name);
> +        goto out;
> +    }
> +
> +    spapr->cmd_line_caps.caps[cap->index] = value | SPAPR_CAP_CMD_LINE;
> +out:
> +    g_free(val);
> +}
> +
>  static void cap_htm_allow(sPAPRMachineState *spapr, uint8_t val, Error **errp)
>  {
>      if (!val) {
> @@ -122,6 +180,31 @@ static void cap_dfp_allow(sPAPRMachineState *spapr, uint8_t val, Error **errp)
>      }
>  }
> 
> +static void cap_safe_cache_allow(sPAPRMachineState *spapr, uint8_t val,
> +                                 Error **errp)
> +{
> +    if (kvm_enabled() && (val > kvmppc_get_cap_safe_cache())) {
> +        error_setg(errp, "Requested safe cache capability level not supported by kvm, try a different value for cap-cfpc");
> +    }
> +}
> +
> +static void cap_safe_bounds_check_allow(sPAPRMachineState *spapr, uint8_t val,
> +                                        Error **errp)
> +{
> +    if (kvm_enabled() && (val > kvmppc_get_cap_safe_bounds_check())) {
> +        error_setg(errp, "Requested safe bounds check capability level not supported by kvm, try a different value for cap-sbbc");
> +    }
> +}
> +
> +static void cap_safe_indirect_branch_allow(sPAPRMachineState *spapr,
> +                                           uint8_t val, Error **errp)
> +{
> +    if (kvm_enabled() && (val > kvmppc_get_cap_safe_indirect_branch())) {
> +        error_setg(errp, "Requested safe indirect branch capability level not supported by kvm, try a different value for cap-ibs");
> +    }
> +}
> +
> +#define VALUE_DESC_TRISTATE     " (broken, workaround, fixed)"
> 
>  sPAPRCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
>      [SPAPR_CAP_HTM] = {
> @@ -154,6 +237,36 @@ sPAPRCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
>          .type = "bool",
>          .allow = cap_dfp_allow,
>      },
> +    [SPAPR_CAP_CFPC] = {
> +        .name = "cfpc",
> +        .description = "Cache Flush on Privilege Change",
> +        .options = VALUE_DESC_TRISTATE,
> +        .index = SPAPR_CAP_CFPC,
> +        .get = spapr_cap_get_tristate,
> +        .set = spapr_cap_set_tristate,
> +        .type = "string",
> +        .allow = cap_safe_cache_allow,
> +    },
> +    [SPAPR_CAP_SBBC] = {
> +        .name = "sbbc",
> +        .description = "Speculation Barrier Bounds Checking",
> +        .options = VALUE_DESC_TRISTATE,
> +        .index = SPAPR_CAP_SBBC,
> +        .get = spapr_cap_get_tristate,
> +        .set = spapr_cap_set_tristate,
> +        .type = "string",
> +        .allow = cap_safe_bounds_check_allow,
> +    },
> +    [SPAPR_CAP_IBS] = {
> +        .name = "ibs",
> +        .description = "Indirect Branch Serialisation",
> +        .options = VALUE_DESC_TRISTATE,
> +        .index = SPAPR_CAP_IBS,
> +        .get = spapr_cap_get_tristate,
> +        .set = spapr_cap_set_tristate,
> +        .type = "string",
> +        .allow = cap_safe_indirect_branch_allow,
> +    },
>  };
> 
>  static sPAPRCapabilities default_caps_with_cpu(sPAPRMachineState *spapr,
> @@ -326,6 +439,117 @@ const VMStateDescription vmstate_spapr_cap_dfp = {
>      },
>  };
> 
> +static bool spapr_cap_safe_cache_needed(void *opaque)
> +{
> +    sPAPRMachineState *spapr = opaque;
> +
> +    return !!(spapr->cmd_line_caps.caps[SPAPR_CAP_CFPC] & SPAPR_CAP_CMD_LINE);
> +}
> +
> +static int spapr_cap_safe_cache_pre_save(void *opaque)
> +{
> +    sPAPRMachineState *spapr = opaque;
> +
> +    spapr->mig_caps.caps[SPAPR_CAP_CFPC] =
> +        spapr->cmd_line_caps.caps[SPAPR_CAP_CFPC];
> +    return 0;
> +}
> +
> +static int spapr_cap_safe_cache_pre_load(void *opaque)
> +{
> +    sPAPRMachineState *spapr = opaque;
> +
> +    spapr->mig_caps.caps[SPAPR_CAP_CFPC] = 0;
> +    return 0;
> +}
> +
> +const VMStateDescription vmstate_spapr_cap_cfpc = {
> +    .name = "spapr/cap_cfpc",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .needed = spapr_cap_safe_cache_needed,
> +    .pre_save = spapr_cap_safe_cache_pre_save,
> +    .pre_load = spapr_cap_safe_cache_pre_load,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT8(mig_caps.caps[SPAPR_CAP_CFPC], sPAPRMachineState),
> +        VMSTATE_END_OF_LIST()
> +    },
> +};
> +
> +static bool spapr_cap_safe_bounds_check_needed(void *opaque)
> +{
> +    sPAPRMachineState *spapr = opaque;
> +
> +    return !!(spapr->cmd_line_caps.caps[SPAPR_CAP_SBBC] & SPAPR_CAP_CMD_LINE);
> +}
> +
> +static int spapr_cap_safe_bounds_check_pre_save(void *opaque)
> +{
> +    sPAPRMachineState *spapr = opaque;
> +
> +    spapr->mig_caps.caps[SPAPR_CAP_SBBC] =
> +        spapr->cmd_line_caps.caps[SPAPR_CAP_SBBC];
> +    return 0;
> +}
> +
> +static int spapr_cap_safe_bounds_check_pre_load(void *opaque)
> +{
> +    sPAPRMachineState *spapr = opaque;
> +
> +    spapr->mig_caps.caps[SPAPR_CAP_SBBC] = 0;
> +    return 0;
> +}
> +
> +const VMStateDescription vmstate_spapr_cap_sbbc = {
> +    .name = "spapr/cap_sbbc",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .needed = spapr_cap_safe_bounds_check_needed,
> +    .pre_save = spapr_cap_safe_bounds_check_pre_save,
> +    .pre_load = spapr_cap_safe_bounds_check_pre_load,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT8(mig_caps.caps[SPAPR_CAP_SBBC], sPAPRMachineState),
> +        VMSTATE_END_OF_LIST()
> +    },
> +};
> +
> +static bool spapr_cap_safe_indirect_branch_needed(void *opaque)
> +{
> +    sPAPRMachineState *spapr = opaque;
> +
> +    return !!(spapr->cmd_line_caps.caps[SPAPR_CAP_IBS] & SPAPR_CAP_CMD_LINE);
> +}
> +
> +static int spapr_cap_safe_indirect_branch_pre_save(void *opaque)
> +{
> +    sPAPRMachineState *spapr = opaque;
> +
> +    spapr->mig_caps.caps[SPAPR_CAP_IBS] =
> +        spapr->cmd_line_caps.caps[SPAPR_CAP_IBS];
> +    return 0;
> +}
> +
> +static int spapr_cap_safe_indirect_branch_pre_load(void *opaque)
> +{
> +    sPAPRMachineState *spapr = opaque;
> +
> +    spapr->mig_caps.caps[SPAPR_CAP_IBS] = 0;
> +    return 0;
> +}
> +
> +const VMStateDescription vmstate_spapr_cap_ibs = {
> +    .name = "spapr/cap_ibs",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .needed = spapr_cap_safe_indirect_branch_needed,
> +    .pre_save = spapr_cap_safe_indirect_branch_pre_save,
> +    .pre_load = spapr_cap_safe_indirect_branch_pre_load,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT8(mig_caps.caps[SPAPR_CAP_IBS], sPAPRMachineState),
> +        VMSTATE_END_OF_LIST()
> +    },
> +};
> +
>  void spapr_caps_reset(sPAPRMachineState *spapr)
>  {
>      sPAPRCapabilities caps;
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 2804fbbf12..2db2f3e2e2 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -62,8 +62,14 @@ typedef enum {
>  #define SPAPR_CAP_VSX                   0x01
>  /* Decimal Floating Point */
>  #define SPAPR_CAP_DFP                   0x02
> +/* Cache Flush on Privilege Change */
> +#define SPAPR_CAP_CFPC                  0x03
> +/* Speculation Barrier Bounds Checking */
> +#define SPAPR_CAP_SBBC                  0x04
> +/* Indirect Branch Serialisation */
> +#define SPAPR_CAP_IBS                   0x05
>  /* Num Caps */
> -#define SPAPR_CAP_NUM                   (SPAPR_CAP_DFP + 1)
> +#define SPAPR_CAP_NUM                   (SPAPR_CAP_IBS + 1)
> 
>  /*
>   * Capability Values
> @@ -73,6 +79,10 @@ typedef enum {
>  /* Bool Caps */
>  #define SPAPR_CAP_OFF                   0x00
>  #define SPAPR_CAP_ON                    0x01
> +/* Broken | Workaround | Fixed Caps */
> +#define SPAPR_CAP_BROKEN                0x00
> +#define SPAPR_CAP_WORKAROUND            0x01
> +#define SPAPR_CAP_FIXED                 0x02
> 
>  typedef struct sPAPRCapabilities sPAPRCapabilities;
>  struct sPAPRCapabilities {
> @@ -763,6 +773,9 @@ qemu_irq spapr_qirq(sPAPRMachineState *spapr, int irq);
>  extern const VMStateDescription vmstate_spapr_cap_htm;
>  extern const VMStateDescription vmstate_spapr_cap_vsx;
>  extern const VMStateDescription vmstate_spapr_cap_dfp;
> +extern const VMStateDescription vmstate_spapr_cap_cfpc;
> +extern const VMStateDescription vmstate_spapr_cap_sbbc;
> +extern const VMStateDescription vmstate_spapr_cap_ibs;
> 
>  static inline uint8_t spapr_get_cap(sPAPRMachineState *spapr, int cap)
>  {
> diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h
> index ce6c2f11f4..0abe0a0abb 100644
> --- a/linux-headers/linux/kvm.h
> +++ b/linux-headers/linux/kvm.h
> @@ -932,6 +932,9 @@ struct kvm_ppc_resize_hpt {
>  #define KVM_CAP_HYPERV_SYNIC2 148
>  #define KVM_CAP_HYPERV_VP_INDEX 149
>  #define KVM_CAP_S390_AIS_MIGRATION 150
> +#define KVM_CAP_PPC_SAFE_CACHE 151
> +#define KVM_CAP_PPC_SAFE_BOUNDS_CHECK 152
> +#define KVM_CAP_PPC_SAFE_INDIRECT_BRANCH 153
> 
>  #ifdef KVM_CAP_IRQ_ROUTING
> 
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index 518dd06e98..818499237c 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -89,6 +89,9 @@ static int cap_mmu_radix;
>  static int cap_mmu_hash_v3;
>  static int cap_resize_hpt;
>  static int cap_ppc_pvr_compat;
> +static int cap_ppc_safe_cache;
> +static int cap_ppc_safe_bounds_check;
> +static int cap_ppc_safe_indirect_branch;
> 
>  static uint32_t debug_inst_opcode;
> 
> @@ -147,6 +150,16 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
>      cap_mmu_radix = kvm_vm_check_extension(s, KVM_CAP_PPC_MMU_RADIX);
>      cap_mmu_hash_v3 = kvm_vm_check_extension(s, KVM_CAP_PPC_MMU_HASH_V3);
>      cap_resize_hpt = kvm_vm_check_extension(s, KVM_CAP_SPAPR_RESIZE_HPT);
> +    cap_ppc_safe_cache = kvm_vm_check_extension(s, KVM_CAP_PPC_SAFE_CACHE);
> +    cap_ppc_safe_cache = cap_ppc_safe_cache > 0 ? cap_ppc_safe_cache : 0;
> +    cap_ppc_safe_bounds_check = kvm_vm_check_extension(s,
> +                                KVM_CAP_PPC_SAFE_BOUNDS_CHECK);
> +    cap_ppc_safe_bounds_check = cap_ppc_safe_bounds_check > 0 ?
> +                                cap_ppc_safe_bounds_check : 0;
> +    cap_ppc_safe_indirect_branch = kvm_vm_check_extension(s,
> +                                   KVM_CAP_PPC_SAFE_INDIRECT_BRANCH);
> +    cap_ppc_safe_indirect_branch = cap_ppc_safe_indirect_branch > 0 ?
> +                                   cap_ppc_safe_indirect_branch : 0;
>      /*
>       * Note: setting it to false because there is not such capability
>       * in KVM at this moment.
> @@ -2456,6 +2469,21 @@ bool kvmppc_has_cap_mmu_hash_v3(void)
>      return cap_mmu_hash_v3;
>  }
> 
> +int kvmppc_get_cap_safe_cache(void)
> +{
> +    return cap_ppc_safe_cache;
> +}
> +
> +int kvmppc_get_cap_safe_bounds_check(void)
> +{
> +    return cap_ppc_safe_bounds_check;
> +}
> +
> +int kvmppc_get_cap_safe_indirect_branch(void)
> +{
> +    return cap_ppc_safe_indirect_branch;
> +}
> +
>  PowerPCCPUClass *kvm_ppc_get_host_cpu_class(void)
>  {
>      uint32_t host_pvr = mfpvr();
> diff --git a/target/ppc/kvm_ppc.h b/target/ppc/kvm_ppc.h
> index ecb55493cc..39830baa77 100644
> --- a/target/ppc/kvm_ppc.h
> +++ b/target/ppc/kvm_ppc.h
> @@ -59,6 +59,9 @@ bool kvmppc_has_cap_fixup_hcalls(void);
>  bool kvmppc_has_cap_htm(void);
>  bool kvmppc_has_cap_mmu_radix(void);
>  bool kvmppc_has_cap_mmu_hash_v3(void);
> +int kvmppc_get_cap_safe_cache(void);
> +int kvmppc_get_cap_safe_bounds_check(void);
> +int kvmppc_get_cap_safe_indirect_branch(void);
>  int kvmppc_enable_hwrng(void);
>  int kvmppc_put_books_sregs(PowerPCCPU *cpu);
>  PowerPCCPUClass *kvm_ppc_get_host_cpu_class(void);
> @@ -290,6 +293,21 @@ static inline bool kvmppc_has_cap_mmu_hash_v3(void)
>      return false;
>  }
> 
> +static inline int kvmppc_get_cap_safe_cache(void)
> +{
> +    return 0;
> +}
> +
> +static inline int kvmppc_get_cap_safe_bounds_check(void)
> +{
> +    return 0;
> +}
> +
> +static inline int kvmppc_get_cap_safe_indirect_branch(void)
> +{
> +    return 0;
> +}
> +
>  static inline int kvmppc_enable_hwrng(void)
>  {
>      return -1;
> -- 
> 2.13.6
> 
> 

Re: [Qemu-devel] [QEMU-PPC] [RFC 2/3] hw/spapr/spapr_caps: Add new caps safe_[cache/bounds_check/indirect_branch]

[Suraj Jitindar Singh]

On Tue, 2018-01-09 at 10:02 -0200, joserz@linux.vnet.ibm.com wrote:
> On Tue, Jan 09, 2018 at 08:21:02PM +1100, Suraj Jitindar Singh wrote:
> > This patch adds three new capabilities:
> > cap-cfpc -> safe_cache
> > cap-sbbc -> safe_bounds_check
> > cap-ibs  -> safe_indirect_branch
> > 
> > Each capability is tristate with the possible values "broken",
> > "workaround" or "fixed". Add generic getter and setter functions
> > for
> > this new capability type. Add these new capabilities to the
> > capabilities
> > list. The maximum value for the capabilities is queried from kvm
> > through
> > new kvm capabilities. The requested values are considered to be
> > compatible if kvm can support an equal or higher value for each
> > capability.
> > 
> > Discussion:
> > Currently these new capabilities default to broken to allow for
> > backwards compatibility, is this the best option?
> > ---
> >  hw/ppc/spapr.c            |   6 ++
> >  hw/ppc/spapr_caps.c       | 224
> > ++++++++++++++++++++++++++++++++++++++++++++++
> >  include/hw/ppc/spapr.h    |  15 +++-
> >  linux-headers/linux/kvm.h |   3 +
> >  target/ppc/kvm.c          |  28 ++++++
> >  target/ppc/kvm_ppc.h      |  18 ++++
> >  6 files changed, 293 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 7fa45729ba..d9700b0254 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -1765,6 +1765,9 @@ static const VMStateDescription vmstate_spapr
> > = {
> >          &vmstate_spapr_cap_htm,
> >          &vmstate_spapr_cap_vsx,
> >          &vmstate_spapr_cap_dfp,
> > +        &vmstate_spapr_cap_cfpc,
> > +        &vmstate_spapr_cap_sbbc,
> > +        &vmstate_spapr_cap_ibs,
> >          NULL
> >      }
> >  };
> > @@ -3837,6 +3840,9 @@ static void
> > spapr_machine_class_init(ObjectClass *oc, void *data)
> >      smc->default_caps.caps[SPAPR_CAP_HTM] = SPAPR_CAP_OFF;
> >      smc->default_caps.caps[SPAPR_CAP_VSX] = SPAPR_CAP_ON;
> >      smc->default_caps.caps[SPAPR_CAP_DFP] = SPAPR_CAP_ON;
> > +    smc->default_caps.caps[SPAPR_CAP_CFPC] = SPAPR_CAP_BROKEN;
> > +    smc->default_caps.caps[SPAPR_CAP_SBBC] = SPAPR_CAP_BROKEN;
> > +    smc->default_caps.caps[SPAPR_CAP_IBS] = SPAPR_CAP_BROKEN;
> >      spapr_caps_add_properties(smc, &error_abort);
> >  }
> > 
> > diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
> > index af40f2e469..e6910aa191 100644
> > --- a/hw/ppc/spapr_caps.c
> > +++ b/hw/ppc/spapr_caps.c
> > @@ -75,6 +75,64 @@ static void spapr_cap_set_bool(Object *obj,
> > Visitor *v, const char *name,
> >                                               SPAPR_CAP_CMD_LINE;
> >  }
> > 
> > +static void spapr_cap_get_tristate(Object *obj, Visitor *v, const
> > char *name,
> > +                                   void *opaque, Error **errp)
> > +{
> > +    sPAPRCapabilityInfo *cap = opaque;
> > +    sPAPRMachineState *spapr = SPAPR_MACHINE(obj);
> > +    char *val = NULL;
> > +    uint8_t value = spapr_get_cap(spapr, cap->index);
> > +
> > +    switch (value) {
> > +    case SPAPR_CAP_BROKEN:
> > +        val = g_strdup("broken");
> > +        break;
> > +    case SPAPR_CAP_WORKAROUND:
> > +        val = g_strdup("workaround");
> > +        break;
> > +    case SPAPR_CAP_FIXED:
> > +        val = g_strdup("fixed");
> > +        break;
> > +    default:
> > +        break;
> 
> Hello Suraj
> 
> Is default a possible case? If so, will val be handled correctly by
> visit_type_str and g_free?

The default case would represent an internal error somehow and should
throw an error. Will change that.

Thanks :)

> 
> if not, maybe a throwing an error like you did in
> spapr_cap_set_tristate could be better.
> 
> > +    }
> > +
> > +    visit_type_str(v, name, &val, errp);
> > +    g_free(val);
> > +}
> > +
> > +static void spapr_cap_set_tristate(Object *obj, Visitor *v, const
> > char *name,
> > +                                   void *opaque, Error **errp)
> > +{
> > +    sPAPRCapabilityInfo *cap = opaque;
> > +    sPAPRMachineState *spapr = SPAPR_MACHINE(obj);
> > +    char *val;
> > +    Error *local_err = NULL;
> > +    uint8_t value;
> > +
> > +    visit_type_str(v, name, &val, &local_err);
> > +    if (local_err) {
> > +        error_propagate(errp, local_err);
> > +        return;
> > +    }
> > +
> > +    if (!strcasecmp(val, "broken")) {
> > +        value = SPAPR_CAP_BROKEN;
> > +    } else if (!strcasecmp(val, "workaround")) {
> > +        value = SPAPR_CAP_WORKAROUND;
> > +    } else if (!strcasecmp(val, "fixed")) {
> > +        value = SPAPR_CAP_FIXED;
> > +    } else {
> > +        error_setg(errp, "Invalid capability mode \"%s\" for cap-
> > %s", val,
> > +                   cap->name);
> > +        goto out;
> > +    }
> > +
> > +    spapr->cmd_line_caps.caps[cap->index] = value |
> > SPAPR_CAP_CMD_LINE;
> > +out:
> > +    g_free(val);
> > +}
> > +
> >  static void cap_htm_allow(sPAPRMachineState *spapr, uint8_t val,
> > Error **errp)
> >  {
> >      if (!val) {
> > @@ -122,6 +180,31 @@ static void cap_dfp_allow(sPAPRMachineState
> > *spapr, uint8_t val, Error **errp)
> >      }
> >  }
> > 
> > +static void cap_safe_cache_allow(sPAPRMachineState *spapr, uint8_t
> > val,
> > +                                 Error **errp)
> > +{
> > +    if (kvm_enabled() && (val > kvmppc_get_cap_safe_cache())) {
> > +        error_setg(errp, "Requested safe cache capability level
> > not supported by kvm, try a different value for cap-cfpc");
> > +    }
> > +}
> > +
> > +static void cap_safe_bounds_check_allow(sPAPRMachineState *spapr,
> > uint8_t val,
> > +                                        Error **errp)
> > +{
> > +    if (kvm_enabled() && (val >
> > kvmppc_get_cap_safe_bounds_check())) {
> > +        error_setg(errp, "Requested safe bounds check capability
> > level not supported by kvm, try a different value for cap-sbbc");
> > +    }
> > +}
> > +
> > +static void cap_safe_indirect_branch_allow(sPAPRMachineState
> > *spapr,
> > +                                           uint8_t val, Error
> > **errp)
> > +{
> > +    if (kvm_enabled() && (val >
> > kvmppc_get_cap_safe_indirect_branch())) {
> > +        error_setg(errp, "Requested safe indirect branch
> > capability level not supported by kvm, try a different value for
> > cap-ibs");
> > +    }
> > +}
> > +
> > +#define VALUE_DESC_TRISTATE     " (broken, workaround, fixed)"
> > 
> >  sPAPRCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
> >      [SPAPR_CAP_HTM] = {
> > @@ -154,6 +237,36 @@ sPAPRCapabilityInfo
> > capability_table[SPAPR_CAP_NUM] = {
> >          .type = "bool",
> >          .allow = cap_dfp_allow,
> >      },
> > +    [SPAPR_CAP_CFPC] = {
> > +        .name = "cfpc",
> > +        .description = "Cache Flush on Privilege Change",
> > +        .options = VALUE_DESC_TRISTATE,
> > +        .index = SPAPR_CAP_CFPC,
> > +        .get = spapr_cap_get_tristate,
> > +        .set = spapr_cap_set_tristate,
> > +        .type = "string",
> > +        .allow = cap_safe_cache_allow,
> > +    },
> > +    [SPAPR_CAP_SBBC] = {
> > +        .name = "sbbc",
> > +        .description = "Speculation Barrier Bounds Checking",
> > +        .options = VALUE_DESC_TRISTATE,
> > +        .index = SPAPR_CAP_SBBC,
> > +        .get = spapr_cap_get_tristate,
> > +        .set = spapr_cap_set_tristate,
> > +        .type = "string",
> > +        .allow = cap_safe_bounds_check_allow,
> > +    },
> > +    [SPAPR_CAP_IBS] = {
> > +        .name = "ibs",
> > +        .description = "Indirect Branch Serialisation",
> > +        .options = VALUE_DESC_TRISTATE,
> > +        .index = SPAPR_CAP_IBS,
> > +        .get = spapr_cap_get_tristate,
> > +        .set = spapr_cap_set_tristate,
> > +        .type = "string",
> > +        .allow = cap_safe_indirect_branch_allow,
> > +    },
> >  };
> > 
> >  static sPAPRCapabilities default_caps_with_cpu(sPAPRMachineState
> > *spapr,
> > @@ -326,6 +439,117 @@ const VMStateDescription
> > vmstate_spapr_cap_dfp = {
> >      },
> >  };
> > 
> > +static bool spapr_cap_safe_cache_needed(void *opaque)
> > +{
> > +    sPAPRMachineState *spapr = opaque;
> > +
> > +    return !!(spapr->cmd_line_caps.caps[SPAPR_CAP_CFPC] &
> > SPAPR_CAP_CMD_LINE);
> > +}
> > +
> > +static int spapr_cap_safe_cache_pre_save(void *opaque)
> > +{
> > +    sPAPRMachineState *spapr = opaque;
> > +
> > +    spapr->mig_caps.caps[SPAPR_CAP_CFPC] =
> > +        spapr->cmd_line_caps.caps[SPAPR_CAP_CFPC];
> > +    return 0;
> > +}
> > +
> > +static int spapr_cap_safe_cache_pre_load(void *opaque)
> > +{
> > +    sPAPRMachineState *spapr = opaque;
> > +
> > +    spapr->mig_caps.caps[SPAPR_CAP_CFPC] = 0;
> > +    return 0;
> > +}
> > +
> > +const VMStateDescription vmstate_spapr_cap_cfpc = {
> > +    .name = "spapr/cap_cfpc",
> > +    .version_id = 1,
> > +    .minimum_version_id = 1,
> > +    .needed = spapr_cap_safe_cache_needed,
> > +    .pre_save = spapr_cap_safe_cache_pre_save,
> > +    .pre_load = spapr_cap_safe_cache_pre_load,
> > +    .fields = (VMStateField[]) {
> > +        VMSTATE_UINT8(mig_caps.caps[SPAPR_CAP_CFPC],
> > sPAPRMachineState),
> > +        VMSTATE_END_OF_LIST()
> > +    },
> > +};
> > +
> > +static bool spapr_cap_safe_bounds_check_needed(void *opaque)
> > +{
> > +    sPAPRMachineState *spapr = opaque;
> > +
> > +    return !!(spapr->cmd_line_caps.caps[SPAPR_CAP_SBBC] &
> > SPAPR_CAP_CMD_LINE);
> > +}
> > +
> > +static int spapr_cap_safe_bounds_check_pre_save(void *opaque)
> > +{
> > +    sPAPRMachineState *spapr = opaque;
> > +
> > +    spapr->mig_caps.caps[SPAPR_CAP_SBBC] =
> > +        spapr->cmd_line_caps.caps[SPAPR_CAP_SBBC];
> > +    return 0;
> > +}
> > +
> > +static int spapr_cap_safe_bounds_check_pre_load(void *opaque)
> > +{
> > +    sPAPRMachineState *spapr = opaque;
> > +
> > +    spapr->mig_caps.caps[SPAPR_CAP_SBBC] = 0;
> > +    return 0;
> > +}
> > +
> > +const VMStateDescription vmstate_spapr_cap_sbbc = {
> > +    .name = "spapr/cap_sbbc",
> > +    .version_id = 1,
> > +    .minimum_version_id = 1,
> > +    .needed = spapr_cap_safe_bounds_check_needed,
> > +    .pre_save = spapr_cap_safe_bounds_check_pre_save,
> > +    .pre_load = spapr_cap_safe_bounds_check_pre_load,
> > +    .fields = (VMStateField[]) {
> > +        VMSTATE_UINT8(mig_caps.caps[SPAPR_CAP_SBBC],
> > sPAPRMachineState),
> > +        VMSTATE_END_OF_LIST()
> > +    },
> > +};
> > +
> > +static bool spapr_cap_safe_indirect_branch_needed(void *opaque)
> > +{
> > +    sPAPRMachineState *spapr = opaque;
> > +
> > +    return !!(spapr->cmd_line_caps.caps[SPAPR_CAP_IBS] &
> > SPAPR_CAP_CMD_LINE);
> > +}
> > +
> > +static int spapr_cap_safe_indirect_branch_pre_save(void *opaque)
> > +{
> > +    sPAPRMachineState *spapr = opaque;
> > +
> > +    spapr->mig_caps.caps[SPAPR_CAP_IBS] =
> > +        spapr->cmd_line_caps.caps[SPAPR_CAP_IBS];
> > +    return 0;
> > +}
> > +
> > +static int spapr_cap_safe_indirect_branch_pre_load(void *opaque)
> > +{
> > +    sPAPRMachineState *spapr = opaque;
> > +
> > +    spapr->mig_caps.caps[SPAPR_CAP_IBS] = 0;
> > +    return 0;
> > +}
> > +
> > +const VMStateDescription vmstate_spapr_cap_ibs = {
> > +    .name = "spapr/cap_ibs",
> > +    .version_id = 1,
> > +    .minimum_version_id = 1,
> > +    .needed = spapr_cap_safe_indirect_branch_needed,
> > +    .pre_save = spapr_cap_safe_indirect_branch_pre_save,
> > +    .pre_load = spapr_cap_safe_indirect_branch_pre_load,
> > +    .fields = (VMStateField[]) {
> > +        VMSTATE_UINT8(mig_caps.caps[SPAPR_CAP_IBS],
> > sPAPRMachineState),
> > +        VMSTATE_END_OF_LIST()
> > +    },
> > +};
> > +
> >  void spapr_caps_reset(sPAPRMachineState *spapr)
> >  {
> >      sPAPRCapabilities caps;
> > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> > index 2804fbbf12..2db2f3e2e2 100644
> > --- a/include/hw/ppc/spapr.h
> > +++ b/include/hw/ppc/spapr.h
> > @@ -62,8 +62,14 @@ typedef enum {
> >  #define SPAPR_CAP_VSX                   0x01
> >  /* Decimal Floating Point */
> >  #define SPAPR_CAP_DFP                   0x02
> > +/* Cache Flush on Privilege Change */
> > +#define SPAPR_CAP_CFPC                  0x03
> > +/* Speculation Barrier Bounds Checking */
> > +#define SPAPR_CAP_SBBC                  0x04
> > +/* Indirect Branch Serialisation */
> > +#define SPAPR_CAP_IBS                   0x05
> >  /* Num Caps */
> > -#define SPAPR_CAP_NUM                   (SPAPR_CAP_DFP + 1)
> > +#define SPAPR_CAP_NUM                   (SPAPR_CAP_IBS + 1)
> > 
> >  /*
> >   * Capability Values
> > @@ -73,6 +79,10 @@ typedef enum {
> >  /* Bool Caps */
> >  #define SPAPR_CAP_OFF                   0x00
> >  #define SPAPR_CAP_ON                    0x01
> > +/* Broken | Workaround | Fixed Caps */
> > +#define SPAPR_CAP_BROKEN                0x00
> > +#define SPAPR_CAP_WORKAROUND            0x01
> > +#define SPAPR_CAP_FIXED                 0x02
> > 
> >  typedef struct sPAPRCapabilities sPAPRCapabilities;
> >  struct sPAPRCapabilities {
> > @@ -763,6 +773,9 @@ qemu_irq spapr_qirq(sPAPRMachineState *spapr,
> > int irq);
> >  extern const VMStateDescription vmstate_spapr_cap_htm;
> >  extern const VMStateDescription vmstate_spapr_cap_vsx;
> >  extern const VMStateDescription vmstate_spapr_cap_dfp;
> > +extern const VMStateDescription vmstate_spapr_cap_cfpc;
> > +extern const VMStateDescription vmstate_spapr_cap_sbbc;
> > +extern const VMStateDescription vmstate_spapr_cap_ibs;
> > 
> >  static inline uint8_t spapr_get_cap(sPAPRMachineState *spapr, int
> > cap)
> >  {
> > diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h
> > index ce6c2f11f4..0abe0a0abb 100644
> > --- a/linux-headers/linux/kvm.h
> > +++ b/linux-headers/linux/kvm.h
> > @@ -932,6 +932,9 @@ struct kvm_ppc_resize_hpt {
> >  #define KVM_CAP_HYPERV_SYNIC2 148
> >  #define KVM_CAP_HYPERV_VP_INDEX 149
> >  #define KVM_CAP_S390_AIS_MIGRATION 150
> > +#define KVM_CAP_PPC_SAFE_CACHE 151
> > +#define KVM_CAP_PPC_SAFE_BOUNDS_CHECK 152
> > +#define KVM_CAP_PPC_SAFE_INDIRECT_BRANCH 153
> > 
> >  #ifdef KVM_CAP_IRQ_ROUTING
> > 
> > diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> > index 518dd06e98..818499237c 100644
> > --- a/target/ppc/kvm.c
> > +++ b/target/ppc/kvm.c
> > @@ -89,6 +89,9 @@ static int cap_mmu_radix;
> >  static int cap_mmu_hash_v3;
> >  static int cap_resize_hpt;
> >  static int cap_ppc_pvr_compat;
> > +static int cap_ppc_safe_cache;
> > +static int cap_ppc_safe_bounds_check;
> > +static int cap_ppc_safe_indirect_branch;
> > 
> >  static uint32_t debug_inst_opcode;
> > 
> > @@ -147,6 +150,16 @@ int kvm_arch_init(MachineState *ms, KVMState
> > *s)
> >      cap_mmu_radix = kvm_vm_check_extension(s,
> > KVM_CAP_PPC_MMU_RADIX);
> >      cap_mmu_hash_v3 = kvm_vm_check_extension(s,
> > KVM_CAP_PPC_MMU_HASH_V3);
> >      cap_resize_hpt = kvm_vm_check_extension(s,
> > KVM_CAP_SPAPR_RESIZE_HPT);
> > +    cap_ppc_safe_cache = kvm_vm_check_extension(s,
> > KVM_CAP_PPC_SAFE_CACHE);
> > +    cap_ppc_safe_cache = cap_ppc_safe_cache > 0 ?
> > cap_ppc_safe_cache : 0;
> > +    cap_ppc_safe_bounds_check = kvm_vm_check_extension(s,
> > +                                KVM_CAP_PPC_SAFE_BOUNDS_CHECK);
> > +    cap_ppc_safe_bounds_check = cap_ppc_safe_bounds_check > 0 ?
> > +                                cap_ppc_safe_bounds_check : 0;
> > +    cap_ppc_safe_indirect_branch = kvm_vm_check_extension(s,
> > +                                   KVM_CAP_PPC_SAFE_INDIRECT_BRANC
> > H);
> > +    cap_ppc_safe_indirect_branch = cap_ppc_safe_indirect_branch >
> > 0 ?
> > +                                   cap_ppc_safe_indirect_branch :
> > 0;
> >      /*
> >       * Note: setting it to false because there is not such
> > capability
> >       * in KVM at this moment.
> > @@ -2456,6 +2469,21 @@ bool kvmppc_has_cap_mmu_hash_v3(void)
> >      return cap_mmu_hash_v3;
> >  }
> > 
> > +int kvmppc_get_cap_safe_cache(void)
> > +{
> > +    return cap_ppc_safe_cache;
> > +}
> > +
> > +int kvmppc_get_cap_safe_bounds_check(void)
> > +{
> > +    return cap_ppc_safe_bounds_check;
> > +}
> > +
> > +int kvmppc_get_cap_safe_indirect_branch(void)
> > +{
> > +    return cap_ppc_safe_indirect_branch;
> > +}
> > +
> >  PowerPCCPUClass *kvm_ppc_get_host_cpu_class(void)
> >  {
> >      uint32_t host_pvr = mfpvr();
> > diff --git a/target/ppc/kvm_ppc.h b/target/ppc/kvm_ppc.h
> > index ecb55493cc..39830baa77 100644
> > --- a/target/ppc/kvm_ppc.h
> > +++ b/target/ppc/kvm_ppc.h
> > @@ -59,6 +59,9 @@ bool kvmppc_has_cap_fixup_hcalls(void);
> >  bool kvmppc_has_cap_htm(void);
> >  bool kvmppc_has_cap_mmu_radix(void);
> >  bool kvmppc_has_cap_mmu_hash_v3(void);
> > +int kvmppc_get_cap_safe_cache(void);
> > +int kvmppc_get_cap_safe_bounds_check(void);
> > +int kvmppc_get_cap_safe_indirect_branch(void);
> >  int kvmppc_enable_hwrng(void);
> >  int kvmppc_put_books_sregs(PowerPCCPU *cpu);
> >  PowerPCCPUClass *kvm_ppc_get_host_cpu_class(void);
> > @@ -290,6 +293,21 @@ static inline bool
> > kvmppc_has_cap_mmu_hash_v3(void)
> >      return false;
> >  }
> > 
> > +static inline int kvmppc_get_cap_safe_cache(void)
> > +{
> > +    return 0;
> > +}
> > +
> > +static inline int kvmppc_get_cap_safe_bounds_check(void)
> > +{
> > +    return 0;
> > +}
> > +
> > +static inline int kvmppc_get_cap_safe_indirect_branch(void)
> > +{
> > +    return 0;
> > +}
> > +
> >  static inline int kvmppc_enable_hwrng(void)
> >  {
> >      return -1;
> > -- 
> > 2.13.6
> > 
> > 
> 
> 

Re: [Qemu-devel] [QEMU-PPC] [RFC 2/3] hw/spapr/spapr_caps: Add new caps safe_[cache/bounds_check/indirect_branch]

[David Gibson]

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

On Tue, Jan 09, 2018 at 08:21:02PM +1100, Suraj Jitindar Singh wrote:
> This patch adds three new capabilities:
> cap-cfpc -> safe_cache
> cap-sbbc -> safe_bounds_check
> cap-ibs  -> safe_indirect_branch
> 
> Each capability is tristate with the possible values "broken",
> "workaround" or "fixed". Add generic getter and setter functions for
> this new capability type. Add these new capabilities to the capabilities
> list. The maximum value for the capabilities is queried from kvm through
> new kvm capabilities. The requested values are considered to be
> compatible if kvm can support an equal or higher value for each
> capability.
> 
> Discussion:
> Currently these new capabilities default to broken to allow for
> backwards compatibility, is this the best option?
> ---
>  hw/ppc/spapr.c            |   6 ++
>  hw/ppc/spapr_caps.c       | 224 ++++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/ppc/spapr.h    |  15 +++-
>  linux-headers/linux/kvm.h |   3 +
>  target/ppc/kvm.c          |  28 ++++++
>  target/ppc/kvm_ppc.h      |  18 ++++
>  6 files changed, 293 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 7fa45729ba..d9700b0254 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1765,6 +1765,9 @@ static const VMStateDescription vmstate_spapr = {
>          &vmstate_spapr_cap_htm,
>          &vmstate_spapr_cap_vsx,
>          &vmstate_spapr_cap_dfp,
> +        &vmstate_spapr_cap_cfpc,
> +        &vmstate_spapr_cap_sbbc,
> +        &vmstate_spapr_cap_ibs,
>          NULL
>      }
>  };
> @@ -3837,6 +3840,9 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
>      smc->default_caps.caps[SPAPR_CAP_HTM] = SPAPR_CAP_OFF;
>      smc->default_caps.caps[SPAPR_CAP_VSX] = SPAPR_CAP_ON;
>      smc->default_caps.caps[SPAPR_CAP_DFP] = SPAPR_CAP_ON;
> +    smc->default_caps.caps[SPAPR_CAP_CFPC] = SPAPR_CAP_BROKEN;
> +    smc->default_caps.caps[SPAPR_CAP_SBBC] = SPAPR_CAP_BROKEN;
> +    smc->default_caps.caps[SPAPR_CAP_IBS] = SPAPR_CAP_BROKEN;
>      spapr_caps_add_properties(smc, &error_abort);
>  }
>  
> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
> index af40f2e469..e6910aa191 100644
> --- a/hw/ppc/spapr_caps.c
> +++ b/hw/ppc/spapr_caps.c
> @@ -75,6 +75,64 @@ static void spapr_cap_set_bool(Object *obj, Visitor *v, const char *name,
>                                               SPAPR_CAP_CMD_LINE;
>  }
>  
> +static void spapr_cap_get_tristate(Object *obj, Visitor *v, const char *name,
> +                                   void *opaque, Error **errp)
> +{
> +    sPAPRCapabilityInfo *cap = opaque;
> +    sPAPRMachineState *spapr = SPAPR_MACHINE(obj);
> +    char *val = NULL;
> +    uint8_t value = spapr_get_cap(spapr, cap->index);
> +
> +    switch (value) {
> +    case SPAPR_CAP_BROKEN:
> +        val = g_strdup("broken");
> +        break;
> +    case SPAPR_CAP_WORKAROUND:
> +        val = g_strdup("workaround");
> +        break;
> +    case SPAPR_CAP_FIXED:
> +        val = g_strdup("fixed");
> +        break;
> +    default:
> +        break;
> +    }
> +
> +    visit_type_str(v, name, &val, errp);
> +    g_free(val);
> +}
> +
> +static void spapr_cap_set_tristate(Object *obj, Visitor *v, const char *name,
> +                                   void *opaque, Error **errp)
> +{
> +    sPAPRCapabilityInfo *cap = opaque;
> +    sPAPRMachineState *spapr = SPAPR_MACHINE(obj);
> +    char *val;
> +    Error *local_err = NULL;
> +    uint8_t value;
> +
> +    visit_type_str(v, name, &val, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +
> +    if (!strcasecmp(val, "broken")) {
> +        value = SPAPR_CAP_BROKEN;
> +    } else if (!strcasecmp(val, "workaround")) {
> +        value = SPAPR_CAP_WORKAROUND;
> +    } else if (!strcasecmp(val, "fixed")) {
> +        value = SPAPR_CAP_FIXED;
> +    } else {
> +        error_setg(errp, "Invalid capability mode \"%s\" for cap-%s", val,
> +                   cap->name);
> +        goto out;
> +    }
> +
> +    spapr->cmd_line_caps.caps[cap->index] = value | SPAPR_CAP_CMD_LINE;
> +out:
> +    g_free(val);
> +}
> +
>  static void cap_htm_allow(sPAPRMachineState *spapr, uint8_t val, Error **errp)
>  {
>      if (!val) {
> @@ -122,6 +180,31 @@ static void cap_dfp_allow(sPAPRMachineState *spapr, uint8_t val, Error **errp)
>      }
>  }
>  
> +static void cap_safe_cache_allow(sPAPRMachineState *spapr, uint8_t val,
> +                                 Error **errp)
> +{
> +    if (kvm_enabled() && (val > kvmppc_get_cap_safe_cache())) {
> +        error_setg(errp, "Requested safe cache capability level not supported by kvm, try a different value for cap-cfpc");
> +    }

Maybe throw in a FIXME comment for TCG.  Given the bugs can be
triggered via Javascript, there's a good chance they can be triggered
via TCG as well - but obviously we don't want to delay the KVM fixes
to work out what the TCG situation is.

> +}
> +
> +static void cap_safe_bounds_check_allow(sPAPRMachineState *spapr, uint8_t val,
> +                                        Error **errp)
> +{
> +    if (kvm_enabled() && (val > kvmppc_get_cap_safe_bounds_check())) {
> +        error_setg(errp, "Requested safe bounds check capability level not supported by kvm, try a different value for cap-sbbc");
> +    }
> +}
> +
> +static void cap_safe_indirect_branch_allow(sPAPRMachineState *spapr,
> +                                           uint8_t val, Error **errp)
> +{
> +    if (kvm_enabled() && (val > kvmppc_get_cap_safe_indirect_branch())) {
> +        error_setg(errp, "Requested safe indirect branch capability level not supported by kvm, try a different value for cap-ibs");
> +    }
> +}
> +
> +#define VALUE_DESC_TRISTATE     " (broken, workaround, fixed)"
>  
>  sPAPRCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
>      [SPAPR_CAP_HTM] = {
> @@ -154,6 +237,36 @@ sPAPRCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
>          .type = "bool",
>          .allow = cap_dfp_allow,
>      },
> +    [SPAPR_CAP_CFPC] = {
> +        .name = "cfpc",
> +        .description = "Cache Flush on Privilege Change",
> +        .options = VALUE_DESC_TRISTATE,
> +        .index = SPAPR_CAP_CFPC,
> +        .get = spapr_cap_get_tristate,
> +        .set = spapr_cap_set_tristate,
> +        .type = "string",
> +        .allow = cap_safe_cache_allow,
> +    },
> +    [SPAPR_CAP_SBBC] = {
> +        .name = "sbbc",
> +        .description = "Speculation Barrier Bounds Checking",
> +        .options = VALUE_DESC_TRISTATE,
> +        .index = SPAPR_CAP_SBBC,
> +        .get = spapr_cap_get_tristate,
> +        .set = spapr_cap_set_tristate,
> +        .type = "string",
> +        .allow = cap_safe_bounds_check_allow,
> +    },
> +    [SPAPR_CAP_IBS] = {
> +        .name = "ibs",
> +        .description = "Indirect Branch Serialisation",
> +        .options = VALUE_DESC_TRISTATE,
> +        .index = SPAPR_CAP_IBS,
> +        .get = spapr_cap_get_tristate,
> +        .set = spapr_cap_set_tristate,
> +        .type = "string",
> +        .allow = cap_safe_indirect_branch_allow,
> +    },
>  };
>  
>  static sPAPRCapabilities default_caps_with_cpu(sPAPRMachineState *spapr,
> @@ -326,6 +439,117 @@ const VMStateDescription vmstate_spapr_cap_dfp = {
>      },
>  };
>  
> +static bool spapr_cap_safe_cache_needed(void *opaque)
> +{
> +    sPAPRMachineState *spapr = opaque;
> +
> +    return !!(spapr->cmd_line_caps.caps[SPAPR_CAP_CFPC] & SPAPR_CAP_CMD_LINE);
> +}
> +
> +static int spapr_cap_safe_cache_pre_save(void *opaque)
> +{
> +    sPAPRMachineState *spapr = opaque;
> +
> +    spapr->mig_caps.caps[SPAPR_CAP_CFPC] =
> +        spapr->cmd_line_caps.caps[SPAPR_CAP_CFPC];
> +    return 0;
> +}
> +
> +static int spapr_cap_safe_cache_pre_load(void *opaque)
> +{
> +    sPAPRMachineState *spapr = opaque;
> +
> +    spapr->mig_caps.caps[SPAPR_CAP_CFPC] = 0;
> +    return 0;
> +}
> +
> +const VMStateDescription vmstate_spapr_cap_cfpc = {
> +    .name = "spapr/cap_cfpc",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .needed = spapr_cap_safe_cache_needed,
> +    .pre_save = spapr_cap_safe_cache_pre_save,
> +    .pre_load = spapr_cap_safe_cache_pre_load,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT8(mig_caps.caps[SPAPR_CAP_CFPC], sPAPRMachineState),
> +        VMSTATE_END_OF_LIST()
> +    },
> +};
> +
> +static bool spapr_cap_safe_bounds_check_needed(void *opaque)
> +{
> +    sPAPRMachineState *spapr = opaque;
> +
> +    return !!(spapr->cmd_line_caps.caps[SPAPR_CAP_SBBC] & SPAPR_CAP_CMD_LINE);
> +}
> +
> +static int spapr_cap_safe_bounds_check_pre_save(void *opaque)
> +{
> +    sPAPRMachineState *spapr = opaque;
> +
> +    spapr->mig_caps.caps[SPAPR_CAP_SBBC] =
> +        spapr->cmd_line_caps.caps[SPAPR_CAP_SBBC];
> +    return 0;
> +}
> +
> +static int spapr_cap_safe_bounds_check_pre_load(void *opaque)
> +{
> +    sPAPRMachineState *spapr = opaque;
> +
> +    spapr->mig_caps.caps[SPAPR_CAP_SBBC] = 0;
> +    return 0;
> +}
> +
> +const VMStateDescription vmstate_spapr_cap_sbbc = {
> +    .name = "spapr/cap_sbbc",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .needed = spapr_cap_safe_bounds_check_needed,
> +    .pre_save = spapr_cap_safe_bounds_check_pre_save,
> +    .pre_load = spapr_cap_safe_bounds_check_pre_load,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT8(mig_caps.caps[SPAPR_CAP_SBBC], sPAPRMachineState),
> +        VMSTATE_END_OF_LIST()
> +    },
> +};
> +
> +static bool spapr_cap_safe_indirect_branch_needed(void *opaque)
> +{
> +    sPAPRMachineState *spapr = opaque;
> +
> +    return !!(spapr->cmd_line_caps.caps[SPAPR_CAP_IBS] & SPAPR_CAP_CMD_LINE);
> +}
> +
> +static int spapr_cap_safe_indirect_branch_pre_save(void *opaque)
> +{
> +    sPAPRMachineState *spapr = opaque;
> +
> +    spapr->mig_caps.caps[SPAPR_CAP_IBS] =
> +        spapr->cmd_line_caps.caps[SPAPR_CAP_IBS];
> +    return 0;
> +}
> +
> +static int spapr_cap_safe_indirect_branch_pre_load(void *opaque)
> +{
> +    sPAPRMachineState *spapr = opaque;
> +
> +    spapr->mig_caps.caps[SPAPR_CAP_IBS] = 0;
> +    return 0;
> +}
> +
> +const VMStateDescription vmstate_spapr_cap_ibs = {
> +    .name = "spapr/cap_ibs",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .needed = spapr_cap_safe_indirect_branch_needed,
> +    .pre_save = spapr_cap_safe_indirect_branch_pre_save,
> +    .pre_load = spapr_cap_safe_indirect_branch_pre_load,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT8(mig_caps.caps[SPAPR_CAP_IBS], sPAPRMachineState),
> +        VMSTATE_END_OF_LIST()
> +    },
> +};
> +
>  void spapr_caps_reset(sPAPRMachineState *spapr)
>  {
>      sPAPRCapabilities caps;
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 2804fbbf12..2db2f3e2e2 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -62,8 +62,14 @@ typedef enum {
>  #define SPAPR_CAP_VSX                   0x01
>  /* Decimal Floating Point */
>  #define SPAPR_CAP_DFP                   0x02
> +/* Cache Flush on Privilege Change */
> +#define SPAPR_CAP_CFPC                  0x03
> +/* Speculation Barrier Bounds Checking */
> +#define SPAPR_CAP_SBBC                  0x04
> +/* Indirect Branch Serialisation */
> +#define SPAPR_CAP_IBS                   0x05
>  /* Num Caps */
> -#define SPAPR_CAP_NUM                   (SPAPR_CAP_DFP + 1)
> +#define SPAPR_CAP_NUM                   (SPAPR_CAP_IBS + 1)
>  
>  /*
>   * Capability Values
> @@ -73,6 +79,10 @@ typedef enum {
>  /* Bool Caps */
>  #define SPAPR_CAP_OFF                   0x00
>  #define SPAPR_CAP_ON                    0x01
> +/* Broken | Workaround | Fixed Caps */
> +#define SPAPR_CAP_BROKEN                0x00
> +#define SPAPR_CAP_WORKAROUND            0x01
> +#define SPAPR_CAP_FIXED                 0x02
>  
>  typedef struct sPAPRCapabilities sPAPRCapabilities;
>  struct sPAPRCapabilities {
> @@ -763,6 +773,9 @@ qemu_irq spapr_qirq(sPAPRMachineState *spapr, int irq);
>  extern const VMStateDescription vmstate_spapr_cap_htm;
>  extern const VMStateDescription vmstate_spapr_cap_vsx;
>  extern const VMStateDescription vmstate_spapr_cap_dfp;
> +extern const VMStateDescription vmstate_spapr_cap_cfpc;
> +extern const VMStateDescription vmstate_spapr_cap_sbbc;
> +extern const VMStateDescription vmstate_spapr_cap_ibs;
>  
>  static inline uint8_t spapr_get_cap(sPAPRMachineState *spapr, int cap)
>  {
> diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h
> index ce6c2f11f4..0abe0a0abb 100644
> --- a/linux-headers/linux/kvm.h
> +++ b/linux-headers/linux/kvm.h
> @@ -932,6 +932,9 @@ struct kvm_ppc_resize_hpt {
>  #define KVM_CAP_HYPERV_SYNIC2 148
>  #define KVM_CAP_HYPERV_VP_INDEX 149
>  #define KVM_CAP_S390_AIS_MIGRATION 150
> +#define KVM_CAP_PPC_SAFE_CACHE 151
> +#define KVM_CAP_PPC_SAFE_BOUNDS_CHECK 152
> +#define KVM_CAP_PPC_SAFE_INDIRECT_BRANCH 153
>  
>  #ifdef KVM_CAP_IRQ_ROUTING
>  
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index 518dd06e98..818499237c 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -89,6 +89,9 @@ static int cap_mmu_radix;
>  static int cap_mmu_hash_v3;
>  static int cap_resize_hpt;
>  static int cap_ppc_pvr_compat;
> +static int cap_ppc_safe_cache;
> +static int cap_ppc_safe_bounds_check;
> +static int cap_ppc_safe_indirect_branch;
>  
>  static uint32_t debug_inst_opcode;
>  
> @@ -147,6 +150,16 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
>      cap_mmu_radix = kvm_vm_check_extension(s, KVM_CAP_PPC_MMU_RADIX);
>      cap_mmu_hash_v3 = kvm_vm_check_extension(s, KVM_CAP_PPC_MMU_HASH_V3);
>      cap_resize_hpt = kvm_vm_check_extension(s, KVM_CAP_SPAPR_RESIZE_HPT);
> +    cap_ppc_safe_cache = kvm_vm_check_extension(s, KVM_CAP_PPC_SAFE_CACHE);
> +    cap_ppc_safe_cache = cap_ppc_safe_cache > 0 ? cap_ppc_safe_cache : 0;
> +    cap_ppc_safe_bounds_check = kvm_vm_check_extension(s,
> +                                KVM_CAP_PPC_SAFE_BOUNDS_CHECK);
> +    cap_ppc_safe_bounds_check = cap_ppc_safe_bounds_check > 0 ?
> +                                cap_ppc_safe_bounds_check : 0;
> +    cap_ppc_safe_indirect_branch = kvm_vm_check_extension(s,
> +                                   KVM_CAP_PPC_SAFE_INDIRECT_BRANCH);
> +    cap_ppc_safe_indirect_branch = cap_ppc_safe_indirect_branch > 0 ?
> +                                   cap_ppc_safe_indirect_branch : 0;
>      /*
>       * Note: setting it to false because there is not such capability
>       * in KVM at this moment.
> @@ -2456,6 +2469,21 @@ bool kvmppc_has_cap_mmu_hash_v3(void)
>      return cap_mmu_hash_v3;
>  }
>  
> +int kvmppc_get_cap_safe_cache(void)
> +{
> +    return cap_ppc_safe_cache;
> +}
> +
> +int kvmppc_get_cap_safe_bounds_check(void)
> +{
> +    return cap_ppc_safe_bounds_check;
> +}
> +
> +int kvmppc_get_cap_safe_indirect_branch(void)
> +{
> +    return cap_ppc_safe_indirect_branch;
> +}
> +
>  PowerPCCPUClass *kvm_ppc_get_host_cpu_class(void)
>  {
>      uint32_t host_pvr = mfpvr();
> diff --git a/target/ppc/kvm_ppc.h b/target/ppc/kvm_ppc.h
> index ecb55493cc..39830baa77 100644
> --- a/target/ppc/kvm_ppc.h
> +++ b/target/ppc/kvm_ppc.h
> @@ -59,6 +59,9 @@ bool kvmppc_has_cap_fixup_hcalls(void);
>  bool kvmppc_has_cap_htm(void);
>  bool kvmppc_has_cap_mmu_radix(void);
>  bool kvmppc_has_cap_mmu_hash_v3(void);
> +int kvmppc_get_cap_safe_cache(void);
> +int kvmppc_get_cap_safe_bounds_check(void);
> +int kvmppc_get_cap_safe_indirect_branch(void);
>  int kvmppc_enable_hwrng(void);
>  int kvmppc_put_books_sregs(PowerPCCPU *cpu);
>  PowerPCCPUClass *kvm_ppc_get_host_cpu_class(void);
> @@ -290,6 +293,21 @@ static inline bool kvmppc_has_cap_mmu_hash_v3(void)
>      return false;
>  }
>  
> +static inline int kvmppc_get_cap_safe_cache(void)
> +{
> +    return 0;
> +}
> +
> +static inline int kvmppc_get_cap_safe_bounds_check(void)
> +{
> +    return 0;
> +}
> +
> +static inline int kvmppc_get_cap_safe_indirect_branch(void)
> +{
> +    return 0;
> +}
> +
>  static inline int kvmppc_enable_hwrng(void)
>  {
>      return -1;

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

[Qemu-devel] [QEMU-PPC] [RFC 3/3] target/ppc: Add H-Call H_GET_CPU_CHARACTERISTICS

[Suraj Jitindar Singh]

The new H-Call H_GET_CPU_CHARACTERISTICS is used by the guest to query
behaviours and available characteristics of the cpu.

Implement the handler for this new H-Call which formulates its response
based on the setting of the new capabilities added in the previous
patch.

Note: Currently we return H_FUNCTION under TCG which will direct the
      guest to fall back to doing a displacement flush

Discussion:
Is TCG affected?
Is there any point in telling the guest to do these workarounds on TCG
given they're unlikely to translate to host instructions which have the
desired effect?
---
 hw/ppc/spapr_hcall.c   | 81 ++++++++++++++++++++++++++++++++++++++++++++++++++
 include/hw/ppc/spapr.h |  1 +
 2 files changed, 82 insertions(+)

diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index 51eba52e86..b62b47c8d9 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -1654,6 +1654,84 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
     return H_SUCCESS;
 }
 
+#define CPU_CHARACTERISTIC_SPEC_BARRIER         (1ULL << (63 - 0))
+#define CPU_CHARACTERISTIC_BCCTR_SERIAL         (1ULL << (63 - 1))
+#define CPU_CHARACTERISTIC_ORI_L1_CACHE         (1ULL << (63 - 2))
+#define CPU_CHARACTERISTIC_MTTRIG_L1_CACHE      (1ULL << (63 - 3))
+#define CPU_CHARACTERISTIC_L1_CACHE_PRIV        (1ULL << (63 - 4))
+#define CPU_CHARACTERISTIC_BRANCH_HINTS         (1ULL << (63 - 5))
+#define CPU_CHARACTERISTIC_MTTRIG_THR_RECONF    (1ULL << (63 - 6))
+#define CPU_BEHAVIOUR_FAVOUR_SECURITY           (1ULL << (63 - 0))
+#define CPU_BEHAVIOUR_L1_CACHE_FLUSH            (1ULL << (63 - 1))
+#define CPU_BEHAVIOUR_SPEC_BARRIER              (1ULL << (63 - 2))
+
+static target_ulong h_get_cpu_characteristics(PowerPCCPU *cpu,
+                                              sPAPRMachineState *spapr,
+                                              target_ulong opcode,
+                                              target_ulong *args)
+{
+    uint64_t characteristics = CPU_CHARACTERISTIC_BRANCH_HINTS;
+    uint64_t behaviour = CPU_BEHAVIOUR_FAVOUR_SECURITY;
+    uint8_t safe_cache = spapr_get_cap(spapr, SPAPR_CAP_CFPC);
+    uint8_t safe_bounds_check = spapr_get_cap(spapr, SPAPR_CAP_SBBC);
+    uint8_t safe_indirect_branch = spapr_get_cap(spapr, SPAPR_CAP_IBS);
+
+    /* TODO: Is TCG vulnerable? */
+    if (!kvm_enabled()) {
+        return H_FUNCTION;
+    }
+
+    switch (safe_cache) {
+    case SPAPR_CAP_WORKAROUND:
+        characteristics |= CPU_CHARACTERISTIC_ORI_L1_CACHE;
+        characteristics |= CPU_CHARACTERISTIC_MTTRIG_L1_CACHE;
+        characteristics |= CPU_CHARACTERISTIC_L1_CACHE_PRIV;
+        behaviour |= CPU_BEHAVIOUR_L1_CACHE_FLUSH;
+        break;
+    case SPAPR_CAP_FIXED:
+        break;
+    default: /* broken */
+        if (safe_cache != SPAPR_CAP_BROKEN) {
+            error_report("Invalid value for KVM_CAP_PPC_SAFE_CACHE (%d), assuming broken",
+                         safe_cache);
+        }
+        behaviour |= CPU_BEHAVIOUR_L1_CACHE_FLUSH;
+        break;
+    }
+
+    switch (safe_bounds_check) {
+    case SPAPR_CAP_WORKAROUND:
+        characteristics |= CPU_CHARACTERISTIC_SPEC_BARRIER;
+        behaviour |= CPU_BEHAVIOUR_SPEC_BARRIER;
+        break;
+    case SPAPR_CAP_FIXED:
+        break;
+    default: /* broken */
+        if (safe_bounds_check != SPAPR_CAP_BROKEN) {
+            error_report("Invalid value for KVM_CAP_PPC_SAFE_BOUNDS_CHECK (%d), assuming broken",
+                         safe_bounds_check);
+        }
+        behaviour |= CPU_BEHAVIOUR_SPEC_BARRIER;
+        break;
+    }
+
+    switch (safe_indirect_branch) {
+    case SPAPR_CAP_FIXED:
+        characteristics |= CPU_CHARACTERISTIC_BCCTR_SERIAL;
+    default: /* broken */
+        if (safe_indirect_branch != SPAPR_CAP_BROKEN) {
+            error_report("Invalid value for KVM_CAP_PPC_SAFE_INDIRECT_BRANCH (%d), assuming broken",
+                         safe_indirect_branch);
+        }
+        break;
+    }
+
+    args[0] = characteristics;
+    args[1] = behaviour;
+
+    return H_SUCCESS;
+}
+
 static spapr_hcall_fn papr_hypercall_table[(MAX_HCALL_OPCODE / 4) + 1];
 static spapr_hcall_fn kvmppc_hypercall_table[KVMPPC_HCALL_MAX - KVMPPC_HCALL_BASE + 1];
 
@@ -1733,6 +1811,9 @@ static void hypercall_register_types(void)
     spapr_register_hypercall(H_INVALIDATE_PID, h_invalidate_pid);
     spapr_register_hypercall(H_REGISTER_PROC_TBL, h_register_process_table);
 
+    /* hcall-get-cpu-characteristics */
+    spapr_register_hypercall(H_GET_CPU_CHARACTERISTICS, h_get_cpu_characteristics);
+
     /* "debugger" hcalls (also used by SLOF). Note: We do -not- differenciate
      * here between the "CI" and the "CACHE" variants, they will use whatever
      * mapping attributes qemu is using. When using KVM, the kernel will
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 2db2f3e2e2..5677c38d2a 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -396,6 +396,7 @@ struct sPAPRMachineState {
 #define H_GET_HCA_INFO          0x1B8
 #define H_GET_PERF_COUNT        0x1BC
 #define H_MANAGE_TRACE          0x1C0
+#define H_GET_CPU_CHARACTERISTICS 0x1C8
 #define H_FREE_LOGICAL_LAN_BUFFER 0x1D4
 #define H_QUERY_INT_STATE       0x1E4
 #define H_POLL_PENDING          0x1D8
-- 
2.13.6

Re: [Qemu-devel] [Qemu-ppc] [QEMU-PPC] [RFC 3/3] target/ppc: Add H-Call H_GET_CPU_CHARACTERISTICS

[Murilo Opsfelder Araújo]

On 01/09/2018 07:21 AM, Suraj Jitindar Singh wrote:
> The new H-Call H_GET_CPU_CHARACTERISTICS is used by the guest to query
> behaviours and available characteristics of the cpu.
> 
> Implement the handler for this new H-Call which formulates its response
> based on the setting of the new capabilities added in the previous
> patch.
> 
> Note: Currently we return H_FUNCTION under TCG which will direct the
>       guest to fall back to doing a displacement flush
> 
> Discussion:
> Is TCG affected?
> Is there any point in telling the guest to do these workarounds on TCG
> given they're unlikely to translate to host instructions which have the
> desired effect?

Hi, Suraj.

What if this is left to the cover letter?

> ---
>  hw/ppc/spapr_hcall.c   | 81 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/ppc/spapr.h |  1 +
>  2 files changed, 82 insertions(+)
> 
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index 51eba52e86..b62b47c8d9 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -1654,6 +1654,84 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
>      return H_SUCCESS;
>  }
> 
> +#define CPU_CHARACTERISTIC_SPEC_BARRIER         (1ULL << (63 - 0))
> +#define CPU_CHARACTERISTIC_BCCTR_SERIAL         (1ULL << (63 - 1))
> +#define CPU_CHARACTERISTIC_ORI_L1_CACHE         (1ULL << (63 - 2))
> +#define CPU_CHARACTERISTIC_MTTRIG_L1_CACHE      (1ULL << (63 - 3))
> +#define CPU_CHARACTERISTIC_L1_CACHE_PRIV        (1ULL << (63 - 4))
> +#define CPU_CHARACTERISTIC_BRANCH_HINTS         (1ULL << (63 - 5))
> +#define CPU_CHARACTERISTIC_MTTRIG_THR_RECONF    (1ULL << (63 - 6))
> +#define CPU_BEHAVIOUR_FAVOUR_SECURITY           (1ULL << (63 - 0))
> +#define CPU_BEHAVIOUR_L1_CACHE_FLUSH            (1ULL << (63 - 1))
> +#define CPU_BEHAVIOUR_SPEC_BARRIER              (1ULL << (63 - 2))

Can PPC_BIT be used here?

Cheers
Murilo

Re: [Qemu-devel] [Qemu-ppc] [QEMU-PPC] [RFC 3/3] target/ppc: Add H-Call H_GET_CPU_CHARACTERISTICS

[Suraj Jitindar Singh]

On Tue, 2018-01-09 at 09:19 -0200, Murilo Opsfelder Araújo wrote:
> On 01/09/2018 07:21 AM, Suraj Jitindar Singh wrote:
> > The new H-Call H_GET_CPU_CHARACTERISTICS is used by the guest to
> > query
> > behaviours and available characteristics of the cpu.
> > 
> > Implement the handler for this new H-Call which formulates its
> > response
> > based on the setting of the new capabilities added in the previous
> > patch.
> > 
> > Note: Currently we return H_FUNCTION under TCG which will direct
> > the
> >       guest to fall back to doing a displacement flush
> > 
> > Discussion:
> > Is TCG affected?
> > Is there any point in telling the guest to do these workarounds on
> > TCG
> > given they're unlikely to translate to host instructions which have
> > the
> > desired effect?
> 
> Hi, Suraj.
> 
> What if this is left to the cover letter?

Again, only because this is RFC.

> 
> > ---
> >  hw/ppc/spapr_hcall.c   | 81
> > ++++++++++++++++++++++++++++++++++++++++++++++++++
> >  include/hw/ppc/spapr.h |  1 +
> >  2 files changed, 82 insertions(+)
> > 
> > diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> > index 51eba52e86..b62b47c8d9 100644
> > --- a/hw/ppc/spapr_hcall.c
> > +++ b/hw/ppc/spapr_hcall.c
> > @@ -1654,6 +1654,84 @@ static target_ulong
> > h_client_architecture_support(PowerPCCPU *cpu,
> >      return H_SUCCESS;
> >  }
> > 
> > +#define CPU_CHARACTERISTIC_SPEC_BARRIER         (1ULL << (63 - 0))
> > +#define CPU_CHARACTERISTIC_BCCTR_SERIAL         (1ULL << (63 - 1))
> > +#define CPU_CHARACTERISTIC_ORI_L1_CACHE         (1ULL << (63 - 2))
> > +#define CPU_CHARACTERISTIC_MTTRIG_L1_CACHE      (1ULL << (63 - 3))
> > +#define CPU_CHARACTERISTIC_L1_CACHE_PRIV        (1ULL << (63 - 4))
> > +#define CPU_CHARACTERISTIC_BRANCH_HINTS         (1ULL << (63 - 5))
> > +#define CPU_CHARACTERISTIC_MTTRIG_THR_RECONF    (1ULL << (63 - 6))
> > +#define CPU_BEHAVIOUR_FAVOUR_SECURITY           (1ULL << (63 - 0))
> > +#define CPU_BEHAVIOUR_L1_CACHE_FLUSH            (1ULL << (63 - 1))
> > +#define CPU_BEHAVIOUR_SPEC_BARRIER              (1ULL << (63 - 2))
> 
> Can PPC_BIT be used here?

Yep, will do

> 
> Cheers
> Murilo
> 

Re: [Qemu-devel] [QEMU-PPC] [RFC 3/3] target/ppc: Add H-Call H_GET_CPU_CHARACTERISTICS

[David Gibson]

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

On Tue, Jan 09, 2018 at 08:21:03PM +1100, Suraj Jitindar Singh wrote:
> The new H-Call H_GET_CPU_CHARACTERISTICS is used by the guest to query
> behaviours and available characteristics of the cpu.
> 
> Implement the handler for this new H-Call which formulates its response
> based on the setting of the new capabilities added in the previous
> patch.
> 
> Note: Currently we return H_FUNCTION under TCG which will direct the
>       guest to fall back to doing a displacement flush
> 
> Discussion:
> Is TCG affected?

Very likely :(.

> Is there any point in telling the guest to do these workarounds on TCG
> given they're unlikely to translate to host instructions which have the
> desired effect?

Probably not.  We might have to just advertise broken on TCG, at least
until someone has time to figure out the details.

> ---
>  hw/ppc/spapr_hcall.c   | 81 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/ppc/spapr.h |  1 +
>  2 files changed, 82 insertions(+)
> 
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index 51eba52e86..b62b47c8d9 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -1654,6 +1654,84 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
>      return H_SUCCESS;
>  }
>  
> +#define CPU_CHARACTERISTIC_SPEC_BARRIER         (1ULL << (63 - 0))
> +#define CPU_CHARACTERISTIC_BCCTR_SERIAL         (1ULL << (63 - 1))
> +#define CPU_CHARACTERISTIC_ORI_L1_CACHE         (1ULL << (63 - 2))
> +#define CPU_CHARACTERISTIC_MTTRIG_L1_CACHE      (1ULL << (63 - 3))
> +#define CPU_CHARACTERISTIC_L1_CACHE_PRIV        (1ULL << (63 - 4))
> +#define CPU_CHARACTERISTIC_BRANCH_HINTS         (1ULL << (63 - 5))
> +#define CPU_CHARACTERISTIC_MTTRIG_THR_RECONF    (1ULL << (63 - 6))
> +#define CPU_BEHAVIOUR_FAVOUR_SECURITY           (1ULL << (63 - 0))
> +#define CPU_BEHAVIOUR_L1_CACHE_FLUSH            (1ULL << (63 - 1))
> +#define CPU_BEHAVIOUR_SPEC_BARRIER              (1ULL << (63 - 2))
> +
> +static target_ulong h_get_cpu_characteristics(PowerPCCPU *cpu,
> +                                              sPAPRMachineState *spapr,
> +                                              target_ulong opcode,
> +                                              target_ulong *args)
> +{
> +    uint64_t characteristics = CPU_CHARACTERISTIC_BRANCH_HINTS;
> +    uint64_t behaviour = CPU_BEHAVIOUR_FAVOUR_SECURITY;

I guess we're going to want another knob for the favour security vs
favour performance bit here.

> +    uint8_t safe_cache = spapr_get_cap(spapr, SPAPR_CAP_CFPC);
> +    uint8_t safe_bounds_check = spapr_get_cap(spapr, SPAPR_CAP_SBBC);
> +    uint8_t safe_indirect_branch = spapr_get_cap(spapr, SPAPR_CAP_IBS);
> +
> +    /* TODO: Is TCG vulnerable? */

Good question, but in any case..

> +    if (!kvm_enabled()) {
> +        return H_FUNCTION;
> +    }

..this should still advertise things based on the caps.  The point we
apply the caps to the virtual hardware is where we need to consider
TCG's vulnerability.

> +
> +    switch (safe_cache) {
> +    case SPAPR_CAP_WORKAROUND:
> +        characteristics |= CPU_CHARACTERISTIC_ORI_L1_CACHE;
> +        characteristics |= CPU_CHARACTERISTIC_MTTRIG_L1_CACHE;
> +        characteristics |= CPU_CHARACTERISTIC_L1_CACHE_PRIV;
> +        behaviour |= CPU_BEHAVIOUR_L1_CACHE_FLUSH;
> +        break;
> +    case SPAPR_CAP_FIXED:
> +        break;
> +    default: /* broken */
> +        if (safe_cache != SPAPR_CAP_BROKEN) {
> +            error_report("Invalid value for KVM_CAP_PPC_SAFE_CACHE (%d), assuming broken",
> +                         safe_cache);
> +        }
> +        behaviour |= CPU_BEHAVIOUR_L1_CACHE_FLUSH;
> +        break;
> +    }
> +
> +    switch (safe_bounds_check) {
> +    case SPAPR_CAP_WORKAROUND:
> +        characteristics |= CPU_CHARACTERISTIC_SPEC_BARRIER;
> +        behaviour |= CPU_BEHAVIOUR_SPEC_BARRIER;
> +        break;
> +    case SPAPR_CAP_FIXED:
> +        break;
> +    default: /* broken */
> +        if (safe_bounds_check != SPAPR_CAP_BROKEN) {
> +            error_report("Invalid value for KVM_CAP_PPC_SAFE_BOUNDS_CHECK (%d), assuming broken",
> +                         safe_bounds_check);
> +        }
> +        behaviour |= CPU_BEHAVIOUR_SPEC_BARRIER;
> +        break;
> +    }
> +
> +    switch (safe_indirect_branch) {
> +    case SPAPR_CAP_FIXED:
> +        characteristics |= CPU_CHARACTERISTIC_BCCTR_SERIAL;
> +    default: /* broken */
> +        if (safe_indirect_branch != SPAPR_CAP_BROKEN) {
> +            error_report("Invalid value for KVM_CAP_PPC_SAFE_INDIRECT_BRANCH (%d), assuming broken",
> +                         safe_indirect_branch);
> +        }
> +        break;
> +    }
> +
> +    args[0] = characteristics;
> +    args[1] = behaviour;
> +
> +    return H_SUCCESS;
> +}
> +
>  static spapr_hcall_fn papr_hypercall_table[(MAX_HCALL_OPCODE / 4) + 1];
>  static spapr_hcall_fn kvmppc_hypercall_table[KVMPPC_HCALL_MAX - KVMPPC_HCALL_BASE + 1];
>  
> @@ -1733,6 +1811,9 @@ static void hypercall_register_types(void)
>      spapr_register_hypercall(H_INVALIDATE_PID, h_invalidate_pid);
>      spapr_register_hypercall(H_REGISTER_PROC_TBL, h_register_process_table);
>  
> +    /* hcall-get-cpu-characteristics */
> +    spapr_register_hypercall(H_GET_CPU_CHARACTERISTICS, h_get_cpu_characteristics);
> +
>      /* "debugger" hcalls (also used by SLOF). Note: We do -not- differenciate
>       * here between the "CI" and the "CACHE" variants, they will use whatever
>       * mapping attributes qemu is using. When using KVM, the kernel will
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 2db2f3e2e2..5677c38d2a 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -396,6 +396,7 @@ struct sPAPRMachineState {
>  #define H_GET_HCA_INFO          0x1B8
>  #define H_GET_PERF_COUNT        0x1BC
>  #define H_MANAGE_TRACE          0x1C0
> +#define H_GET_CPU_CHARACTERISTICS 0x1C8
>  #define H_FREE_LOGICAL_LAN_BUFFER 0x1D4
>  #define H_QUERY_INT_STATE       0x1E4
>  #define H_POLL_PENDING          0x1D8

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