[Qemu-devel] [PATCH 0/6] spapr: Add optional capabilities

[Qemu-devel] [PATCH 0/6] spapr: Add optional capabilities

[David Gibson]

This series is a first draft to add the notion of optional
capabilities to the "pseries" machine type.  A default set of
capabilities is selected based on the machine type version and
selected cpu model, but this can be overridden with machine
parameters.

The purpose of this is to get rid of a number of places where we
implicitly decide what features to advertise to the guest based on
capabilities of the host.  This is bad, because it means it's
difficult to be certain if machines started at different ends of a
migration really match from the guest's point of view.

By giving the user explicit control of these optional features, then
validating that the chosen ones can be supplied on the host we make
behaviour more predictable.

The more specific motivation for this is that POWER9 has bugs in its
hardware transactional memory (HTM) implementation making it unsafe to
migrate POWER8 guests to POWER9 if they use HTM.

Changes since RFC:
 - Two preliminary patches not really related split off and already
   merged.
 - Assorted minor fixes based on review notes
 - Change defaults to disable HTM on pseries-2.12 machine type

David Gibson (6):
  spapr: Capabilities infrastructure
  spapr: Treat Hardware Transactional Memory (HTM) as an optional
    capability
  spapr: Validate capabilities on migration
  target/ppc: Clean up probing of VMX, VSX and DFP availability on KVM
  spapr: Handle VMX/VSX presence as an spapr capability flag
  spapr: Handle Decimal Floating Point (DFP) as an optional capability

 hw/ppc/Makefile.objs   |   2 +-
 hw/ppc/spapr.c         |  47 +++++--
 hw/ppc/spapr_caps.c    | 342 +++++++++++++++++++++++++++++++++++++++++++++++++
 include/hw/ppc/spapr.h |  46 +++++++
 target/ppc/kvm.c       |  27 +---
 target/ppc/kvm_ppc.h   |   2 -
 6 files changed, 429 insertions(+), 37 deletions(-)
 create mode 100644 hw/ppc/spapr_caps.c

-- 
2.14.3

[Qemu-devel] [PATCH 1/6] spapr: Capabilities infrastructure

[David Gibson]

Because PAPR is a paravirtual environment access to certain CPU (or other)
facilities can be blocked by the hypervisor.  PAPR provides ways to
advertise in the device tree whether or not those features are available to
the guest.

In some places we automatically determine whether to make a feature
available based on whether our host can support it, in most cases this is
based on limitations in the available KVM implementation.

Although we correctly advertise this to the guest, it means that host
factors might make changes to the guest visible environment which is bad:
as well as generaly reducing reproducibility, it means that a migration
between different host environments can easily go bad.

We've mostly gotten away with it because the environments considered mature
enough to be well supported (basically, KVM on POWER8) have had consistent
feature availability.  But, it's still not right and some limitations on
POWER9 is going to make it more of an issue in future.

This introduces an infrastructure for defining "sPAPR capabilities".  These
are set by default based on the machine version, masked by the capabilities
of the chosen cpu, but can be overriden with machine properties.

The intention is at reset time we verify that the requested capabilities
can be supported on the host (considering TCG, KVM and/or host cpu
limitations).  If not we simply fail, rather than silently modifying the
advertised featureset to the guest.

This does mean that certain configurations that "worked" may now fail, but
such configurations were already more subtly broken.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/Makefile.objs   |   2 +-
 hw/ppc/spapr.c         |   7 ++
 hw/ppc/spapr_caps.c    | 182 +++++++++++++++++++++++++++++++++++++++++++++++++
 include/hw/ppc/spapr.h |  31 +++++++++
 4 files changed, 221 insertions(+), 1 deletion(-)
 create mode 100644 hw/ppc/spapr_caps.c

diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
index 7efc686748..1faff853b7 100644
--- a/hw/ppc/Makefile.objs
+++ b/hw/ppc/Makefile.objs
@@ -1,7 +1,7 @@
 # shared objects
 obj-y += ppc.o ppc_booke.o fdt.o
 # IBM pSeries (sPAPR)
-obj-$(CONFIG_PSERIES) += spapr.o spapr_vio.o spapr_events.o
+obj-$(CONFIG_PSERIES) += spapr.o spapr_caps.o spapr_vio.o spapr_events.o
 obj-$(CONFIG_PSERIES) += spapr_hcall.o spapr_iommu.o spapr_rtas.o
 obj-$(CONFIG_PSERIES) += spapr_pci.o spapr_rtc.o spapr_drc.o spapr_rng.o
 obj-$(CONFIG_PSERIES) += spapr_cpu_core.o spapr_ovec.o
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 6785a90c60..d472baef8d 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1466,6 +1466,8 @@ static void spapr_machine_reset(void)
     /* Check for unknown sysbus devices */
     foreach_dynamic_sysbus_device(find_unknown_sysbus_device, NULL);
 
+    spapr_caps_reset(spapr);
+
     first_ppc_cpu = POWERPC_CPU(first_cpu);
     if (kvm_enabled() && kvmppc_has_cap_mmu_radix() &&
         ppc_check_compat(first_ppc_cpu, CPU_POWERPC_LOGICAL_3_00, 0,
@@ -2311,6 +2313,8 @@ 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);
@@ -3819,6 +3823,9 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
      * in which LMBs are represented and hot-added
      */
     mc->numa_mem_align_shift = 28;
+
+    smc->default_caps = spapr_caps(0);
+    spapr_caps_add_properties(smc, &error_abort);
 }
 
 static const TypeInfo spapr_machine_info = {
diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
new file mode 100644
index 0000000000..828ac69b36
--- /dev/null
+++ b/hw/ppc/spapr_caps.c
@@ -0,0 +1,182 @@
+/*
+ * QEMU PowerPC pSeries Logical Partition capabilities handling
+ *
+ * Copyright (c) 2017 David Gibson, Red Hat Inc.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "qapi/visitor.h"
+
+#include "hw/ppc/spapr.h"
+
+typedef struct sPAPRCapabilityInfo {
+    const char *name;
+    const char *description;
+    uint64_t flag;
+
+    /* 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);
+} sPAPRCapabilityInfo;
+
+static sPAPRCapabilityInfo capability_table[] = {
+};
+
+static sPAPRCapabilities default_caps_with_cpu(sPAPRMachineState *spapr,
+                                               CPUState *cs)
+{
+    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
+    sPAPRCapabilities caps;
+
+    caps = smc->default_caps;
+
+    /* TODO: clamp according to cpu model */
+
+    return caps;
+}
+
+void spapr_caps_reset(sPAPRMachineState *spapr)
+{
+    Error *local_err = NULL;
+    sPAPRCapabilities caps;
+    int i;
+
+    /* First compute the actual set of caps we're running with.. */
+    caps = default_caps_with_cpu(spapr, first_cpu);
+
+    caps.mask |= spapr->forced_caps.mask;
+    caps.mask &= ~spapr->forbidden_caps.mask;
+
+    spapr->effective_caps = caps;
+
+    /* .. then apply those caps to the virtual hardware */
+
+    for (i = 0; i < ARRAY_SIZE(capability_table); i++) {
+        sPAPRCapabilityInfo *info = &capability_table[i];
+
+        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);
+                error_free(local_err);
+                local_err = NULL;
+            }
+        }
+    }
+}
+
+static void spapr_cap_get(Object *obj, Visitor *v, const char *name,
+                          void *opaque, Error **errp)
+{
+    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()? */
+
+    visit_type_bool(v, name, &value, errp);
+}
+
+static void spapr_cap_set(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;
+    }
+
+    if (value) {
+        spapr->forced_caps.mask |= cap->flag;
+    } else {
+        spapr->forbidden_caps.mask |= cap->flag;
+    }
+}
+
+void spapr_caps_validate(sPAPRMachineState *spapr, Error **errp)
+{
+    uint64_t allcaps = 0;
+    int i;
+
+    for (i = 0; i < ARRAY_SIZE(capability_table); i++) {
+        g_assert((allcaps & capability_table[i].flag) == 0);
+        allcaps |= capability_table[i].flag;
+    }
+
+    g_assert((spapr->forced_caps.mask & ~allcaps) == 0);
+    g_assert((spapr->forbidden_caps.mask & ~allcaps) == 0);
+
+    if (spapr->forced_caps.mask & spapr->forbidden_caps.mask) {
+        error_setg(errp, "Some sPAPR capabilities set both on and off");
+        return;
+    }
+
+    /* Check for any caps incompatible with other caps.  Nothing to do
+     * yet */
+}
+
+void spapr_caps_add_properties(sPAPRMachineClass *smc, Error **errp)
+{
+    Error *local_err = NULL;
+    ObjectClass *klass = OBJECT_CLASS(smc);
+    int i;
+
+    for (i = 0; i < ARRAY_SIZE(capability_table); i++) {
+        sPAPRCapabilityInfo *cap = &capability_table[i];
+        const char *name = g_strdup_printf("cap-%s", cap->name);
+
+        object_class_property_add(klass, name, "bool",
+                                  spapr_cap_get, spapr_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);
+        if (local_err) {
+            error_propagate(errp, local_err);
+            return;
+        }
+    }
+}
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 14757b805e..5569caf1d4 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -50,6 +50,15 @@ typedef enum {
     SPAPR_RESIZE_HPT_REQUIRED,
 } sPAPRResizeHPT;
 
+/**
+ * Capabilities
+ */
+
+typedef struct sPAPRCapabilities sPAPRCapabilities;
+struct sPAPRCapabilities {
+    uint64_t mask;
+};
+
 /**
  * sPAPRMachineClass:
  */
@@ -66,6 +75,7 @@ struct sPAPRMachineClass {
                           hwaddr *mmio32, hwaddr *mmio64,
                           unsigned n_dma, uint32_t *liobns, Error **errp);
     sPAPRResizeHPT resize_hpt_default;
+    sPAPRCapabilities default_caps;
 };
 
 /**
@@ -127,6 +137,9 @@ struct sPAPRMachineState {
     MemoryHotplugState hotplug_memory;
 
     const char *icp_type;
+
+    sPAPRCapabilities forced_caps, forbidden_caps;
+    sPAPRCapabilities effective_caps;
 };
 
 #define H_SUCCESS         0
@@ -724,4 +737,22 @@ int spapr_irq_alloc_block(sPAPRMachineState *spapr, int num, bool lsi,
 void spapr_irq_free(sPAPRMachineState *spapr, int irq, int num);
 qemu_irq spapr_qirq(sPAPRMachineState *spapr, int irq);
 
+/*
+ * Handling of optional capabilities
+ */
+static inline sPAPRCapabilities spapr_caps(uint64_t mask)
+{
+    sPAPRCapabilities caps = { mask };
+    return caps;
+}
+
+static inline bool spapr_has_cap(sPAPRMachineState *spapr, uint64_t cap)
+{
+    return !!(spapr->effective_caps.mask & cap);
+}
+
+void spapr_caps_reset(sPAPRMachineState *spapr);
+void spapr_caps_validate(sPAPRMachineState *spapr, Error **errp);
+void spapr_caps_add_properties(sPAPRMachineClass *smc, Error **errp);
+
 #endif /* HW_SPAPR_H */
-- 
2.14.3

Re: [Qemu-devel] [PATCH 1/6] spapr: Capabilities infrastructure

[Greg Kurz]

On Mon, 18 Dec 2017 20:20:19 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> Because PAPR is a paravirtual environment access to certain CPU (or other)
> facilities can be blocked by the hypervisor.  PAPR provides ways to
> advertise in the device tree whether or not those features are available to
> the guest.
> 
> In some places we automatically determine whether to make a feature
> available based on whether our host can support it, in most cases this is
> based on limitations in the available KVM implementation.
> 
> Although we correctly advertise this to the guest, it means that host
> factors might make changes to the guest visible environment which is bad:
> as well as generaly reducing reproducibility, it means that a migration
> between different host environments can easily go bad.
> 
> We've mostly gotten away with it because the environments considered mature
> enough to be well supported (basically, KVM on POWER8) have had consistent
> feature availability.  But, it's still not right and some limitations on
> POWER9 is going to make it more of an issue in future.
> 
> This introduces an infrastructure for defining "sPAPR capabilities".  These
> are set by default based on the machine version, masked by the capabilities
> of the chosen cpu, but can be overriden with machine properties.
> 
> The intention is at reset time we verify that the requested capabilities
> can be supported on the host (considering TCG, KVM and/or host cpu
> limitations).  If not we simply fail, rather than silently modifying the
> advertised featureset to the guest.
> 
> This does mean that certain configurations that "worked" may now fail, but
> such configurations were already more subtly broken.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/ppc/Makefile.objs   |   2 +-
>  hw/ppc/spapr.c         |   7 ++
>  hw/ppc/spapr_caps.c    | 182 +++++++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/ppc/spapr.h |  31 +++++++++
>  4 files changed, 221 insertions(+), 1 deletion(-)
>  create mode 100644 hw/ppc/spapr_caps.c
> 
> diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
> index 7efc686748..1faff853b7 100644
> --- a/hw/ppc/Makefile.objs
> +++ b/hw/ppc/Makefile.objs
> @@ -1,7 +1,7 @@
>  # shared objects
>  obj-y += ppc.o ppc_booke.o fdt.o
>  # IBM pSeries (sPAPR)
> -obj-$(CONFIG_PSERIES) += spapr.o spapr_vio.o spapr_events.o
> +obj-$(CONFIG_PSERIES) += spapr.o spapr_caps.o spapr_vio.o spapr_events.o
>  obj-$(CONFIG_PSERIES) += spapr_hcall.o spapr_iommu.o spapr_rtas.o
>  obj-$(CONFIG_PSERIES) += spapr_pci.o spapr_rtc.o spapr_drc.o spapr_rng.o
>  obj-$(CONFIG_PSERIES) += spapr_cpu_core.o spapr_ovec.o
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 6785a90c60..d472baef8d 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1466,6 +1466,8 @@ static void spapr_machine_reset(void)
>      /* Check for unknown sysbus devices */
>      foreach_dynamic_sysbus_device(find_unknown_sysbus_device, NULL);
>  
> +    spapr_caps_reset(spapr);
> +
>      first_ppc_cpu = POWERPC_CPU(first_cpu);
>      if (kvm_enabled() && kvmppc_has_cap_mmu_radix() &&
>          ppc_check_compat(first_ppc_cpu, CPU_POWERPC_LOGICAL_3_00, 0,
> @@ -2311,6 +2313,8 @@ 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);
> @@ -3819,6 +3823,9 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
>       * in which LMBs are represented and hot-added
>       */
>      mc->numa_mem_align_shift = 28;
> +
> +    smc->default_caps = spapr_caps(0);
> +    spapr_caps_add_properties(smc, &error_abort);
>  }
>  
>  static const TypeInfo spapr_machine_info = {
> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
> new file mode 100644
> index 0000000000..828ac69b36
> --- /dev/null
> +++ b/hw/ppc/spapr_caps.c
> @@ -0,0 +1,182 @@
> +/*
> + * QEMU PowerPC pSeries Logical Partition capabilities handling
> + *
> + * Copyright (c) 2017 David Gibson, Red Hat Inc.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +#include "qemu/osdep.h"
> +#include "qapi/error.h"
> +#include "qapi/visitor.h"
> +
> +#include "hw/ppc/spapr.h"
> +
> +typedef struct sPAPRCapabilityInfo {
> +    const char *name;
> +    const char *description;
> +    uint64_t flag;
> +
> +    /* 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);
> +} sPAPRCapabilityInfo;
> +
> +static sPAPRCapabilityInfo capability_table[] = {
> +};
> +
> +static sPAPRCapabilities default_caps_with_cpu(sPAPRMachineState *spapr,
> +                                               CPUState *cs)
> +{
> +    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
> +    sPAPRCapabilities caps;
> +
> +    caps = smc->default_caps;
> +
> +    /* TODO: clamp according to cpu model */
> +
> +    return caps;
> +}
> +
> +void spapr_caps_reset(sPAPRMachineState *spapr)
> +{
> +    Error *local_err = NULL;
> +    sPAPRCapabilities caps;
> +    int i;
> +
> +    /* First compute the actual set of caps we're running with.. */
> +    caps = default_caps_with_cpu(spapr, first_cpu);
> +
> +    caps.mask |= spapr->forced_caps.mask;
> +    caps.mask &= ~spapr->forbidden_caps.mask;
> +
> +    spapr->effective_caps = caps;
> +
> +    /* .. then apply those caps to the virtual hardware */
> +
> +    for (i = 0; i < ARRAY_SIZE(capability_table); i++) {
> +        sPAPRCapabilityInfo *info = &capability_table[i];
> +
> +        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);
> +                error_free(local_err);

double free.

/*
 * Convenience function to warn_report() and free @err.
 */
void warn_report_err(Error *err);


The rest of the patch is fine, so with the above fixed, you can add:

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

> +                local_err = NULL;
> +            }
> +        }
> +    }
> +}
> +
> +static void spapr_cap_get(Object *obj, Visitor *v, const char *name,
> +                          void *opaque, Error **errp)
> +{
> +    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()? */
> +
> +    visit_type_bool(v, name, &value, errp);
> +}
> +
> +static void spapr_cap_set(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;
> +    }
> +
> +    if (value) {
> +        spapr->forced_caps.mask |= cap->flag;
> +    } else {
> +        spapr->forbidden_caps.mask |= cap->flag;
> +    }
> +}
> +
> +void spapr_caps_validate(sPAPRMachineState *spapr, Error **errp)
> +{
> +    uint64_t allcaps = 0;
> +    int i;
> +
> +    for (i = 0; i < ARRAY_SIZE(capability_table); i++) {
> +        g_assert((allcaps & capability_table[i].flag) == 0);
> +        allcaps |= capability_table[i].flag;
> +    }
> +
> +    g_assert((spapr->forced_caps.mask & ~allcaps) == 0);
> +    g_assert((spapr->forbidden_caps.mask & ~allcaps) == 0);
> +
> +    if (spapr->forced_caps.mask & spapr->forbidden_caps.mask) {
> +        error_setg(errp, "Some sPAPR capabilities set both on and off");
> +        return;
> +    }
> +
> +    /* Check for any caps incompatible with other caps.  Nothing to do
> +     * yet */
> +}
> +
> +void spapr_caps_add_properties(sPAPRMachineClass *smc, Error **errp)
> +{
> +    Error *local_err = NULL;
> +    ObjectClass *klass = OBJECT_CLASS(smc);
> +    int i;
> +
> +    for (i = 0; i < ARRAY_SIZE(capability_table); i++) {
> +        sPAPRCapabilityInfo *cap = &capability_table[i];
> +        const char *name = g_strdup_printf("cap-%s", cap->name);
> +
> +        object_class_property_add(klass, name, "bool",
> +                                  spapr_cap_get, spapr_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);
> +        if (local_err) {
> +            error_propagate(errp, local_err);
> +            return;
> +        }
> +    }
> +}
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 14757b805e..5569caf1d4 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -50,6 +50,15 @@ typedef enum {
>      SPAPR_RESIZE_HPT_REQUIRED,
>  } sPAPRResizeHPT;
>  
> +/**
> + * Capabilities
> + */
> +
> +typedef struct sPAPRCapabilities sPAPRCapabilities;
> +struct sPAPRCapabilities {
> +    uint64_t mask;
> +};
> +
>  /**
>   * sPAPRMachineClass:
>   */
> @@ -66,6 +75,7 @@ struct sPAPRMachineClass {
>                            hwaddr *mmio32, hwaddr *mmio64,
>                            unsigned n_dma, uint32_t *liobns, Error **errp);
>      sPAPRResizeHPT resize_hpt_default;
> +    sPAPRCapabilities default_caps;
>  };
>  
>  /**
> @@ -127,6 +137,9 @@ struct sPAPRMachineState {
>      MemoryHotplugState hotplug_memory;
>  
>      const char *icp_type;
> +
> +    sPAPRCapabilities forced_caps, forbidden_caps;
> +    sPAPRCapabilities effective_caps;
>  };
>  
>  #define H_SUCCESS         0
> @@ -724,4 +737,22 @@ int spapr_irq_alloc_block(sPAPRMachineState *spapr, int num, bool lsi,
>  void spapr_irq_free(sPAPRMachineState *spapr, int irq, int num);
>  qemu_irq spapr_qirq(sPAPRMachineState *spapr, int irq);
>  
> +/*
> + * Handling of optional capabilities
> + */
> +static inline sPAPRCapabilities spapr_caps(uint64_t mask)
> +{
> +    sPAPRCapabilities caps = { mask };
> +    return caps;
> +}
> +
> +static inline bool spapr_has_cap(sPAPRMachineState *spapr, uint64_t cap)
> +{
> +    return !!(spapr->effective_caps.mask & cap);
> +}
> +
> +void spapr_caps_reset(sPAPRMachineState *spapr);
> +void spapr_caps_validate(sPAPRMachineState *spapr, Error **errp);
> +void spapr_caps_add_properties(sPAPRMachineClass *smc, Error **errp);
> +
>  #endif /* HW_SPAPR_H */

Re: [Qemu-devel] [PATCH 1/6] spapr: Capabilities infrastructure

[David Gibson]

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

On Mon, Dec 18, 2017 at 10:58:00AM +0100, Greg Kurz wrote:
> On Mon, 18 Dec 2017 20:20:19 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
[snip]
> > +void spapr_caps_reset(sPAPRMachineState *spapr)
> > +{
> > +    Error *local_err = NULL;
> > +    sPAPRCapabilities caps;
> > +    int i;
> > +
> > +    /* First compute the actual set of caps we're running with.. */
> > +    caps = default_caps_with_cpu(spapr, first_cpu);
> > +
> > +    caps.mask |= spapr->forced_caps.mask;
> > +    caps.mask &= ~spapr->forbidden_caps.mask;
> > +
> > +    spapr->effective_caps = caps;
> > +
> > +    /* .. then apply those caps to the virtual hardware */
> > +
> > +    for (i = 0; i < ARRAY_SIZE(capability_table); i++) {
> > +        sPAPRCapabilityInfo *info = &capability_table[i];
> > +
> > +        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);
> > +                error_free(local_err);
> 
> double free.
> 
> /*
>  * Convenience function to warn_report() and free @err.
>  */
> void warn_report_err(Error *err);

Drat, I've made that mistake before.  Fixed now.

> The rest of the patch is fine, so with the above fixed, you can add:
> 
> Reviewed-by: Greg Kurz <groug@kaod.org>
> 
> > +                local_err = NULL;
> > +            }
> > +        }
> > +    }
> > +}
> > +
> > +static void spapr_cap_get(Object *obj, Visitor *v, const char *name,
> > +                          void *opaque, Error **errp)
> > +{
> > +    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()? */
> > +
> > +    visit_type_bool(v, name, &value, errp);
> > +}
> > +
> > +static void spapr_cap_set(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;
> > +    }
> > +
> > +    if (value) {
> > +        spapr->forced_caps.mask |= cap->flag;
> > +    } else {
> > +        spapr->forbidden_caps.mask |= cap->flag;
> > +    }
> > +}
> > +
> > +void spapr_caps_validate(sPAPRMachineState *spapr, Error **errp)
> > +{
> > +    uint64_t allcaps = 0;
> > +    int i;
> > +
> > +    for (i = 0; i < ARRAY_SIZE(capability_table); i++) {
> > +        g_assert((allcaps & capability_table[i].flag) == 0);
> > +        allcaps |= capability_table[i].flag;
> > +    }
> > +
> > +    g_assert((spapr->forced_caps.mask & ~allcaps) == 0);
> > +    g_assert((spapr->forbidden_caps.mask & ~allcaps) == 0);
> > +
> > +    if (spapr->forced_caps.mask & spapr->forbidden_caps.mask) {
> > +        error_setg(errp, "Some sPAPR capabilities set both on and off");
> > +        return;
> > +    }
> > +
> > +    /* Check for any caps incompatible with other caps.  Nothing to do
> > +     * yet */
> > +}
> > +
> > +void spapr_caps_add_properties(sPAPRMachineClass *smc, Error **errp)
> > +{
> > +    Error *local_err = NULL;
> > +    ObjectClass *klass = OBJECT_CLASS(smc);
> > +    int i;
> > +
> > +    for (i = 0; i < ARRAY_SIZE(capability_table); i++) {
> > +        sPAPRCapabilityInfo *cap = &capability_table[i];
> > +        const char *name = g_strdup_printf("cap-%s", cap->name);
> > +
> > +        object_class_property_add(klass, name, "bool",
> > +                                  spapr_cap_get, spapr_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);
> > +        if (local_err) {
> > +            error_propagate(errp, local_err);
> > +            return;
> > +        }
> > +    }
> > +}
> > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> > index 14757b805e..5569caf1d4 100644
> > --- a/include/hw/ppc/spapr.h
> > +++ b/include/hw/ppc/spapr.h
> > @@ -50,6 +50,15 @@ typedef enum {
> >      SPAPR_RESIZE_HPT_REQUIRED,
> >  } sPAPRResizeHPT;
> >  
> > +/**
> > + * Capabilities
> > + */
> > +
> > +typedef struct sPAPRCapabilities sPAPRCapabilities;
> > +struct sPAPRCapabilities {
> > +    uint64_t mask;
> > +};
> > +
> >  /**
> >   * sPAPRMachineClass:
> >   */
> > @@ -66,6 +75,7 @@ struct sPAPRMachineClass {
> >                            hwaddr *mmio32, hwaddr *mmio64,
> >                            unsigned n_dma, uint32_t *liobns, Error **errp);
> >      sPAPRResizeHPT resize_hpt_default;
> > +    sPAPRCapabilities default_caps;
> >  };
> >  
> >  /**
> > @@ -127,6 +137,9 @@ struct sPAPRMachineState {
> >      MemoryHotplugState hotplug_memory;
> >  
> >      const char *icp_type;
> > +
> > +    sPAPRCapabilities forced_caps, forbidden_caps;
> > +    sPAPRCapabilities effective_caps;
> >  };
> >  
> >  #define H_SUCCESS         0
> > @@ -724,4 +737,22 @@ int spapr_irq_alloc_block(sPAPRMachineState *spapr, int num, bool lsi,
> >  void spapr_irq_free(sPAPRMachineState *spapr, int irq, int num);
> >  qemu_irq spapr_qirq(sPAPRMachineState *spapr, int irq);
> >  
> > +/*
> > + * Handling of optional capabilities
> > + */
> > +static inline sPAPRCapabilities spapr_caps(uint64_t mask)
> > +{
> > +    sPAPRCapabilities caps = { mask };
> > +    return caps;
> > +}
> > +
> > +static inline bool spapr_has_cap(sPAPRMachineState *spapr, uint64_t cap)
> > +{
> > +    return !!(spapr->effective_caps.mask & cap);
> > +}
> > +
> > +void spapr_caps_reset(sPAPRMachineState *spapr);
> > +void spapr_caps_validate(sPAPRMachineState *spapr, Error **errp);
> > +void spapr_caps_add_properties(sPAPRMachineClass *smc, Error **errp);
> > +
> >  #endif /* HW_SPAPR_H */
> 

-- 
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] [PATCH 2/6] spapr: Treat Hardware Transactional Memory (HTM) as an optional capability

[David Gibson]

This adds an spapr capability bit for Hardware Transactional Memory.  It is
enabled by default for pseries-2.11 and earlier machine types. with POWER8
or later CPUs (as it must be, since earlier qemu versions would implicitly
allow it).  However it is disabled by default for the latest pseries-2.12
machine type.

This means that with the latest machine type, HTM will not be available,
regardless of CPU, unless it is explicitly enabled on the command line.
That change is made on the basis that:

 * This way running with -M pseries,accel=tcg will start with whatever cpu
   and will provide the same guest visible model as with accel=kvm.
     - More specifically, this means existing make check tests don't have
       to be modified to use cap-htm=off in order to run with TCG

 * We hope to add a new "HTM without suspend" feature in the not too
   distant future which could work on both POWER8 and POWER9 cpus, and
   could be enabled by default.

 * Best guesses suggest that future POWER cpus may well only support the
   HTM-without-suspend model, not the (frankly, horribly overcomplicated)
   POWER8 style HTM with suspend.

 * Anecdotal evidence suggests problems with HTM being enabled when it
   wasn't wanted are more common than being missing when it was.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr.c         | 15 ++++++++++-----
 hw/ppc/spapr_caps.c    | 29 ++++++++++++++++++++++++++++-
 include/hw/ppc/spapr.h |  3 +++
 3 files changed, 41 insertions(+), 6 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index d472baef8d..f8fee8ebcf 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -253,7 +253,9 @@ static int spapr_fixup_cpu_numa_dt(void *fdt, int offset, PowerPCCPU *cpu)
 }
 
 /* Populate the "ibm,pa-features" property */
-static void spapr_populate_pa_features(PowerPCCPU *cpu, void *fdt, int offset,
+static void spapr_populate_pa_features(sPAPRMachineState *spapr,
+                                       PowerPCCPU *cpu,
+                                       void *fdt, int offset,
                                        bool legacy_guest)
 {
     CPUPPCState *env = &cpu->env;
@@ -318,7 +320,7 @@ static void spapr_populate_pa_features(PowerPCCPU *cpu, void *fdt, int offset,
          */
         pa_features[3] |= 0x20;
     }
-    if (kvmppc_has_cap_htm() && pa_size > 24) {
+    if (spapr_has_cap(spapr, SPAPR_CAP_HTM) && pa_size > 24) {
         pa_features[24] |= 0x80;    /* Transactional memory support */
     }
     if (legacy_guest && pa_size > 40) {
@@ -384,8 +386,8 @@ static int spapr_fixup_cpu_dt(void *fdt, sPAPRMachineState *spapr)
             return ret;
         }
 
-        spapr_populate_pa_features(cpu, fdt, offset,
-                                         spapr->cas_legacy_guest_workaround);
+        spapr_populate_pa_features(spapr, cpu, fdt, offset,
+                                   spapr->cas_legacy_guest_workaround);
     }
     return ret;
 }
@@ -579,7 +581,7 @@ static void spapr_populate_cpu_dt(CPUState *cs, void *fdt, int offset,
                           page_sizes_prop, page_sizes_prop_size)));
     }
 
-    spapr_populate_pa_features(cpu, fdt, offset, false);
+    spapr_populate_pa_features(spapr, cpu, fdt, offset, false);
 
     _FDT((fdt_setprop_cell(fdt, offset, "ibm,chip-id",
                            cs->cpu_index / vcpus_per_socket)));
@@ -3903,7 +3905,10 @@ static void spapr_machine_2_11_instance_options(MachineState *machine)
 
 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);
     SET_MACHINE_COMPAT(mc, SPAPR_COMPAT_2_11);
 }
 
diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
index 828ac69b36..2f0ef98670 100644
--- a/hw/ppc/spapr_caps.c
+++ b/hw/ppc/spapr_caps.c
@@ -24,6 +24,10 @@
 #include "qemu/osdep.h"
 #include "qapi/error.h"
 #include "qapi/visitor.h"
+#include "sysemu/hw_accel.h"
+#include "target/ppc/cpu.h"
+#include "cpu-models.h"
+#include "kvm_ppc.h"
 
 #include "hw/ppc/spapr.h"
 
@@ -40,18 +44,41 @@ typedef struct sPAPRCapabilityInfo {
     void (*disallow)(sPAPRMachineState *spapr, Error **errp);
 } sPAPRCapabilityInfo;
 
+static void cap_htm_allow(sPAPRMachineState *spapr, Error **errp)
+{
+    if (tcg_enabled()) {
+        error_setg(errp,
+                   "No Transactional Memory support in TCG, try cap-htm=off");
+    } else if (kvm_enabled() && !kvmppc_has_cap_htm()) {
+        error_setg(errp,
+"KVM implementation does not support Transactional Memory, try cap-htm=off"
+            );
+    }
+}
+
 static sPAPRCapabilityInfo capability_table[] = {
+    {
+        .name = "htm",
+        .description = "Allow Hardware Transactional Memory (HTM)",
+        .flag = SPAPR_CAP_HTM,
+        .allow = cap_htm_allow,
+        /* TODO: add cap_htm_disallow */
+    },
 };
 
 static sPAPRCapabilities default_caps_with_cpu(sPAPRMachineState *spapr,
                                                CPUState *cs)
 {
     sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
+    PowerPCCPU *cpu = POWERPC_CPU(cs);
     sPAPRCapabilities caps;
 
     caps = smc->default_caps;
 
-    /* TODO: clamp according to cpu model */
+    if (!ppc_check_compat(cpu, CPU_POWERPC_LOGICAL_2_07,
+                          0, spapr->max_compat_pvr)) {
+        caps.mask &= ~SPAPR_CAP_HTM;
+    }
 
     return caps;
 }
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 5569caf1d4..dc64f4ebcb 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -54,6 +54,9 @@ typedef enum {
  * Capabilities
  */
 
+/* Hardware Transactional Memory */
+#define SPAPR_CAP_HTM               0x0000000000000001ULL
+
 typedef struct sPAPRCapabilities sPAPRCapabilities;
 struct sPAPRCapabilities {
     uint64_t mask;
-- 
2.14.3

Re: [Qemu-devel] [PATCH 2/6] spapr: Treat Hardware Transactional Memory (HTM) as an optional capability

[Greg Kurz]

On Mon, 18 Dec 2017 20:20:20 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> This adds an spapr capability bit for Hardware Transactional Memory.  It is
> enabled by default for pseries-2.11 and earlier machine types. with POWER8
> or later CPUs (as it must be, since earlier qemu versions would implicitly
> allow it).  However it is disabled by default for the latest pseries-2.12
> machine type.
> 
> This means that with the latest machine type, HTM will not be available,
> regardless of CPU, unless it is explicitly enabled on the command line.
> That change is made on the basis that:
> 
>  * This way running with -M pseries,accel=tcg will start with whatever cpu
>    and will provide the same guest visible model as with accel=kvm.
>      - More specifically, this means existing make check tests don't have
>        to be modified to use cap-htm=off in order to run with TCG
> 
>  * We hope to add a new "HTM without suspend" feature in the not too
>    distant future which could work on both POWER8 and POWER9 cpus, and
>    could be enabled by default.
> 
>  * Best guesses suggest that future POWER cpus may well only support the
>    HTM-without-suspend model, not the (frankly, horribly overcomplicated)
>    POWER8 style HTM with suspend.
> 
>  * Anecdotal evidence suggests problems with HTM being enabled when it
>    wasn't wanted are more common than being missing when it was.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/ppc/spapr.c         | 15 ++++++++++-----
>  hw/ppc/spapr_caps.c    | 29 ++++++++++++++++++++++++++++-
>  include/hw/ppc/spapr.h |  3 +++
>  3 files changed, 41 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index d472baef8d..f8fee8ebcf 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -253,7 +253,9 @@ static int spapr_fixup_cpu_numa_dt(void *fdt, int offset, PowerPCCPU *cpu)
>  }
>  
>  /* Populate the "ibm,pa-features" property */
> -static void spapr_populate_pa_features(PowerPCCPU *cpu, void *fdt, int offset,
> +static void spapr_populate_pa_features(sPAPRMachineState *spapr,

While here, maybe you could rename spapr_populate_pa_features() to
spapr_dt_pa_features() ?

But anyway, this isn't really the point of this series, so:

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

> +                                       PowerPCCPU *cpu,
> +                                       void *fdt, int offset,
>                                         bool legacy_guest)
>  {
>      CPUPPCState *env = &cpu->env;
> @@ -318,7 +320,7 @@ static void spapr_populate_pa_features(PowerPCCPU *cpu, void *fdt, int offset,
>           */
>          pa_features[3] |= 0x20;
>      }
> -    if (kvmppc_has_cap_htm() && pa_size > 24) {
> +    if (spapr_has_cap(spapr, SPAPR_CAP_HTM) && pa_size > 24) {
>          pa_features[24] |= 0x80;    /* Transactional memory support */
>      }
>      if (legacy_guest && pa_size > 40) {
> @@ -384,8 +386,8 @@ static int spapr_fixup_cpu_dt(void *fdt, sPAPRMachineState *spapr)
>              return ret;
>          }
>  
> -        spapr_populate_pa_features(cpu, fdt, offset,
> -                                         spapr->cas_legacy_guest_workaround);
> +        spapr_populate_pa_features(spapr, cpu, fdt, offset,
> +                                   spapr->cas_legacy_guest_workaround);
>      }
>      return ret;
>  }
> @@ -579,7 +581,7 @@ static void spapr_populate_cpu_dt(CPUState *cs, void *fdt, int offset,
>                            page_sizes_prop, page_sizes_prop_size)));
>      }
>  
> -    spapr_populate_pa_features(cpu, fdt, offset, false);
> +    spapr_populate_pa_features(spapr, cpu, fdt, offset, false);
>  
>      _FDT((fdt_setprop_cell(fdt, offset, "ibm,chip-id",
>                             cs->cpu_index / vcpus_per_socket)));
> @@ -3903,7 +3905,10 @@ static void spapr_machine_2_11_instance_options(MachineState *machine)
>  
>  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);
>      SET_MACHINE_COMPAT(mc, SPAPR_COMPAT_2_11);
>  }
>  
> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
> index 828ac69b36..2f0ef98670 100644
> --- a/hw/ppc/spapr_caps.c
> +++ b/hw/ppc/spapr_caps.c
> @@ -24,6 +24,10 @@
>  #include "qemu/osdep.h"
>  #include "qapi/error.h"
>  #include "qapi/visitor.h"
> +#include "sysemu/hw_accel.h"
> +#include "target/ppc/cpu.h"
> +#include "cpu-models.h"
> +#include "kvm_ppc.h"
>  
>  #include "hw/ppc/spapr.h"
>  
> @@ -40,18 +44,41 @@ typedef struct sPAPRCapabilityInfo {
>      void (*disallow)(sPAPRMachineState *spapr, Error **errp);
>  } sPAPRCapabilityInfo;
>  
> +static void cap_htm_allow(sPAPRMachineState *spapr, Error **errp)
> +{
> +    if (tcg_enabled()) {
> +        error_setg(errp,
> +                   "No Transactional Memory support in TCG, try cap-htm=off");
> +    } else if (kvm_enabled() && !kvmppc_has_cap_htm()) {
> +        error_setg(errp,
> +"KVM implementation does not support Transactional Memory, try cap-htm=off"
> +            );
> +    }
> +}
> +
>  static sPAPRCapabilityInfo capability_table[] = {
> +    {
> +        .name = "htm",
> +        .description = "Allow Hardware Transactional Memory (HTM)",
> +        .flag = SPAPR_CAP_HTM,
> +        .allow = cap_htm_allow,
> +        /* TODO: add cap_htm_disallow */
> +    },
>  };
>  
>  static sPAPRCapabilities default_caps_with_cpu(sPAPRMachineState *spapr,
>                                                 CPUState *cs)
>  {
>      sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
> +    PowerPCCPU *cpu = POWERPC_CPU(cs);
>      sPAPRCapabilities caps;
>  
>      caps = smc->default_caps;
>  
> -    /* TODO: clamp according to cpu model */
> +    if (!ppc_check_compat(cpu, CPU_POWERPC_LOGICAL_2_07,
> +                          0, spapr->max_compat_pvr)) {
> +        caps.mask &= ~SPAPR_CAP_HTM;
> +    }
>  
>      return caps;
>  }
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 5569caf1d4..dc64f4ebcb 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -54,6 +54,9 @@ typedef enum {
>   * Capabilities
>   */
>  
> +/* Hardware Transactional Memory */
> +#define SPAPR_CAP_HTM               0x0000000000000001ULL
> +
>  typedef struct sPAPRCapabilities sPAPRCapabilities;
>  struct sPAPRCapabilities {
>      uint64_t mask;

Re: [Qemu-devel] [PATCH 2/6] spapr: Treat Hardware Transactional Memory (HTM) as an optional capability

[David Gibson]

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

On Mon, Dec 18, 2017 at 12:10:41PM +0100, Greg Kurz wrote:
> On Mon, 18 Dec 2017 20:20:20 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > This adds an spapr capability bit for Hardware Transactional Memory.  It is
> > enabled by default for pseries-2.11 and earlier machine types. with POWER8
> > or later CPUs (as it must be, since earlier qemu versions would implicitly
> > allow it).  However it is disabled by default for the latest pseries-2.12
> > machine type.
> > 
> > This means that with the latest machine type, HTM will not be available,
> > regardless of CPU, unless it is explicitly enabled on the command line.
> > That change is made on the basis that:
> > 
> >  * This way running with -M pseries,accel=tcg will start with whatever cpu
> >    and will provide the same guest visible model as with accel=kvm.
> >      - More specifically, this means existing make check tests don't have
> >        to be modified to use cap-htm=off in order to run with TCG
> > 
> >  * We hope to add a new "HTM without suspend" feature in the not too
> >    distant future which could work on both POWER8 and POWER9 cpus, and
> >    could be enabled by default.
> > 
> >  * Best guesses suggest that future POWER cpus may well only support the
> >    HTM-without-suspend model, not the (frankly, horribly overcomplicated)
> >    POWER8 style HTM with suspend.
> > 
> >  * Anecdotal evidence suggests problems with HTM being enabled when it
> >    wasn't wanted are more common than being missing when it was.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  hw/ppc/spapr.c         | 15 ++++++++++-----
> >  hw/ppc/spapr_caps.c    | 29 ++++++++++++++++++++++++++++-
> >  include/hw/ppc/spapr.h |  3 +++
> >  3 files changed, 41 insertions(+), 6 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index d472baef8d..f8fee8ebcf 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -253,7 +253,9 @@ static int spapr_fixup_cpu_numa_dt(void *fdt, int offset, PowerPCCPU *cpu)
> >  }
> >  
> >  /* Populate the "ibm,pa-features" property */
> > -static void spapr_populate_pa_features(PowerPCCPU *cpu, void *fdt, int offset,
> > +static void spapr_populate_pa_features(sPAPRMachineState *spapr,
> 
> While here, maybe you could rename spapr_populate_pa_features() to
> spapr_dt_pa_features() ?
> 
> But anyway, this isn't really the point of this series, so:

Yeah, I don't think that's really in scope for this series.

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

Thanks.

-- 
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] [PATCH 3/6] spapr: Validate capabilities on migration

[David Gibson]

Now that the "pseries" machine type implements optional capabilities (well,
one so far) there's the possibility of having different capabilities
available at either end of a migration.  Although arguably a user error,
it would be nice to catch this situation and fail as gracefully as we can.

This adds code to migrate the capabilities flags.  These aren't pulled
directly into the destination's configuration since what the user has
specified on the destination command line should take precedence.  However,
they are checked against the destination capabilities.

If the source was using a capability which is absent on the destination,
we fail the migration, since that could easily cause a guest crash or other
bad behaviour.  If the source lacked a capability which is present on the
destination we warn, but allow the migration to proceed.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr.c         |  6 ++++
 hw/ppc/spapr_caps.c    | 96 ++++++++++++++++++++++++++++++++++++++++++++++++--
 include/hw/ppc/spapr.h |  6 ++++
 3 files changed, 105 insertions(+), 3 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index f8fee8ebcf..86fc83f9c2 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1589,6 +1589,11 @@ static int spapr_post_load(void *opaque, int version_id)
     sPAPRMachineState *spapr = (sPAPRMachineState *)opaque;
     int err = 0;
 
+    err = spapr_caps_post_migration(spapr);
+    if (err) {
+        return err;
+    }
+
     if (!object_dynamic_cast(OBJECT(spapr->ics), TYPE_ICS_KVM)) {
         CPUState *cs;
         CPU_FOREACH(cs) {
@@ -1755,6 +1760,7 @@ static const VMStateDescription vmstate_spapr = {
         &vmstate_spapr_ov5_cas,
         &vmstate_spapr_patb_entry,
         &vmstate_spapr_pending_events,
+        &vmstate_spapr_caps,
         NULL
     }
 };
diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
index 2f0ef98670..a8c726a88b 100644
--- a/hw/ppc/spapr_caps.c
+++ b/hw/ppc/spapr_caps.c
@@ -22,6 +22,7 @@
  * THE SOFTWARE.
  */
 #include "qemu/osdep.h"
+#include "qemu/error-report.h"
 #include "qapi/error.h"
 #include "qapi/visitor.h"
 #include "sysemu/hw_accel.h"
@@ -83,6 +84,93 @@ static sPAPRCapabilities default_caps_with_cpu(sPAPRMachineState *spapr,
     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 < ARRAY_SIZE(capability_table); 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);
+            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 (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)
+{
+    sPAPRMachineState *spapr = opaque;
+
+    spapr->mig_forced_caps = spapr->forced_caps;
+    spapr->mig_forbidden_caps = spapr->forbidden_caps;
+    return 0;
+}
+
+static int spapr_caps_pre_load(void *opaque)
+{
+    sPAPRMachineState *spapr = opaque;
+
+    spapr->mig_forced_caps = spapr_caps(0);
+    spapr->mig_forbidden_caps = spapr_caps(0);
+    return 0;
+}
+
+const VMStateDescription vmstate_spapr_caps = {
+    .name = "spapr/caps",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = spapr_caps_needed,
+    .pre_save = spapr_caps_pre_save,
+    .pre_load = spapr_caps_pre_load,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT64(mig_forced_caps.mask, sPAPRMachineState),
+        VMSTATE_UINT64(mig_forbidden_caps.mask, sPAPRMachineState),
+        VMSTATE_END_OF_LIST()
+    },
+};
+
 void spapr_caps_reset(sPAPRMachineState *spapr)
 {
     Error *local_err = NULL;
@@ -92,6 +180,11 @@ void spapr_caps_reset(sPAPRMachineState *spapr)
     /* First compute the actual set of caps we're running with.. */
     caps = default_caps_with_cpu(spapr, first_cpu);
 
+    /* Remove unnecessary forced/forbidden bits (this will help us
+     * with migration) */
+    spapr->forced_caps.mask &= ~caps.mask;
+    spapr->forbidden_caps.mask &= caps.mask;
+
     caps.mask |= spapr->forced_caps.mask;
     caps.mask &= ~spapr->forbidden_caps.mask;
 
@@ -176,9 +269,6 @@ void spapr_caps_validate(sPAPRMachineState *spapr, Error **errp)
         error_setg(errp, "Some sPAPR capabilities set both on and off");
         return;
     }
-
-    /* Check for any caps incompatible with other caps.  Nothing to do
-     * yet */
 }
 
 void spapr_caps_add_properties(sPAPRMachineClass *smc, Error **errp)
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index dc64f4ebcb..5c85f39c3b 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -54,6 +54,8 @@ typedef enum {
  * Capabilities
  */
 
+/* These bits go in the migration stream, so they can't be reassigned */
+
 /* Hardware Transactional Memory */
 #define SPAPR_CAP_HTM               0x0000000000000001ULL
 
@@ -142,6 +144,7 @@ struct sPAPRMachineState {
     const char *icp_type;
 
     sPAPRCapabilities forced_caps, forbidden_caps;
+    sPAPRCapabilities mig_forced_caps, mig_forbidden_caps;
     sPAPRCapabilities effective_caps;
 };
 
@@ -743,6 +746,8 @@ 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 };
@@ -757,5 +762,6 @@ static inline bool spapr_has_cap(sPAPRMachineState *spapr, uint64_t 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);
 
 #endif /* HW_SPAPR_H */
-- 
2.14.3

Re: [Qemu-devel] [PATCH 3/6] spapr: Validate capabilities on migration

[Greg Kurz]

On Mon, 18 Dec 2017 20:20:21 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> Now that the "pseries" machine type implements optional capabilities (well,
> one so far) there's the possibility of having different capabilities
> available at either end of a migration.  Although arguably a user error,
> it would be nice to catch this situation and fail as gracefully as we can.
> 
> This adds code to migrate the capabilities flags.  These aren't pulled
> directly into the destination's configuration since what the user has
> specified on the destination command line should take precedence.  However,
> they are checked against the destination capabilities.
> 
> If the source was using a capability which is absent on the destination,
> we fail the migration, since that could easily cause a guest crash or other
> bad behaviour.  If the source lacked a capability which is present on the
> destination we warn, but allow the migration to proceed.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---

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

>  hw/ppc/spapr.c         |  6 ++++
>  hw/ppc/spapr_caps.c    | 96 ++++++++++++++++++++++++++++++++++++++++++++++++--
>  include/hw/ppc/spapr.h |  6 ++++
>  3 files changed, 105 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index f8fee8ebcf..86fc83f9c2 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1589,6 +1589,11 @@ static int spapr_post_load(void *opaque, int version_id)
>      sPAPRMachineState *spapr = (sPAPRMachineState *)opaque;
>      int err = 0;
>  
> +    err = spapr_caps_post_migration(spapr);
> +    if (err) {
> +        return err;
> +    }
> +
>      if (!object_dynamic_cast(OBJECT(spapr->ics), TYPE_ICS_KVM)) {
>          CPUState *cs;
>          CPU_FOREACH(cs) {
> @@ -1755,6 +1760,7 @@ static const VMStateDescription vmstate_spapr = {
>          &vmstate_spapr_ov5_cas,
>          &vmstate_spapr_patb_entry,
>          &vmstate_spapr_pending_events,
> +        &vmstate_spapr_caps,
>          NULL
>      }
>  };
> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
> index 2f0ef98670..a8c726a88b 100644
> --- a/hw/ppc/spapr_caps.c
> +++ b/hw/ppc/spapr_caps.c
> @@ -22,6 +22,7 @@
>   * THE SOFTWARE.
>   */
>  #include "qemu/osdep.h"
> +#include "qemu/error-report.h"
>  #include "qapi/error.h"
>  #include "qapi/visitor.h"
>  #include "sysemu/hw_accel.h"
> @@ -83,6 +84,93 @@ static sPAPRCapabilities default_caps_with_cpu(sPAPRMachineState *spapr,
>      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 < ARRAY_SIZE(capability_table); 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);
> +            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 (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)
> +{
> +    sPAPRMachineState *spapr = opaque;
> +
> +    spapr->mig_forced_caps = spapr->forced_caps;
> +    spapr->mig_forbidden_caps = spapr->forbidden_caps;
> +    return 0;
> +}
> +
> +static int spapr_caps_pre_load(void *opaque)
> +{
> +    sPAPRMachineState *spapr = opaque;
> +
> +    spapr->mig_forced_caps = spapr_caps(0);
> +    spapr->mig_forbidden_caps = spapr_caps(0);
> +    return 0;
> +}
> +
> +const VMStateDescription vmstate_spapr_caps = {
> +    .name = "spapr/caps",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .needed = spapr_caps_needed,
> +    .pre_save = spapr_caps_pre_save,
> +    .pre_load = spapr_caps_pre_load,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT64(mig_forced_caps.mask, sPAPRMachineState),
> +        VMSTATE_UINT64(mig_forbidden_caps.mask, sPAPRMachineState),
> +        VMSTATE_END_OF_LIST()
> +    },
> +};
> +
>  void spapr_caps_reset(sPAPRMachineState *spapr)
>  {
>      Error *local_err = NULL;
> @@ -92,6 +180,11 @@ void spapr_caps_reset(sPAPRMachineState *spapr)
>      /* First compute the actual set of caps we're running with.. */
>      caps = default_caps_with_cpu(spapr, first_cpu);
>  
> +    /* Remove unnecessary forced/forbidden bits (this will help us
> +     * with migration) */
> +    spapr->forced_caps.mask &= ~caps.mask;
> +    spapr->forbidden_caps.mask &= caps.mask;
> +
>      caps.mask |= spapr->forced_caps.mask;
>      caps.mask &= ~spapr->forbidden_caps.mask;
>  
> @@ -176,9 +269,6 @@ void spapr_caps_validate(sPAPRMachineState *spapr, Error **errp)
>          error_setg(errp, "Some sPAPR capabilities set both on and off");
>          return;
>      }
> -
> -    /* Check for any caps incompatible with other caps.  Nothing to do
> -     * yet */
>  }
>  
>  void spapr_caps_add_properties(sPAPRMachineClass *smc, Error **errp)
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index dc64f4ebcb..5c85f39c3b 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -54,6 +54,8 @@ typedef enum {
>   * Capabilities
>   */
>  
> +/* These bits go in the migration stream, so they can't be reassigned */
> +
>  /* Hardware Transactional Memory */
>  #define SPAPR_CAP_HTM               0x0000000000000001ULL
>  
> @@ -142,6 +144,7 @@ struct sPAPRMachineState {
>      const char *icp_type;
>  
>      sPAPRCapabilities forced_caps, forbidden_caps;
> +    sPAPRCapabilities mig_forced_caps, mig_forbidden_caps;
>      sPAPRCapabilities effective_caps;
>  };
>  
> @@ -743,6 +746,8 @@ 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 };
> @@ -757,5 +762,6 @@ static inline bool spapr_has_cap(sPAPRMachineState *spapr, uint64_t 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);
>  
>  #endif /* HW_SPAPR_H */

[Qemu-devel] [PATCH 4/6] target/ppc: Clean up probing of VMX, VSX and DFP availability on KVM

[David Gibson]

When constructing the "host" cpu class we modify whether the VMX and VSX
vector extensions and DFP (Decimal Floating Point) are available
based on whether KVM can support those instructions.  This can depend on
policy in the host kernel as well as on the actual host cpu capabilities.

However, the way we probe for this is not very nice: we explicitly check
the host's device tree.  That works in practice, but it's not really
correct, since the device tree is a property of the host kernel's platform
which we don't really know about.  We get away with it because the only
modern POWER platforms happen to encode VMX, VSX and DFP availability in
the device tree in the same way.

Arguably we should have an explicit KVM capability for this, but we haven't
needed one so far.  Barring specific KVM policies which don't yet exist,
each of these instruction classes will be available in the guest if and
only if they're available in the qemu userspace process.  We can determine
that from the ELF AUX vector we're supplied with.

Once reworked like this, there are no more callers for kvmppc_get_vmx() and
kvmppc_get_dfp() so remove them.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 target/ppc/kvm.c     | 27 ++++++---------------------
 target/ppc/kvm_ppc.h |  2 --
 2 files changed, 6 insertions(+), 23 deletions(-)

diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
index 9d57debf0e..81d9bd56c7 100644
--- a/target/ppc/kvm.c
+++ b/target/ppc/kvm.c
@@ -2014,16 +2014,6 @@ uint64_t kvmppc_get_clockfreq(void)
     return kvmppc_read_int_cpu_dt("clock-frequency");
 }
 
-uint32_t kvmppc_get_vmx(void)
-{
-    return kvmppc_read_int_cpu_dt("ibm,vmx");
-}
-
-uint32_t kvmppc_get_dfp(void)
-{
-    return kvmppc_read_int_cpu_dt("ibm,dfp");
-}
-
 static int kvmppc_get_pvinfo(CPUPPCState *env, struct kvm_ppc_pvinfo *pvinfo)
  {
      PowerPCCPU *cpu = ppc_env_get_cpu(env);
@@ -2407,23 +2397,18 @@ static void alter_insns(uint64_t *word, uint64_t flags, bool on)
 static void kvmppc_host_cpu_class_init(ObjectClass *oc, void *data)
 {
     PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc);
-    uint32_t vmx = kvmppc_get_vmx();
-    uint32_t dfp = kvmppc_get_dfp();
     uint32_t dcache_size = kvmppc_read_int_cpu_dt("d-cache-size");
     uint32_t icache_size = kvmppc_read_int_cpu_dt("i-cache-size");
 
     /* Now fix up the class with information we can query from the host */
     pcc->pvr = mfpvr();
 
-    if (vmx != -1) {
-        /* Only override when we know what the host supports */
-        alter_insns(&pcc->insns_flags, PPC_ALTIVEC, vmx > 0);
-        alter_insns(&pcc->insns_flags2, PPC2_VSX, vmx > 1);
-    }
-    if (dfp != -1) {
-        /* Only override when we know what the host supports */
-        alter_insns(&pcc->insns_flags2, PPC2_DFP, dfp);
-    }
+    alter_insns(&pcc->insns_flags, PPC_ALTIVEC,
+                qemu_getauxval(AT_HWCAP) & PPC_FEATURE_HAS_ALTIVEC);
+    alter_insns(&pcc->insns_flags2, PPC2_VSX,
+                qemu_getauxval(AT_HWCAP) & PPC_FEATURE_HAS_VSX);
+    alter_insns(&pcc->insns_flags2, PPC2_DFP,
+                qemu_getauxval(AT_HWCAP) & PPC_FEATURE_HAS_DFP);
 
     if (dcache_size != -1) {
         pcc->l1_dcache_size = dcache_size;
diff --git a/target/ppc/kvm_ppc.h b/target/ppc/kvm_ppc.h
index d6be38ecaf..ecb55493cc 100644
--- a/target/ppc/kvm_ppc.h
+++ b/target/ppc/kvm_ppc.h
@@ -15,8 +15,6 @@
 
 uint32_t kvmppc_get_tbfreq(void);
 uint64_t kvmppc_get_clockfreq(void);
-uint32_t kvmppc_get_vmx(void);
-uint32_t kvmppc_get_dfp(void);
 bool kvmppc_get_host_model(char **buf);
 bool kvmppc_get_host_serial(char **buf);
 int kvmppc_get_hasidle(CPUPPCState *env);
-- 
2.14.3

Re: [Qemu-devel] [PATCH 4/6] target/ppc: Clean up probing of VMX, VSX and DFP availability on KVM

[Greg Kurz]

On Mon, 18 Dec 2017 20:20:22 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> When constructing the "host" cpu class we modify whether the VMX and VSX
> vector extensions and DFP (Decimal Floating Point) are available
> based on whether KVM can support those instructions.  This can depend on
> policy in the host kernel as well as on the actual host cpu capabilities.
> 
> However, the way we probe for this is not very nice: we explicitly check
> the host's device tree.  That works in practice, but it's not really
> correct, since the device tree is a property of the host kernel's platform
> which we don't really know about.  We get away with it because the only
> modern POWER platforms happen to encode VMX, VSX and DFP availability in
> the device tree in the same way.
> 
> Arguably we should have an explicit KVM capability for this, but we haven't
> needed one so far.  Barring specific KVM policies which don't yet exist,
> each of these instruction classes will be available in the guest if and
> only if they're available in the qemu userspace process.  We can determine
> that from the ELF AUX vector we're supplied with.
> 
> Once reworked like this, there are no more callers for kvmppc_get_vmx() and
> kvmppc_get_dfp() so remove them.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---

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

>  target/ppc/kvm.c     | 27 ++++++---------------------
>  target/ppc/kvm_ppc.h |  2 --
>  2 files changed, 6 insertions(+), 23 deletions(-)
> 
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index 9d57debf0e..81d9bd56c7 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -2014,16 +2014,6 @@ uint64_t kvmppc_get_clockfreq(void)
>      return kvmppc_read_int_cpu_dt("clock-frequency");
>  }
>  
> -uint32_t kvmppc_get_vmx(void)
> -{
> -    return kvmppc_read_int_cpu_dt("ibm,vmx");
> -}
> -
> -uint32_t kvmppc_get_dfp(void)
> -{
> -    return kvmppc_read_int_cpu_dt("ibm,dfp");
> -}
> -
>  static int kvmppc_get_pvinfo(CPUPPCState *env, struct kvm_ppc_pvinfo *pvinfo)
>   {
>       PowerPCCPU *cpu = ppc_env_get_cpu(env);
> @@ -2407,23 +2397,18 @@ static void alter_insns(uint64_t *word, uint64_t flags, bool on)
>  static void kvmppc_host_cpu_class_init(ObjectClass *oc, void *data)
>  {
>      PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc);
> -    uint32_t vmx = kvmppc_get_vmx();
> -    uint32_t dfp = kvmppc_get_dfp();
>      uint32_t dcache_size = kvmppc_read_int_cpu_dt("d-cache-size");
>      uint32_t icache_size = kvmppc_read_int_cpu_dt("i-cache-size");
>  
>      /* Now fix up the class with information we can query from the host */
>      pcc->pvr = mfpvr();
>  
> -    if (vmx != -1) {
> -        /* Only override when we know what the host supports */
> -        alter_insns(&pcc->insns_flags, PPC_ALTIVEC, vmx > 0);
> -        alter_insns(&pcc->insns_flags2, PPC2_VSX, vmx > 1);
> -    }
> -    if (dfp != -1) {
> -        /* Only override when we know what the host supports */
> -        alter_insns(&pcc->insns_flags2, PPC2_DFP, dfp);
> -    }
> +    alter_insns(&pcc->insns_flags, PPC_ALTIVEC,
> +                qemu_getauxval(AT_HWCAP) & PPC_FEATURE_HAS_ALTIVEC);
> +    alter_insns(&pcc->insns_flags2, PPC2_VSX,
> +                qemu_getauxval(AT_HWCAP) & PPC_FEATURE_HAS_VSX);
> +    alter_insns(&pcc->insns_flags2, PPC2_DFP,
> +                qemu_getauxval(AT_HWCAP) & PPC_FEATURE_HAS_DFP);
>  
>      if (dcache_size != -1) {
>          pcc->l1_dcache_size = dcache_size;
> diff --git a/target/ppc/kvm_ppc.h b/target/ppc/kvm_ppc.h
> index d6be38ecaf..ecb55493cc 100644
> --- a/target/ppc/kvm_ppc.h
> +++ b/target/ppc/kvm_ppc.h
> @@ -15,8 +15,6 @@
>  
>  uint32_t kvmppc_get_tbfreq(void);
>  uint64_t kvmppc_get_clockfreq(void);
> -uint32_t kvmppc_get_vmx(void);
> -uint32_t kvmppc_get_dfp(void);
>  bool kvmppc_get_host_model(char **buf);
>  bool kvmppc_get_host_serial(char **buf);
>  int kvmppc_get_hasidle(CPUPPCState *env);

[Qemu-devel] [PATCH 5/6] spapr: Handle VMX/VSX presence as an spapr capability flag

[David Gibson]

We currently have some conditionals in the spapr device tree code to decide
whether or not to advertise the availability of the VMX (aka Altivec) and
VSX vector extensions to the guest, based on whether the guest cpu has
those features.

This can lead to confusion and subtle failures on migration, since it makes
a guest visible change based only on host capabilities.  We now have a
better mechanism for this, in spapr capabilities flags, which explicitly
depend on user options rather than host capabilities.

Rework the advertisement of VSX and VMX based on a new VSX capability.  We
no longer bother with a conditional for VMX support, because every CPU
that's ever been supported by the pseries machine type supports VMX.

NOTE: Some userspace distributions (e.g. RHEL7.4) already rely on
availability of VSX in libc, so using cap-vsx=off may lead to a fatal
SIGILL in init.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr.c         | 20 +++++++++++---------
 hw/ppc/spapr_caps.c    | 25 +++++++++++++++++++++++++
 include/hw/ppc/spapr.h |  3 +++
 3 files changed, 39 insertions(+), 9 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 86fc83f9c2..693dd6f7b3 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -557,14 +557,16 @@ static void spapr_populate_cpu_dt(CPUState *cs, void *fdt, int offset,
                           segs, sizeof(segs))));
     }
 
-    /* Advertise VMX/VSX (vector extensions) if available
-     *   0 / no property == no vector extensions
+    /* Advertise VSX (vector extensions) if available
      *   1               == VMX / Altivec available
-     *   2               == VSX available */
-    if (env->insns_flags & PPC_ALTIVEC) {
-        uint32_t vmx = (env->insns_flags2 & PPC2_VSX) ? 2 : 1;
-
-        _FDT((fdt_setprop_cell(fdt, offset, "ibm,vmx", vmx)));
+     *   2               == VSX available
+     *
+     * 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)) {
+        _FDT((fdt_setprop_cell(fdt, offset, "ibm,vmx", 2)));
+    } else {
+        _FDT((fdt_setprop_cell(fdt, offset, "ibm,vmx", 1)));
     }
 
     /* Advertise DFP (Decimal Floating Point) if available
@@ -3832,7 +3834,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
      */
     mc->numa_mem_align_shift = 28;
 
-    smc->default_caps = spapr_caps(0);
+    smc->default_caps = spapr_caps(SPAPR_CAP_VSX);
     spapr_caps_add_properties(smc, &error_abort);
 }
 
@@ -3914,7 +3916,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);
+    smc->default_caps = spapr_caps(SPAPR_CAP_HTM | SPAPR_CAP_VSX);
     SET_MACHINE_COMPAT(mc, SPAPR_COMPAT_2_11);
 }
 
diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
index a8c726a88b..da066aec8f 100644
--- a/hw/ppc/spapr_caps.c
+++ b/hw/ppc/spapr_caps.c
@@ -57,6 +57,19 @@ static void cap_htm_allow(sPAPRMachineState *spapr, Error **errp)
     }
 }
 
+static void cap_vsx_allow(sPAPRMachineState *spapr, Error **errp)
+{
+    PowerPCCPU *cpu = POWERPC_CPU(first_cpu);
+    CPUPPCState *env = &cpu->env;
+
+    /* 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);
+    if (!(env->insns_flags2 & PPC2_VSX)) {
+        error_setg(errp, "VSX support not available, try cap-vsx=off");
+    }
+}
+
 static sPAPRCapabilityInfo capability_table[] = {
     {
         .name = "htm",
@@ -65,6 +78,13 @@ static sPAPRCapabilityInfo capability_table[] = {
         .allow = cap_htm_allow,
         /* TODO: add cap_htm_disallow */
     },
+    {
+        .name = "vsx",
+        .description = "Allow Vector Scalar Extensions (VSX)",
+        .flag = SPAPR_CAP_VSX,
+        .allow = cap_vsx_allow,
+        /* TODO: add cap_vsx_disallow */
+    },
 };
 
 static sPAPRCapabilities default_caps_with_cpu(sPAPRMachineState *spapr,
@@ -81,6 +101,11 @@ static sPAPRCapabilities default_caps_with_cpu(sPAPRMachineState *spapr,
         caps.mask &= ~SPAPR_CAP_HTM;
     }
 
+    if (!ppc_check_compat(cpu, CPU_POWERPC_LOGICAL_2_06,
+                          0, spapr->max_compat_pvr)) {
+        caps.mask &= ~SPAPR_CAP_VSX;
+    }
+
     return caps;
 }
 
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 5c85f39c3b..148a03d189 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -59,6 +59,9 @@ typedef enum {
 /* Hardware Transactional Memory */
 #define SPAPR_CAP_HTM               0x0000000000000001ULL
 
+/* Vector Scalar Extensions */
+#define SPAPR_CAP_VSX               0x0000000000000002ULL
+
 typedef struct sPAPRCapabilities sPAPRCapabilities;
 struct sPAPRCapabilities {
     uint64_t mask;
-- 
2.14.3

Re: [Qemu-devel] [PATCH 5/6] spapr: Handle VMX/VSX presence as an spapr capability flag

[Greg Kurz]

On Mon, 18 Dec 2017 20:20:23 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> We currently have some conditionals in the spapr device tree code to decide
> whether or not to advertise the availability of the VMX (aka Altivec) and
> VSX vector extensions to the guest, based on whether the guest cpu has
> those features.
> 
> This can lead to confusion and subtle failures on migration, since it makes
> a guest visible change based only on host capabilities.  We now have a
> better mechanism for this, in spapr capabilities flags, which explicitly
> depend on user options rather than host capabilities.
> 
> Rework the advertisement of VSX and VMX based on a new VSX capability.  We
> no longer bother with a conditional for VMX support, because every CPU
> that's ever been supported by the pseries machine type supports VMX.
> 
> NOTE: Some userspace distributions (e.g. RHEL7.4) already rely on
> availability of VSX in libc, so using cap-vsx=off may lead to a fatal
> SIGILL in init.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---

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

>  hw/ppc/spapr.c         | 20 +++++++++++---------
>  hw/ppc/spapr_caps.c    | 25 +++++++++++++++++++++++++
>  include/hw/ppc/spapr.h |  3 +++
>  3 files changed, 39 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 86fc83f9c2..693dd6f7b3 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -557,14 +557,16 @@ static void spapr_populate_cpu_dt(CPUState *cs, void *fdt, int offset,
>                            segs, sizeof(segs))));
>      }
>  
> -    /* Advertise VMX/VSX (vector extensions) if available
> -     *   0 / no property == no vector extensions
> +    /* Advertise VSX (vector extensions) if available
>       *   1               == VMX / Altivec available
> -     *   2               == VSX available */
> -    if (env->insns_flags & PPC_ALTIVEC) {
> -        uint32_t vmx = (env->insns_flags2 & PPC2_VSX) ? 2 : 1;
> -
> -        _FDT((fdt_setprop_cell(fdt, offset, "ibm,vmx", vmx)));
> +     *   2               == VSX available
> +     *
> +     * 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)) {
> +        _FDT((fdt_setprop_cell(fdt, offset, "ibm,vmx", 2)));
> +    } else {
> +        _FDT((fdt_setprop_cell(fdt, offset, "ibm,vmx", 1)));
>      }
>  
>      /* Advertise DFP (Decimal Floating Point) if available
> @@ -3832,7 +3834,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
>       */
>      mc->numa_mem_align_shift = 28;
>  
> -    smc->default_caps = spapr_caps(0);
> +    smc->default_caps = spapr_caps(SPAPR_CAP_VSX);
>      spapr_caps_add_properties(smc, &error_abort);
>  }
>  
> @@ -3914,7 +3916,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);
> +    smc->default_caps = spapr_caps(SPAPR_CAP_HTM | SPAPR_CAP_VSX);
>      SET_MACHINE_COMPAT(mc, SPAPR_COMPAT_2_11);
>  }
>  
> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
> index a8c726a88b..da066aec8f 100644
> --- a/hw/ppc/spapr_caps.c
> +++ b/hw/ppc/spapr_caps.c
> @@ -57,6 +57,19 @@ static void cap_htm_allow(sPAPRMachineState *spapr, Error **errp)
>      }
>  }
>  
> +static void cap_vsx_allow(sPAPRMachineState *spapr, Error **errp)
> +{
> +    PowerPCCPU *cpu = POWERPC_CPU(first_cpu);
> +    CPUPPCState *env = &cpu->env;
> +
> +    /* 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);
> +    if (!(env->insns_flags2 & PPC2_VSX)) {
> +        error_setg(errp, "VSX support not available, try cap-vsx=off");
> +    }
> +}
> +
>  static sPAPRCapabilityInfo capability_table[] = {
>      {
>          .name = "htm",
> @@ -65,6 +78,13 @@ static sPAPRCapabilityInfo capability_table[] = {
>          .allow = cap_htm_allow,
>          /* TODO: add cap_htm_disallow */
>      },
> +    {
> +        .name = "vsx",
> +        .description = "Allow Vector Scalar Extensions (VSX)",
> +        .flag = SPAPR_CAP_VSX,
> +        .allow = cap_vsx_allow,
> +        /* TODO: add cap_vsx_disallow */
> +    },
>  };
>  
>  static sPAPRCapabilities default_caps_with_cpu(sPAPRMachineState *spapr,
> @@ -81,6 +101,11 @@ static sPAPRCapabilities default_caps_with_cpu(sPAPRMachineState *spapr,
>          caps.mask &= ~SPAPR_CAP_HTM;
>      }
>  
> +    if (!ppc_check_compat(cpu, CPU_POWERPC_LOGICAL_2_06,
> +                          0, spapr->max_compat_pvr)) {
> +        caps.mask &= ~SPAPR_CAP_VSX;
> +    }
> +
>      return caps;
>  }
>  
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 5c85f39c3b..148a03d189 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -59,6 +59,9 @@ typedef enum {
>  /* Hardware Transactional Memory */
>  #define SPAPR_CAP_HTM               0x0000000000000001ULL
>  
> +/* Vector Scalar Extensions */
> +#define SPAPR_CAP_VSX               0x0000000000000002ULL
> +
>  typedef struct sPAPRCapabilities sPAPRCapabilities;
>  struct sPAPRCapabilities {
>      uint64_t mask;

[Qemu-devel] [PATCH 6/6] spapr: Handle Decimal Floating Point (DFP) as an optional capability

[David Gibson]

Decimal Floating Point has been available on POWER7 and later (server)
cpus.  However, it can be disabled on the hypervisor, meaning that it's
not available to guests.

We currently handle this by conditionally advertising DFP support in the
device tree depending on whether the guest CPU model supports it - which
can also depend on what's allowed in the host for -cpu host.  That can lead
to confusion on migration, since host properties are silently affecting
guest visible properties.

This patch handles it by treating it as an optional capability for the
pseries machine type.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr.c         |  7 ++++---
 hw/ppc/spapr_caps.c    | 18 ++++++++++++++++++
 include/hw/ppc/spapr.h |  3 +++
 3 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 693dd6f7b3..e22888ba06 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -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 (env->insns_flags2 & PPC2_DFP) {
+    if (spapr_has_cap(spapr, SPAPR_CAP_DFP)) {
         _FDT((fdt_setprop_cell(fdt, offset, "ibm,dfp", 1)));
     }
 
@@ -3834,7 +3834,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
      */
     mc->numa_mem_align_shift = 28;
 
-    smc->default_caps = spapr_caps(SPAPR_CAP_VSX);
+    smc->default_caps = spapr_caps(SPAPR_CAP_VSX | SPAPR_CAP_DFP);
     spapr_caps_add_properties(smc, &error_abort);
 }
 
@@ -3916,7 +3916,8 @@ 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);
+    smc->default_caps = spapr_caps(SPAPR_CAP_HTM | SPAPR_CAP_VSX
+                                   | SPAPR_CAP_DFP);
     SET_MACHINE_COMPAT(mc, SPAPR_COMPAT_2_11);
 }
 
diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
index da066aec8f..61745f0b32 100644
--- a/hw/ppc/spapr_caps.c
+++ b/hw/ppc/spapr_caps.c
@@ -70,6 +70,16 @@ static void cap_vsx_allow(sPAPRMachineState *spapr, Error **errp)
     }
 }
 
+static void cap_dfp_allow(sPAPRMachineState *spapr, Error **errp)
+{
+    PowerPCCPU *cpu = POWERPC_CPU(first_cpu);
+    CPUPPCState *env = &cpu->env;
+
+    if (!(env->insns_flags2 & PPC2_DFP)) {
+        error_setg(errp, "DFP support not available, try cap-dfp=off");
+    }
+}
+
 static sPAPRCapabilityInfo capability_table[] = {
     {
         .name = "htm",
@@ -85,6 +95,13 @@ static sPAPRCapabilityInfo capability_table[] = {
         .allow = cap_vsx_allow,
         /* TODO: add cap_vsx_disallow */
     },
+    {
+        .name = "dfp",
+        .description = "Allow Decimal Floating Point (DFP)",
+        .flag = SPAPR_CAP_DFP,
+        .allow = cap_dfp_allow,
+        /* TODO: add cap_dfp_disallow */
+    },
 };
 
 static sPAPRCapabilities default_caps_with_cpu(sPAPRMachineState *spapr,
@@ -104,6 +121,7 @@ static sPAPRCapabilities default_caps_with_cpu(sPAPRMachineState *spapr,
     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;
     }
 
     return caps;
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 148a03d189..26ac17e641 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -62,6 +62,9 @@ typedef enum {
 /* Vector Scalar Extensions */
 #define SPAPR_CAP_VSX               0x0000000000000002ULL
 
+/* Decimal Floating Point */
+#define SPAPR_CAP_DFP               0x0000000000000004ULL
+
 typedef struct sPAPRCapabilities sPAPRCapabilities;
 struct sPAPRCapabilities {
     uint64_t mask;
-- 
2.14.3

Re: [Qemu-devel] [PATCH 6/6] spapr: Handle Decimal Floating Point (DFP) as an optional capability

[Greg Kurz]

On Mon, 18 Dec 2017 20:20:24 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> Decimal Floating Point has been available on POWER7 and later (server)
> cpus.  However, it can be disabled on the hypervisor, meaning that it's
> not available to guests.
> 
> We currently handle this by conditionally advertising DFP support in the
> device tree depending on whether the guest CPU model supports it - which
> can also depend on what's allowed in the host for -cpu host.  That can lead
> to confusion on migration, since host properties are silently affecting
> guest visible properties.
> 
> This patch handles it by treating it as an optional capability for the
> pseries machine type.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---

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

>  hw/ppc/spapr.c         |  7 ++++---
>  hw/ppc/spapr_caps.c    | 18 ++++++++++++++++++
>  include/hw/ppc/spapr.h |  3 +++
>  3 files changed, 25 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 693dd6f7b3..e22888ba06 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -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 (env->insns_flags2 & PPC2_DFP) {
> +    if (spapr_has_cap(spapr, SPAPR_CAP_DFP)) {
>          _FDT((fdt_setprop_cell(fdt, offset, "ibm,dfp", 1)));
>      }
>  
> @@ -3834,7 +3834,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
>       */
>      mc->numa_mem_align_shift = 28;
>  
> -    smc->default_caps = spapr_caps(SPAPR_CAP_VSX);
> +    smc->default_caps = spapr_caps(SPAPR_CAP_VSX | SPAPR_CAP_DFP);
>      spapr_caps_add_properties(smc, &error_abort);
>  }
>  
> @@ -3916,7 +3916,8 @@ 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);
> +    smc->default_caps = spapr_caps(SPAPR_CAP_HTM | SPAPR_CAP_VSX
> +                                   | SPAPR_CAP_DFP);
>      SET_MACHINE_COMPAT(mc, SPAPR_COMPAT_2_11);
>  }
>  
> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
> index da066aec8f..61745f0b32 100644
> --- a/hw/ppc/spapr_caps.c
> +++ b/hw/ppc/spapr_caps.c
> @@ -70,6 +70,16 @@ static void cap_vsx_allow(sPAPRMachineState *spapr, Error **errp)
>      }
>  }
>  
> +static void cap_dfp_allow(sPAPRMachineState *spapr, Error **errp)
> +{
> +    PowerPCCPU *cpu = POWERPC_CPU(first_cpu);
> +    CPUPPCState *env = &cpu->env;
> +
> +    if (!(env->insns_flags2 & PPC2_DFP)) {
> +        error_setg(errp, "DFP support not available, try cap-dfp=off");
> +    }
> +}
> +
>  static sPAPRCapabilityInfo capability_table[] = {
>      {
>          .name = "htm",
> @@ -85,6 +95,13 @@ static sPAPRCapabilityInfo capability_table[] = {
>          .allow = cap_vsx_allow,
>          /* TODO: add cap_vsx_disallow */
>      },
> +    {
> +        .name = "dfp",
> +        .description = "Allow Decimal Floating Point (DFP)",
> +        .flag = SPAPR_CAP_DFP,
> +        .allow = cap_dfp_allow,
> +        /* TODO: add cap_dfp_disallow */
> +    },
>  };
>  
>  static sPAPRCapabilities default_caps_with_cpu(sPAPRMachineState *spapr,
> @@ -104,6 +121,7 @@ static sPAPRCapabilities default_caps_with_cpu(sPAPRMachineState *spapr,
>      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;
>      }
>  
>      return caps;
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 148a03d189..26ac17e641 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -62,6 +62,9 @@ typedef enum {
>  /* Vector Scalar Extensions */
>  #define SPAPR_CAP_VSX               0x0000000000000002ULL
>  
> +/* Decimal Floating Point */
> +#define SPAPR_CAP_DFP               0x0000000000000004ULL
> +
>  typedef struct sPAPRCapabilities sPAPRCapabilities;
>  struct sPAPRCapabilities {
>      uint64_t mask;

Re: [Qemu-devel] [PATCH 0/6] spapr: Add optional capabilities

[David Gibson]

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

On Mon, Dec 18, 2017 at 08:20:18PM +1100, David Gibson wrote:
> This series is a first draft to add the notion of optional
> capabilities to the "pseries" machine type.  A default set of
> capabilities is selected based on the machine type version and
> selected cpu model, but this can be overridden with machine
> parameters.
> 
> The purpose of this is to get rid of a number of places where we
> implicitly decide what features to advertise to the guest based on
> capabilities of the host.  This is bad, because it means it's
> difficult to be certain if machines started at different ends of a
> migration really match from the guest's point of view.
> 
> By giving the user explicit control of these optional features, then
> validating that the chosen ones can be supplied on the host we make
> behaviour more predictable.
> 
> The more specific motivation for this is that POWER9 has bugs in its
> hardware transactional memory (HTM) implementation making it unsafe to
> migrate POWER8 guests to POWER9 if they use HTM.

I've fixed the user-after-free bug Greg pointed out and merged this
into ppc-for-2.12.

-- 
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] [PATCH 0/6] spapr: Add optional capabilities

[Benjamin Herrenschmidt]

On Mon, 2017-12-18 at 20:20 +1100, David Gibson wrote:
> This series is a first draft to add the notion of optional
> capabilities to the "pseries" machine type.  A default set of
> capabilities is selected based on the machine type version and
> selected cpu model, but this can be overridden with machine
> parameters.
> 
> The purpose of this is to get rid of a number of places where we
> implicitly decide what features to advertise to the guest based on
> capabilities of the host.  This is bad, because it means it's
> difficult to be certain if machines started at different ends of a
> migration really match from the guest's point of view.
> 
> By giving the user explicit control of these optional features, then
> validating that the chosen ones can be supplied on the host we make
> behaviour more predictable.
> 
> The more specific motivation for this is that POWER9 has bugs in its
> hardware transactional memory (HTM) implementation making it unsafe to
> migrate POWER8 guests to POWER9 if they use HTM

It might be worthwhile mentioning that we do plan to support TM and
migration from P8 for KVM guests at some point. Working on it ... there
are some SW workarounds involved in KVM to make it possible.

> Changes since RFC:
>  - Two preliminary patches not really related split off and already
>    merged.
>  - Assorted minor fixes based on review notes
>  - Change defaults to disable HTM on pseries-2.12 machine type
> 
> David Gibson (6):
>   spapr: Capabilities infrastructure
>   spapr: Treat Hardware Transactional Memory (HTM) as an optional
>     capability
>   spapr: Validate capabilities on migration
>   target/ppc: Clean up probing of VMX, VSX and DFP availability on KVM
>   spapr: Handle VMX/VSX presence as an spapr capability flag
>   spapr: Handle Decimal Floating Point (DFP) as an optional capability
> 
>  hw/ppc/Makefile.objs   |   2 +-
>  hw/ppc/spapr.c         |  47 +++++--
>  hw/ppc/spapr_caps.c    | 342 +++++++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/ppc/spapr.h |  46 +++++++
>  target/ppc/kvm.c       |  27 +---
>  target/ppc/kvm_ppc.h   |   2 -
>  6 files changed, 429 insertions(+), 37 deletions(-)
>  create mode 100644 hw/ppc/spapr_caps.c
>