Re: [Qemu-devel] [PATCH v3 1/2] target-i386: KVM: add basic Intel LMCE support

[Haozhong Zhang]

On 06/04/16 18:03, Eduardo Habkost wrote:
> On Sat, Jun 04, 2016 at 12:34:39PM +0200, Boris Petkov wrote:
> > Haozhong Zhang <address@hidden> wrote:
> > 
> > >This patch adds the support to inject SRAR and SRAO as LMCE, i.e. they
> > >will be injected to only one VCPU rather than broadcast to all
> > >VCPUs. As KVM reports LMCE support on Intel platforms, this features is
> > >only available on Intel platforms.
> > >
> > >Signed-off-by: Ashok Raj <address@hidden>
> > >Signed-off-by: Haozhong Zhang <address@hidden>
> > >---
> > >Cc: Paolo Bonzini <address@hidden>
> > >Cc: Richard Henderson <address@hidden>
> > >Cc: Eduardo Habkost <address@hidden>
> > >Cc: Marcelo Tosatti <address@hidden>
> > >Cc: Boris Petkov <address@hidden>
> > >Cc: address@hidden
> > >Cc: Tony Luck <address@hidden>
> > >Cc: Andi Kleen <address@hidden>
> > >---
> > > target-i386/cpu.c | 26 ++++++++++++++++++++++++++
> > > target-i386/cpu.h | 13 ++++++++++++-
> > > target-i386/kvm.c | 35 +++++++++++++++++++++++++++++++----
> > > 3 files changed, 69 insertions(+), 5 deletions(-)
> > 
> > ...
> > 
> > >@@ -1173,6 +1182,8 @@ struct X86CPU {
> > >      */
> > >     bool enable_pmu;
> > > 
> > >+    bool enable_lmce;
> > 
> > That struct would go fat pretty fast if it grows a bool per CPU
> > feature. Perhaps a more clever, a-bit-per-featurebit scheme
> > would be in order.
> 
> We already have X86CPU.features, but it's specific for feature
> flags appearing on CPUID. We can eventually extend
> FeatureWord/FeatureWordInfo/x86_cpu_get_supported_feature_word()
> to represent features that don't appear directly on CPUID.
>

You mean CPUX86State.features?

enable_lmce is also used in patch 2 to avoid migrating from
lmce-enabled qemu to lmce-disabled qemu, so I don't think
CPUX86State.features is the correct place for enable_lmce.

Or, we may append one or more bit words to X86CPU.filtered_features
for enable_pmu, enable_lmce and future features, e.g.

modified   target-i386/cpu.h
@@ -455,6 +455,14 @@ typedef enum FeatureWord {
 
 typedef uint32_t FeatureWordArray[FEATURE_WORDS];
 
+typedef enum ExtFeatureBit {
+    EXT_FEAT_PMU,
+    EXT_FEAT_LMCE,
+    EXT_FEATURE_BITS,
+} ExtFeatureBit;
+
+#define EXT_FEATURE_WORDS ((EXT_FEATURE_BITS + 31) / 32)
+
+#define EXT_FEATURE_FILTER_ADD(words, feat)             \
+    do {                                                \
+        uint32_t *__words = (words);                    \
+        int __idx = (feat) / 32;                        \
+        int __oft = (feat) % 32;                        \
+        __words[__idx + FEATURE_WORDS] |= (1 << __oft); \
+    } while (0)
+
+#define EXT_FEATURE_FILTER_REMOVE(words, feat)           \
+    do {                                                 \
+        uint32_t *__words = (words);                     \
+        int __idx = (feat) / 32;                         \
+        int __oft = (feat) % 32;                         \
+        __words[__idx + FEATURE_WORDS] &= ~(1 << __oft); \
+    } while (0)
+
+#define EXT_FEATURE_FILTERED(words, feat)           \
+    ({                                              \
+        uint32_t *__words = (words);                \
+        int __idx = (feat) / 32;                    \
+        int __oft = (feat) % 32;                    \
+        __words[__idx + FEATURE_WORDS] & (1 << oft) \
+    })
+
 /* cpuid_features bits */
 #define CPUID_FP87 (1U << 0)
 #define CPUID_VME  (1U << 1)
@@ -1173,21 +1181,7 @@ struct X86CPU {
     /* Features that were filtered out because of missing host capabilities */
-    uint32_t filtered_features[FEATURE_WORDS];
+    /* Features that were filtered out because of missing host capabilities or
+       being disabled by default */
+    uint32_t filtered_features[FEATURE_WORDS + EXT_FEATURE_WORDS];
-
-    /* Enable PMU CPUID bits. This can't be enabled by default yet because
-     * it doesn't have ABI stability guarantees, as it passes all PMU CPUID
-     * bits returned by GET_SUPPORTED_CPUID (that depend on host CPU and kernel
-     * capabilities) directly to the guest.
-     */
-    bool enable_pmu;
-
-    /* Enable LMCE support which is set via cpu option 'lmce=on/off'. LMCE is
-     * disabled by default to avoid breaking the migration between QEMU with
-     * different LMCE support. Only migrating between QEMU with the same LMCE
-     * support is allowed.
-     */
-    bool enable_lmce;

Every place using X86CPU.enable_pmu and .enable_lmce is then replaced
by above macros EXT_FEATURE_FILTER_ADD, EXT_FEATURE_FILTER_REMOVE and
EXT_FEATURE_FILTERED. Of course, those bits in X86CPU.filtered_features
have the opposite meaning to original enable_lmce and enable_pmu.


Haozhong