[PATCH v3 1/2] qdev-properties: Add a new macro to validate bitmask for setter

[PATCH v3 1/2] qdev-properties: Add a new macro to validate bitmask for setter

[Like Xu]

The new generic DEFINE_PROP_BITMASK_UINT64 could be used to ensure
that a user-provided property value complies with its bitmask rule
and the default value is recommended to be set in instance_init().

Signed-off-by: Like Xu <like.xu@linux.intel.com>
---
 hw/core/qdev-properties.c    | 19 +++++++++++++++++++
 include/hw/qdev-properties.h | 12 ++++++++++++
 include/qapi/qmp/qerror.h    |  3 +++
 3 files changed, 34 insertions(+)

diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index 50f40949f5..3784d3b30d 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -428,6 +428,25 @@ const PropertyInfo qdev_prop_int64 = {
     .set_default_value = qdev_propinfo_set_default_value_int,
 };
 
+static void set_bitmask_uint64(Object *obj, Visitor *v, const char *name,
+                      void *opaque, Error **errp)
+{
+    Property *prop = opaque;
+    uint64_t *ptr = object_field_prop_ptr(obj, prop);
+
+    visit_type_uint64(v, name, ptr, errp);
+
+    if (*ptr & ~prop->bitmask) {
+        error_setg(errp, QERR_INVALID_BITMASK_VALUE, name, prop->bitmask);
+    }
+}
+
+const PropertyInfo qdev_prop_bitmask_uint64 = {
+    .name  = "int64",
+    .get   = get_uint64,
+    .set   = set_bitmask_uint64,
+};
+
 /* --- string --- */
 
 static void release_string(Object *obj, const char *name, void *opaque)
diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
index 0ef97d60ce..42f0112e14 100644
--- a/include/hw/qdev-properties.h
+++ b/include/hw/qdev-properties.h
@@ -17,6 +17,7 @@ struct Property {
     const PropertyInfo *info;
     ptrdiff_t    offset;
     uint8_t      bitnr;
+    uint64_t     bitmask;
     bool         set_default;
     union {
         int64_t i;
@@ -53,6 +54,7 @@ extern const PropertyInfo qdev_prop_uint16;
 extern const PropertyInfo qdev_prop_uint32;
 extern const PropertyInfo qdev_prop_int32;
 extern const PropertyInfo qdev_prop_uint64;
+extern const PropertyInfo qdev_prop_bitmask_uint64;
 extern const PropertyInfo qdev_prop_int64;
 extern const PropertyInfo qdev_prop_size;
 extern const PropertyInfo qdev_prop_string;
@@ -102,6 +104,16 @@ extern const PropertyInfo qdev_prop_link;
                 .set_default = true,                         \
                 .defval.u    = (bool)_defval)
 
+/**
+ * The DEFINE_PROP_BITMASK_UINT64 could be used to ensure that
+ * a user-provided value complies with certain bitmask rule and
+ * the default value is recommended to be set in instance_init().
+ */
+#define DEFINE_PROP_BITMASK_UINT64(_name, _state, _field, _bitmask)   \
+    DEFINE_PROP(_name, _state, _field, qdev_prop_bitmask_uint64, uint64_t, \
+                .bitmask    = (_bitmask),                     \
+                .set_default = false)
+
 #define PROP_ARRAY_LEN_PREFIX "len-"
 
 /**
diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h
index 596fce0c54..aab7902760 100644
--- a/include/qapi/qmp/qerror.h
+++ b/include/qapi/qmp/qerror.h
@@ -68,4 +68,7 @@
 #define QERR_UNSUPPORTED \
     "this feature or command is not currently supported"
 
+#define QERR_INVALID_BITMASK_VALUE \
+    "the requested value for '%s' violates its bitmask '0x%lx'"
+
 #endif /* QERROR_H */
-- 
2.30.2

[PATCH v3 2/2] target/i386: add "-cpu, lbr-fmt=*" support to enable guest LBR

[Like Xu]

The last branch recording (LBR) is a performance monitor unit (PMU)
feature on Intel processors that records a running trace of the most
recent branches taken by the processor in the LBR stack. The QEMU
could configure whether it's enabled or not for each guest via CLI.

The LBR feature would be enabled on the guest if:
- the KVM is enabled and the PMU is enabled and,
- the msr-based-feature IA32_PERF_CAPABILITIES is supporterd on KVM and,
- the supported returned value for lbr_fmt from this msr is not zero and,
- the requested guest vcpu model does support FEAT_1_ECX.CPUID_EXT_PDCM,
- the user-provided lbr-fmt value should not violate its bitmask (0x3f)
  and it should be the same as the host lbr_fmt value or just use the
  QEMU option "-cpu host,migratable=no" to enable guest LBR.

Signed-off-by: Like Xu <like.xu@linux.intel.com>
---
v2-v3 Changelog:
- Add a new generic property macro to validate its bitmask;
- Differentiate "lbr-fmt=0" from "lbr-fmt not set";
- Do what the user asked for whenever possible;
- Treat mismatch or violatation as an error rather than warning;

Testcases for a lbr-fmt=5 host:

 "-cpu host" --> "Disable LBR"
 "-cpu host,lbr-fmt=0" --> "Disable LBR"
 "-cpu host,lbr-fmt=5" --> "Enable LBR"
 "-cpu host,lbr-fmt=6" --> "Error out, lbr mismatch"
 "-cpu host,lbr-fmt=0xff" --> "Error out, bitmask violatation"
 "-cpu host,migratable=no" --> "Enable LBR"
 "-cpu host,migratable=no,lbr-fmt=0" --> "Disable LBR"
 "-cpu host,migratable=no,lbr-fmt=5" --> "Enable LBR"
 "-cpu host,migratable=no,lbr-fmt=6" --> "Error out, lbr mismatch"
 "-cpu host,migratable=no,lbr-fmt=0xff" --> "Error out, bitmask violatation"

 target/i386/cpu.c | 39 +++++++++++++++++++++++++++++++++++++++
 target/i386/cpu.h | 10 ++++++++++
 2 files changed, 49 insertions(+)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index ad99cad0e7..d03306179a 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -6748,6 +6748,41 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
         goto out;
     }
 
+    /*
+     * Override env->features[FEAT_PERF_CAPABILITIES]
+     * with explicit user-provided settings.
+     */
+    if (cpu->lbr_fmt != ~PERF_CAP_LBR_FMT) {
+        if ((cpu->lbr_fmt & PERF_CAP_LBR_FMT) != cpu->lbr_fmt) {
+            error_setg(errp, "invalid lbr-fmt");
+            return;
+        }
+        env->features[FEAT_PERF_CAPABILITIES] &= ~PERF_CAP_LBR_FMT;
+        env->features[FEAT_PERF_CAPABILITIES] |= cpu->lbr_fmt;
+    }
+
+    /*
+     * We can always validate env->features[FEAT_PERF_CAPABILITIES],
+     * no matter how it was initialized:
+     */
+    uint64_t requested_lbr_fmt =
+        env->features[FEAT_PERF_CAPABILITIES] & PERF_CAP_LBR_FMT;
+    if (requested_lbr_fmt && kvm_enabled()) {
+        uint64_t host_perf_cap =
+            x86_cpu_get_supported_feature_word(FEAT_PERF_CAPABILITIES, false);
+        uint64_t host_lbr_fmt = host_perf_cap & PERF_CAP_LBR_FMT;
+        if (!cpu->enable_pmu) {
+            error_setg(errp, "vPMU: LBR is unsupported without pmu=on");
+            return;
+        }
+        if (requested_lbr_fmt != host_lbr_fmt) {
+            error_setg(errp, "vPMU: the lbr-fmt value (0x%lx) mismatches "
+                        "the host supported value (0x%lx).",
+                        requested_lbr_fmt, host_lbr_fmt);
+            return;
+        }
+    }
+
     x86_cpu_filter_features(cpu, cpu->check_cpuid || cpu->enforce_cpuid);
 
     if (cpu->enforce_cpuid && x86_cpu_have_filtered_features(cpu)) {
@@ -7150,6 +7185,9 @@ static void x86_cpu_initfn(Object *obj)
     object_property_add_alias(obj, "sse4_1", obj, "sse4.1");
     object_property_add_alias(obj, "sse4_2", obj, "sse4.2");
 
+    cpu->lbr_fmt = ~PERF_CAP_LBR_FMT;
+    object_property_add_alias(obj, "lbr_fmt", obj, "lbr-fmt");
+
     if (xcc->model) {
         x86_cpu_load_model(cpu, xcc->model);
     }
@@ -7300,6 +7338,7 @@ static Property x86_cpu_properties[] = {
 #endif
     DEFINE_PROP_INT32("node-id", X86CPU, node_id, CPU_UNSET_NUMA_NODE_ID),
     DEFINE_PROP_BOOL("pmu", X86CPU, enable_pmu, false),
+    DEFINE_PROP_BITMASK_UINT64("lbr-fmt", X86CPU, lbr_fmt, PERF_CAP_LBR_FMT),
 
     DEFINE_PROP_UINT32("hv-spinlocks", X86CPU, hyperv_spinlock_attempts,
                        HYPERV_SPINLOCK_NEVER_NOTIFY),
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index 1bc300ce85..bab394e18e 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -354,6 +354,7 @@ typedef enum X86Seg {
 #define ARCH_CAP_TSX_CTRL_MSR		(1<<7)
 
 #define MSR_IA32_PERF_CAPABILITIES      0x345
+#define PERF_CAP_LBR_FMT                0x3f
 
 #define MSR_IA32_TSX_CTRL		0x122
 #define MSR_IA32_TSCDEADLINE            0x6e0
@@ -1726,6 +1727,15 @@ struct X86CPU {
      */
     bool enable_pmu;
 
+    /*
+     * Configure LBR_FMT bits on IA32_PERF_CAPABILITIES MSR.
+     * This can't be enabled by default yet because it doesn't have
+     * ABI stability guarantees, as it is only allowed to pass all
+     * LBR_FMT bits returned by kvm_arch_get_supported_msr_feature()
+     * (that depends on host CPU and kernel capabilities) to the guest.
+     */
+    uint64_t lbr_fmt;
+
     /* LMCE support can be enabled/disabled via cpu option 'lmce=on/off'. It is
      * disabled by default to avoid breaking migration between QEMU with
      * different LMCE configurations.
-- 
2.30.2

Re: [PATCH v3 1/2] qdev-properties: Add a new macro to validate bitmask for setter

[Markus Armbruster]

Like Xu <like.xu@linux.intel.com> writes:

> The new generic DEFINE_PROP_BITMASK_UINT64 could be used to ensure
> that a user-provided property value complies with its bitmask rule
> and the default value is recommended to be set in instance_init().
>
> Signed-off-by: Like Xu <like.xu@linux.intel.com>
> ---
>  hw/core/qdev-properties.c    | 19 +++++++++++++++++++
>  include/hw/qdev-properties.h | 12 ++++++++++++
>  include/qapi/qmp/qerror.h    |  3 +++
>  3 files changed, 34 insertions(+)
>
> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> index 50f40949f5..3784d3b30d 100644
> --- a/hw/core/qdev-properties.c
> +++ b/hw/core/qdev-properties.c
> @@ -428,6 +428,25 @@ const PropertyInfo qdev_prop_int64 = {
>      .set_default_value = qdev_propinfo_set_default_value_int,
>  };
>  
> +static void set_bitmask_uint64(Object *obj, Visitor *v, const char *name,
> +                      void *opaque, Error **errp)
> +{
> +    Property *prop = opaque;
> +    uint64_t *ptr = object_field_prop_ptr(obj, prop);
> +
> +    visit_type_uint64(v, name, ptr, errp);
> +
> +    if (*ptr & ~prop->bitmask) {
> +        error_setg(errp, QERR_INVALID_BITMASK_VALUE, name, prop->bitmask);
> +    }
> +}
> +
> +const PropertyInfo qdev_prop_bitmask_uint64 = {
> +    .name  = "int64",
> +    .get   = get_uint64,
> +    .set   = set_bitmask_uint64,
> +};
> +
>  /* --- string --- */
>  
>  static void release_string(Object *obj, const char *name, void *opaque)
> diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
> index 0ef97d60ce..42f0112e14 100644
> --- a/include/hw/qdev-properties.h
> +++ b/include/hw/qdev-properties.h
> @@ -17,6 +17,7 @@ struct Property {
>      const PropertyInfo *info;
>      ptrdiff_t    offset;
>      uint8_t      bitnr;
> +    uint64_t     bitmask;

The new member is used just for BITMASK properties.  Similar to how
.bitnr is used just for BIT properties, .link_type is just for LINK
properties, and the .arrayFOO are just for ARRAY properties.  I don't
like that, to be frank, but I'm not the maintainer.

>      bool         set_default;
>      union {
>          int64_t i;
> @@ -53,6 +54,7 @@ extern const PropertyInfo qdev_prop_uint16;
>  extern const PropertyInfo qdev_prop_uint32;
>  extern const PropertyInfo qdev_prop_int32;
>  extern const PropertyInfo qdev_prop_uint64;
> +extern const PropertyInfo qdev_prop_bitmask_uint64;
>  extern const PropertyInfo qdev_prop_int64;
>  extern const PropertyInfo qdev_prop_size;
>  extern const PropertyInfo qdev_prop_string;
> @@ -102,6 +104,16 @@ extern const PropertyInfo qdev_prop_link;
>                  .set_default = true,                         \
>                  .defval.u    = (bool)_defval)
>  
> +/**
> + * The DEFINE_PROP_BITMASK_UINT64 could be used to ensure that
> + * a user-provided value complies with certain bitmask rule and
> + * the default value is recommended to be set in instance_init().
> + */
> +#define DEFINE_PROP_BITMASK_UINT64(_name, _state, _field, _bitmask)   \
> +    DEFINE_PROP(_name, _state, _field, qdev_prop_bitmask_uint64, uint64_t, \
> +                .bitmask    = (_bitmask),                     \
> +                .set_default = false)
> +
>  #define PROP_ARRAY_LEN_PREFIX "len-"
>  
>  /**
> diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h
> index 596fce0c54..aab7902760 100644
> --- a/include/qapi/qmp/qerror.h
> +++ b/include/qapi/qmp/qerror.h
> @@ -68,4 +68,7 @@
>  #define QERR_UNSUPPORTED \
>      "this feature or command is not currently supported"
>  
> +#define QERR_INVALID_BITMASK_VALUE \
> +    "the requested value for '%s' violates its bitmask '0x%lx'"
> +
>  #endif /* QERROR_H */

Note the comment further up:

   /*
    * These macros will go away, please don't use in new code, and do not
    * add new ones!
    */

Please put the error message string into set_bitmask_uint64()'s
error_setg() call instead.