| Message ID | 1412365191-22858-6-git-send-email-ehabkost@redhat.com |
|---|---|
| State | New |
| Headers | show |
Am 03.10.2014 um 21:39 schrieb Eduardo Habkost: > TCG doesn't support VMX, and nested VMX is not enabled by default on the > KVM kernel module. > > So, there's no reason to have VMX enabled by default on the core2duo and > coreduo CPU models, today. Even the newer Intel CPU model definitions > don't have it enabled. > > In this case, we need machine-type compat code, as people may be running > the older machine-types on hosts that had VMX nesting enabled. > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > --- > hw/i386/pc_piix.c | 2 ++ > hw/i386/pc_q35.c | 2 ++ > target-i386/cpu.c | 8 ++++---- > 3 files changed, 8 insertions(+), 4 deletions(-) [...] > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > index 1e9fff9..c336003 100644 > --- a/target-i386/cpu.c > +++ b/target-i386/cpu.c > @@ -720,10 +720,10 @@ static X86CPUDefinition builtin_x86_defs[] = { > CPUID_MTRR | CPUID_CLFLUSH | CPUID_MCA | > CPUID_PSE36 | CPUID_VME | CPUID_ACPI | CPUID_SS, > /* Missing: CPUID_EXT_DTES64, CPUID_EXT_DSCPL, CPUID_EXT_EST, > - * CPUID_EXT_TM2, CPUID_EXT_XTPR, CPUID_EXT_PDCM */ > + * CPUID_EXT_TM2, CPUID_EXT_XTPR, CPUID_EXT_PDCM, CPUID_EXT_VMX */ > .features[FEAT_1_ECX] = > CPUID_EXT_SSE3 | CPUID_EXT_MONITOR | CPUID_EXT_SSSE3 | > - CPUID_EXT_VMX | CPUID_EXT_CX16, > + CPUID_EXT_CX16, > .features[FEAT_8000_0001_EDX] = > CPUID_EXT2_LM | CPUID_EXT2_SYSCALL | CPUID_EXT2_NX, > .features[FEAT_8000_0001_ECX] = [snip] Here I'm less certain what the best approach is. As you point out, there's an inconsistency that I agree should be fixed. I wonder however whether an approach similar to 3/6 for KVM only would be better? I.e., have VMX as a sometimes-KVM-supported feature be listed in the model and filter it out for accel=kvm so that -cpu enforce works, but let accel=tcg fail with features not implemented. Regards, Andreas
On Wed, Oct 29, 2014 at 06:40:48PM +0100, Andreas Färber wrote: > Am 03.10.2014 um 21:39 schrieb Eduardo Habkost: > > TCG doesn't support VMX, and nested VMX is not enabled by default on the > > KVM kernel module. > > > > So, there's no reason to have VMX enabled by default on the core2duo and > > coreduo CPU models, today. Even the newer Intel CPU model definitions > > don't have it enabled. > > > > In this case, we need machine-type compat code, as people may be running > > the older machine-types on hosts that had VMX nesting enabled. > > > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > > --- > > hw/i386/pc_piix.c | 2 ++ > > hw/i386/pc_q35.c | 2 ++ > > target-i386/cpu.c | 8 ++++---- > > 3 files changed, 8 insertions(+), 4 deletions(-) > [...] > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > > index 1e9fff9..c336003 100644 > > --- a/target-i386/cpu.c > > +++ b/target-i386/cpu.c > > @@ -720,10 +720,10 @@ static X86CPUDefinition builtin_x86_defs[] = { > > CPUID_MTRR | CPUID_CLFLUSH | CPUID_MCA | > > CPUID_PSE36 | CPUID_VME | CPUID_ACPI | CPUID_SS, > > /* Missing: CPUID_EXT_DTES64, CPUID_EXT_DSCPL, CPUID_EXT_EST, > > - * CPUID_EXT_TM2, CPUID_EXT_XTPR, CPUID_EXT_PDCM */ > > + * CPUID_EXT_TM2, CPUID_EXT_XTPR, CPUID_EXT_PDCM, CPUID_EXT_VMX */ > > .features[FEAT_1_ECX] = > > CPUID_EXT_SSE3 | CPUID_EXT_MONITOR | CPUID_EXT_SSSE3 | > > - CPUID_EXT_VMX | CPUID_EXT_CX16, > > + CPUID_EXT_CX16, > > .features[FEAT_8000_0001_EDX] = > > CPUID_EXT2_LM | CPUID_EXT2_SYSCALL | CPUID_EXT2_NX, > > .features[FEAT_8000_0001_ECX] = > [snip] > > Here I'm less certain what the best approach is. As you point out, > there's an inconsistency that I agree should be fixed. I wonder however > whether an approach similar to 3/6 for KVM only would be better? I.e., > have VMX as a sometimes-KVM-supported feature be listed in the model and > filter it out for accel=kvm so that -cpu enforce works, but let > accel=tcg fail with features not implemented. I don't mind either way, this is up for TCG maintainers. But I don't understand what would be the benefit in keeping it enabled by default for TCG. Fortunately the answer for KVM and TCG are completely independent from each other, now. We just need to follow this depending on the defaults we choose: | Default on | | TCG | KVM | Implementation +------+-----+------------------------------------------------- A | NO | NO | Unset it on CPU model table (builtin_x86_defs) B | NO | YES | Unset on table, set on kvm_default_features C | YES | NO | Set on table, set on kvm_default_unset_features B | YES | YES | Set it on CPU model table +------+-----+------------------------------------------------- This patch implements row A. If you tell me you really want VMX to be enabled by default on TCG mode, then we can implement row C like we already did for CPUID_ACPI, CPUID_EXT_MONITOR, and CPUID_EXT3_SVM. But, are you really sure you do? Considering accel=tcg behavior only (ignore KVM by now): why is this different from all the features listed on patch 1/6? VMX was never supported by TCG, so (like all the features from 1/6) it is not useful to have it enabled by default when accel=tcg.
> Here I'm less certain what the best approach is. As you point out, > there's an inconsistency that I agree should be fixed. I wonder however > whether an approach similar to 3/6 for KVM only would be better? I.e., > have VMX as a sometimes-KVM-supported feature be listed in the model and > filter it out for accel=kvm so that -cpu enforce works, but let > accel=tcg fail with features not implemented. This would mean that -cpu coreduo,enforce doesn't work on TCG, but -cpu Nehalem,enforce works. This does not make much sense to me. In fact, I would even omit the x86_cpu_compat_set_features altogether. The inclusion of vmx in these models was a mistake, and nested VMX is not really useful with anything but "-cpu host" because there are too many capabilities communicated via MSRs rather than CPUID. Paolo
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c index d2d34d7..1d5d57e 100644 --- a/hw/i386/pc_piix.c +++ b/hw/i386/pc_piix.c @@ -304,6 +304,8 @@ static void pc_init_pci(MachineState *machine) static void pc_compat_2_1(MachineState *machine) { + x86_cpu_compat_set_features("coreduo", FEAT_1_ECX, CPUID_EXT_VMX, 0); + x86_cpu_compat_set_features("core2duo", FEAT_1_ECX, CPUID_EXT_VMX, 0); } static void pc_compat_2_0(MachineState *machine) diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c index d06481c..c8f7a88 100644 --- a/hw/i386/pc_q35.c +++ b/hw/i386/pc_q35.c @@ -278,6 +278,8 @@ static void pc_q35_init(MachineState *machine) static void pc_compat_2_1(MachineState *machine) { + x86_cpu_compat_set_features("coreduo", FEAT_1_ECX, CPUID_EXT_VMX, 0); + x86_cpu_compat_set_features("core2duo", FEAT_1_ECX, CPUID_EXT_VMX, 0); } static void pc_compat_2_0(MachineState *machine) diff --git a/target-i386/cpu.c b/target-i386/cpu.c index 1e9fff9..c336003 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -720,10 +720,10 @@ static X86CPUDefinition builtin_x86_defs[] = { CPUID_MTRR | CPUID_CLFLUSH | CPUID_MCA | CPUID_PSE36 | CPUID_VME | CPUID_ACPI | CPUID_SS, /* Missing: CPUID_EXT_DTES64, CPUID_EXT_DSCPL, CPUID_EXT_EST, - * CPUID_EXT_TM2, CPUID_EXT_XTPR, CPUID_EXT_PDCM */ + * CPUID_EXT_TM2, CPUID_EXT_XTPR, CPUID_EXT_PDCM, CPUID_EXT_VMX */ .features[FEAT_1_ECX] = CPUID_EXT_SSE3 | CPUID_EXT_MONITOR | CPUID_EXT_SSSE3 | - CPUID_EXT_VMX | CPUID_EXT_CX16, + CPUID_EXT_CX16, .features[FEAT_8000_0001_EDX] = CPUID_EXT2_LM | CPUID_EXT2_SYSCALL | CPUID_EXT2_NX, .features[FEAT_8000_0001_ECX] = @@ -804,9 +804,9 @@ static X86CPUDefinition builtin_x86_defs[] = { CPUID_MTRR | CPUID_CLFLUSH | CPUID_MCA | CPUID_ACPI | CPUID_SS, /* Missing: CPUID_EXT_EST, CPUID_EXT_TM2 , CPUID_EXT_XTPR, - * CPUID_EXT_PDCM */ + * CPUID_EXT_PDCM, CPUID_EXT_VMX */ .features[FEAT_1_ECX] = - CPUID_EXT_SSE3 | CPUID_EXT_MONITOR | CPUID_EXT_VMX, + CPUID_EXT_SSE3 | CPUID_EXT_MONITOR, .features[FEAT_8000_0001_EDX] = CPUID_EXT2_NX, .xlevel = 0x80000008,
TCG doesn't support VMX, and nested VMX is not enabled by default on the KVM kernel module. So, there's no reason to have VMX enabled by default on the core2duo and coreduo CPU models, today. Even the newer Intel CPU model definitions don't have it enabled. In this case, we need machine-type compat code, as people may be running the older machine-types on hosts that had VMX nesting enabled. Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> --- hw/i386/pc_piix.c | 2 ++ hw/i386/pc_q35.c | 2 ++ target-i386/cpu.c | 8 ++++---- 3 files changed, 8 insertions(+), 4 deletions(-)