| Message ID | 1434641064-8405-3-git-send-email-rkrcmar@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Jun 18, 2015 at 05:24:24PM +0200, Radim Krčmář wrote: > We already bump to level 7 if features there are requested, so do the > same for 0xD. > > Signed-off-by: Radim Krčmář <rkrcmar@redhat.com> This breaks guest ABI and live-migration, as CPUID data is not part of the migration stream (although we have considered including it in the future). If we are going to add more special cases like this, we must provide a way to make QEMU honour an explicit "level" option from the config file or command-line. I have considered introducing "min-[x]level" and "max-{x]level" properties to control automatic increasing of level/xlevel. The existing X86CPUDefinition.level field could just control min_level, while explicit "level=" on the command-line or config file would explicitly force a specific value. Probably setting "max-level" on machine-type compat code would be enough to restore the previous behavior. > --- > If we want this behavior, we should not do it by writing a case for > every level. > > target-i386/cpu.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > index d392cf46f517..7a32ead690d2 100644 > --- a/target-i386/cpu.c > +++ b/target-i386/cpu.c > @@ -2796,6 +2796,10 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp) > env->cpuid_level = 7; > } > > + if (env->features[FEAT_XSAVE] && env->cpuid_level < 0xd) { > + env->cpuid_level = 0xd; > + } > + > /* On AMD CPUs, some CPUID[8000_0001].EDX bits must match the bits on > * CPUID[1].EDX. > */ > -- > 2.4.4 >
Eduardo Habkost <ehabkost@redhat.com> writes: > On Thu, Jun 18, 2015 at 05:24:24PM +0200, Radim Krčmář wrote: >> We already bump to level 7 if features there are requested, so do the >> same for 0xD. But doesn't bumping to 7 for feat[ebx] have the potential to break ABI too ? >> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com> > > This breaks guest ABI and live-migration, as CPUID data is not part of > the migration stream (although we have considered including it in the > future). > > If we are going to add more special cases like this, we must provide a > way to make QEMU honour an explicit "level" option from the config file > or command-line. > > I have considered introducing "min-[x]level" and "max-{x]level" > properties to control automatic increasing of level/xlevel. The existing > X86CPUDefinition.level field could just control min_level, while > explicit "level=" on the command-line or config file would explicitly > force a specific value. Probably setting "max-level" on machine-type > compat code would be enough to restore the previous behavior. > > >> --- >> If we want this behavior, we should not do it by writing a case for >> every level. Agreed, we should really have a more generic way of doing this. Bandan >> target-i386/cpu.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/target-i386/cpu.c b/target-i386/cpu.c >> index d392cf46f517..7a32ead690d2 100644 >> --- a/target-i386/cpu.c >> +++ b/target-i386/cpu.c >> @@ -2796,6 +2796,10 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp) >> env->cpuid_level = 7; >> } >> >> + if (env->features[FEAT_XSAVE] && env->cpuid_level < 0xd) { >> + env->cpuid_level = 0xd; >> + } >> + >> /* On AMD CPUs, some CPUID[8000_0001].EDX bits must match the bits on >> * CPUID[1].EDX. >> */ >> -- >> 2.4.4 >>
On Thu, Jun 18, 2015 at 01:12:32PM -0400, Bandan Das wrote: > Eduardo Habkost <ehabkost@redhat.com> writes: > > > On Thu, Jun 18, 2015 at 05:24:24PM +0200, Radim Krčmář wrote: > >> We already bump to level 7 if features there are requested, so do the > >> same for 0xD. > > But doesn't bumping to 7 for feat[ebx] have the potential to break > ABI too ? It's not the auto-bumping that breaks ABI, but having older machine-types now exposing different CPUID data to guests after applying the patch. The auto-bump to 7 was introduced at the same time we introduced the first CPUID[7] features, so you couldn't have any running machines that would break.
2015-06-18 12:50-0300, Eduardo Habkost: > On Thu, Jun 18, 2015 at 05:24:24PM +0200, Radim Krčmář wrote: > > We already bump to level 7 if features there are requested, so do the > > same for 0xD. > > > > Signed-off-by: Radim Krčmář <rkrcmar@redhat.com> > > This breaks guest ABI and live-migration, as CPUID data is not part of > the migration stream (although we have considered including it in the > future). > > If we are going to add more special cases like this, we must provide a > way to make QEMU honour an explicit "level" option from the config file > or command-line. Thanks, I'll drop this patch. > I have considered introducing "min-[x]level" and "max-{x]level" > properties to control automatic increasing of level/xlevel. The existing > X86CPUDefinition.level field could just control min_level, while > explicit "level=" on the command-line or config file would explicitly > force a specific value. Probably setting "max-level" on machine-type > compat code would be enough to restore the previous behavior. We'd need to set min-level at least to 7, to capture the raising we do now, but a feature in level between default and 7 would result in a different behavior, so we need to make it much uglier :/ We can add 'compat-level' bit for old machine types and raise to highest habited function otherwise, optionally with controls you described.
2015-06-19 11:47+0200, Radim Krčmář: > 2015-06-18 12:50-0300, Eduardo Habkost: > > I have considered introducing "min-[x]level" and "max-{x]level" > > properties to control automatic increasing of level/xlevel. The existing > > X86CPUDefinition.level field could just control min_level, while > > explicit "level=" on the command-line or config file would explicitly > > force a specific value. Probably setting "max-level" on machine-type > > compat code would be enough to restore the previous behavior. > > We'd need to set min-level at least to 7, to capture the raising we do ^^^^^^^^^ Should have been max-level. (The alcohol level doesn't drop fast enough.) > now, but a feature in level between default and 7 would result in a > different behavior, so we need to make it much uglier :/ > We can add 'compat-level' bit for old machine types and raise to highest > habited function otherwise, optionally with controls you described.
2015-06-19 11:47+0200, Radim Krčmář: > 2015-06-18 12:50-0300, Eduardo Habkost: > > I have considered introducing "min-[x]level" and "max-{x]level" > > properties to control automatic increasing of level/xlevel. The existing > > X86CPUDefinition.level field could just control min_level, while > > explicit "level=" on the command-line or config file would explicitly > > force a specific value. Probably setting "max-level" on machine-type > > compat code would be enough to restore the previous behavior. > > We'd need to set min-level at least to 7, to capture the raising we do > now, but a feature in level between default and 7 would result in a > different behavior, so we need to make it much uglier :/ > We can add 'compat-level' bit for old machine types and raise to highest > habited function otherwise, optionally with controls you described. No, features are only in 0x7 and 0xd, so the original solution is good. (We should also be bumping the CPUID level when adding specific features, e.g. to at least 0xB when x2apic is selected.)
diff --git a/target-i386/cpu.c b/target-i386/cpu.c index d392cf46f517..7a32ead690d2 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -2796,6 +2796,10 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp) env->cpuid_level = 7; } + if (env->features[FEAT_XSAVE] && env->cpuid_level < 0xd) { + env->cpuid_level = 0xd; + } + /* On AMD CPUs, some CPUID[8000_0001].EDX bits must match the bits on * CPUID[1].EDX. */
We already bump to level 7 if features there are requested, so do the same for 0xD. Signed-off-by: Radim Krčmář <rkrcmar@redhat.com> --- If we want this behavior, we should not do it by writing a case for every level. target-i386/cpu.c | 4 ++++ 1 file changed, 4 insertions(+)