| Message ID | 1305206625-28982-1-git-send-email-avi@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, May 12, 2011 at 09:23:45AM -0400, Avi Kivity wrote: > Instead of blacklisting known-unsupported cpuid leaves, whitelist known- > supported leaves. This is more conservative and prevents us from reporting > features we don't support. Also whitelist a few more leaves while at it. Good improvement, some small annotations inline. > > Signed-off-by: Avi Kivity <avi@redhat.com> > --- > > Joerg, if you can review the AMD bits... I was somewhat dazed and confused > when I got there (but tried to continue). > > arch/x86/kvm/x86.c | 37 +++++++++++++++++++++++++++++++++++-- > 1 files changed, 35 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 77c9d867..78149d3 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -2283,6 +2283,13 @@ static void do_cpuid_1_ent(struct kvm_cpuid_entry2 *entry, u32 function, > entry->flags = 0; > } > > +static bool supported_xcr0_bit(unsigned bit) > +{ > + u64 mask = ((u64)1 << bit); > + > + return mask & (XSTATE_FP | XSTATE_SSE | XSTATE_YMM) & host_xcr0; > +} > + > #define F(x) bit(X86_FEATURE_##x) > > static void do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function, > @@ -2393,6 +2400,8 @@ static void do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function, > } > break; > } > + case 9: > + break; > case 0xb: { > int i, level_type; > > @@ -2414,7 +2423,7 @@ static void do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function, > > entry->flags |= KVM_CPUID_FLAG_SIGNIFCANT_INDEX; > for (i = 1; *nent < maxnent && i < 64; ++i) { > - if (entry[i].eax == 0) > + if (entry[i].eax == 0 || !supported_xcr0_bit(i)) > continue; > do_cpuid_1_ent(&entry[i], function, i); > entry[i].flags |= > @@ -2451,6 +2460,24 @@ static void do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function, > entry->ecx &= kvm_supported_word6_x86_features; > cpuid_mask(&entry->ecx, 6); > break; > + case 0x80000008: { > + u8 g_phys_as = entry->eax >> 16; > + u8 virt_as = max(entry->eax >> 8, 48U); Shouldn't that be 'max((entry->eax >> 8) & 0xff, 48U)'? Seems safer when the entry->eax contains a non-zero g_phys value. > + u8 phys_as = entry->eax; > + > + if (!g_phys_as) > + g_phys_as = phys_as; > + entry->eax = g_phys_as | (virt_as << 8); It is optional, but since we support nesting we can also encode g_phys_as in bits 23:16. > + entry->ebx = entry->edx = 0; > + break; > + } > + case 0x80000019: > + entry->ecx = entry->edx = 0; > + break; > + case 0x8000001a: > + break; > + case 0x8000001d: > + break; > /*Add support for Centaur's CPUID instruction*/ > case 0xC0000000: > /*Just support up to 0xC0000004 now*/ > @@ -2460,10 +2487,16 @@ static void do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function, > entry->edx &= kvm_supported_word5_x86_features; > cpuid_mask(&entry->edx, 5); > break; > + case 3: /* Processor serial number */ > + case 5: /* MONITOR/MWAIT */ > + case 6: /* Thermal management */ > + case 0xA: /* Architectural Performance Monitoring */ > + case 0x80000007: /* Advanced power management */ > case 0xC0000002: > case 0xC0000003: > case 0xC0000004: > - /*Now nothing to do, reserved for the future*/ > + default: > + entry->eax = entry->ebx = entry->ecx = entry->edx = 0; > break; > } > > -- > 1.7.4.3 > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/18/2011 10:19 AM, Roedel, Joerg wrote: > > break; > > + case 0x80000008: { > > + u8 g_phys_as = entry->eax>> 16; > > + u8 virt_as = max(entry->eax>> 8, 48U); > > Shouldn't that be 'max((entry->eax>> 8)& 0xff, 48U)'? Seems safer when > the entry->eax contains a non-zero g_phys value. Yes, this is a bug. I originally had 'u8 virt_as = entry->eax >> 8', relying on the cast to u8, but missed it when updating. Moral: don't be subtle. > > + u8 phys_as = entry->eax; > > + > > + if (!g_phys_as) > > + g_phys_as = phys_as; > > + entry->eax = g_phys_as | (virt_as<< 8); > > It is optional, but since we support nesting we can also encode > g_phys_as in bits 23:16. I'm relying on a zero value in 23:16 indicating you should use 7:0. Thanks for the review, will post an updated patch.
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 77c9d867..78149d3 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -2283,6 +2283,13 @@ static void do_cpuid_1_ent(struct kvm_cpuid_entry2 *entry, u32 function, entry->flags = 0; } +static bool supported_xcr0_bit(unsigned bit) +{ + u64 mask = ((u64)1 << bit); + + return mask & (XSTATE_FP | XSTATE_SSE | XSTATE_YMM) & host_xcr0; +} + #define F(x) bit(X86_FEATURE_##x) static void do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function, @@ -2393,6 +2400,8 @@ static void do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function, } break; } + case 9: + break; case 0xb: { int i, level_type; @@ -2414,7 +2423,7 @@ static void do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function, entry->flags |= KVM_CPUID_FLAG_SIGNIFCANT_INDEX; for (i = 1; *nent < maxnent && i < 64; ++i) { - if (entry[i].eax == 0) + if (entry[i].eax == 0 || !supported_xcr0_bit(i)) continue; do_cpuid_1_ent(&entry[i], function, i); entry[i].flags |= @@ -2451,6 +2460,24 @@ static void do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function, entry->ecx &= kvm_supported_word6_x86_features; cpuid_mask(&entry->ecx, 6); break; + case 0x80000008: { + u8 g_phys_as = entry->eax >> 16; + u8 virt_as = max(entry->eax >> 8, 48U); + u8 phys_as = entry->eax; + + if (!g_phys_as) + g_phys_as = phys_as; + entry->eax = g_phys_as | (virt_as << 8); + entry->ebx = entry->edx = 0; + break; + } + case 0x80000019: + entry->ecx = entry->edx = 0; + break; + case 0x8000001a: + break; + case 0x8000001d: + break; /*Add support for Centaur's CPUID instruction*/ case 0xC0000000: /*Just support up to 0xC0000004 now*/ @@ -2460,10 +2487,16 @@ static void do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function, entry->edx &= kvm_supported_word5_x86_features; cpuid_mask(&entry->edx, 5); break; + case 3: /* Processor serial number */ + case 5: /* MONITOR/MWAIT */ + case 6: /* Thermal management */ + case 0xA: /* Architectural Performance Monitoring */ + case 0x80000007: /* Advanced power management */ case 0xC0000002: case 0xC0000003: case 0xC0000004: - /*Now nothing to do, reserved for the future*/ + default: + entry->eax = entry->ebx = entry->ecx = entry->edx = 0; break; }
Instead of blacklisting known-unsupported cpuid leaves, whitelist known- supported leaves. This is more conservative and prevents us from reporting features we don't support. Also whitelist a few more leaves while at it. Signed-off-by: Avi Kivity <avi@redhat.com> --- Joerg, if you can review the AMD bits... I was somewhat dazed and confused when I got there (but tried to continue). arch/x86/kvm/x86.c | 37 +++++++++++++++++++++++++++++++++++-- 1 files changed, 35 insertions(+), 2 deletions(-)