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

[Haozhong Zhang]

On 06/07/16 17:41, Haozhong Zhang wrote:
> 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.
>

Sorry, I got things wrong: I thought that CPUX86State.features on
destination is overwritten by the source one in the migration. As long
as it's in fact not, we may extend CPUX86State.features in a similar
way to I proposed for X86CPU.filtered_features to include features not
directly appear in cpuid.

Haozhong

> 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
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to address@hidden
> More majordomo info at  http://vger.kernel.org/majordomo-info.html