| Message ID | 1362017554-1260-1-git-send-email-hpa@zytor.com |
|---|---|
| State | New |
| Headers | show |
Low priority ping on this patchset...? -hpa
Il 24/03/2013 06:06, H. Peter Anvin ha scritto: > Low priority ping on this patchset...? > > -hpa > > > I think it fell through the cracks due to the RFC tag. Patches 1 and 2 look good, but Anthony does not apply TCG patches. CCing Blue and Aurelien. Paolo
Am 24.03.2013 06:06, schrieb H. Peter Anvin:
> Low priority ping on this patchset...?
You forgot to CC me on the CPU models...
Andreas
Am 28.02.2013 03:12, schrieb H. Peter Anvin: > From: "H. Peter Anvin" <hpa@zytor.com> > > Add models for 486SX, and pre-CPUID versions of the 486 (DX & SX). > Change the model number for the standard 486DX to a model which > actually had CPUID. > > Note: these models are fairly vestigial, for example most of the FPU > operations still work; only F*ST[CS]W have been modified to appear as > through there is no FPU. > > This also changes the classic 486 model number to 8 (DX4) which > matches the feature set presented. > > Signed-off-by: H. Peter Anvin <hpa@zytor.com> > --- > target-i386/cpu.c | 39 ++++++++++++++++++++++++++--- > target-i386/fpu_helper.c | 12 +++++++-- > target-i386/misc_helper.c | 15 ++++++++---- > target-i386/translate.c | 62 +++++++++++++++-------------------------------- > 4 files changed, 75 insertions(+), 53 deletions(-) > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > index aab35c7..a5aad19 100644 > --- a/target-i386/cpu.c > +++ b/target-i386/cpu.c > @@ -365,8 +365,11 @@ typedef struct x86_def_t { > uint32_t cpuid_7_0_ebx_features; > } x86_def_t; > > -#define I486_FEATURES (CPUID_FP87 | CPUID_VME | CPUID_PSE) > -#define PENTIUM_FEATURES (I486_FEATURES | CPUID_DE | CPUID_TSC | \ > +#define OLD_I486SX_FEATURES 0 > +#define OLD_I486_FEATURES CPUID_FP87 > +#define I486SX_FEATURES CPUID_VME /* SX2+ */ > +#define I486_FEATURES (CPUID_FP87 | CPUID_VME) /* DX4 and some DX2 */ > +#define PENTIUM_FEATURES (I486_FEATURES | CPUID_PSE | CPUID_DE | CPUID_TSC | \ > CPUID_MSR | CPUID_MCE | CPUID_CX8 | CPUID_MMX | CPUID_APIC) > #define PENTIUM2_FEATURES (PENTIUM_FEATURES | CPUID_PAE | CPUID_SEP | \ > CPUID_MTRR | CPUID_PGE | CPUID_MCA | CPUID_CMOV | CPUID_PAT | \ > @@ -535,16 +538,46 @@ static x86_def_t builtin_x86_defs[] = { > .model_id = "Genuine Intel(R) CPU T2600 @ 2.16GHz", > }, > { > + .name = "old486", > + .level = 0, > + .vendor = CPUID_VENDOR_INTEL, > + .family = 4, > + .model = 1, > + .stepping = 0, > + .features = OLD_I486_FEATURES, > + .xlevel = 0, > + }, > + { > + .name = "old486sx", > + .level = 0, > + .vendor = CPUID_VENDOR_INTEL, > + .family = 4, > + .model = 2, > + .stepping = 0, > + .features = OLD_I486SX_FEATURES, > + .xlevel = 0, > + }, > + { > .name = "486", > .level = 1, > .vendor = CPUID_VENDOR_INTEL, > .family = 4, > - .model = 0, > + .model = 8, Such changes have been rejected in the past (e.g., n270 Atom). I personally wouldn't object to 486 changes, but I guess it should rather be handled via Igor's CPU static properties that I have in my review queue: The .model value would be set to 8 but the PC machine would be changed alongside to set model = 0 for pc-1.4 and earlier. > .stepping = 0, > .features = I486_FEATURES, > .xlevel = 0, > }, > { > + .name = "486sx", > + .level = 1, > + .vendor = CPUID_VENDOR_INTEL, > + .family = 4, > + .model = 5, > + .stepping = 0, > + .features = I486SX_FEATURES, > + .xlevel = 0, > + }, > + { > .name = "pentium", > .level = 1, > .vendor = CPUID_VENDOR_INTEL, [...] > diff --git a/target-i386/translate.c b/target-i386/translate.c > index 112c310..6d8abff 100644 > --- a/target-i386/translate.c > +++ b/target-i386/translate.c [...] > @@ -7926,6 +7900,8 @@ static inline void gen_intermediate_code_internal(CPUX86State *env, > if (flags & HF_SOFTMMU_MASK) { > dc->mem_index = (cpu_mmu_index(env) + 1) << 2; > } > + dc->cpuid_family = (env->cpuid_version >> 8) & 0x0f; > + dc->cpuid_level = env->cpuid_level; > dc->cpuid_features = env->cpuid_features; > dc->cpuid_ext_features = env->cpuid_ext_features; > dc->cpuid_ext2_features = env->cpuid_ext2_features; Would be better to reuse the "family" QOM property that also reads the upper two nibbles, to avoid surprises. Regards, Andreas
On Mon, 25 Mar 2013 10:39:36 +0100 Andreas Färber <afaerber@suse.de> wrote: > Am 28.02.2013 03:12, schrieb H. Peter Anvin: > > From: "H. Peter Anvin" <hpa@zytor.com> > > > > Add models for 486SX, and pre-CPUID versions of the 486 (DX & SX). > > Change the model number for the standard 486DX to a model which > > actually had CPUID. > > > > Note: these models are fairly vestigial, for example most of the FPU > > operations still work; only F*ST[CS]W have been modified to appear as > > through there is no FPU. > > > > This also changes the classic 486 model number to 8 (DX4) which > > matches the feature set presented. > > > > Signed-off-by: H. Peter Anvin <hpa@zytor.com> > > --- > > target-i386/cpu.c | 39 ++++++++++++++++++++++++++--- > > target-i386/fpu_helper.c | 12 +++++++-- > > target-i386/misc_helper.c | 15 ++++++++---- > > target-i386/translate.c | 62 > > +++++++++++++++-------------------------------- 4 files changed, 75 > > insertions(+), 53 deletions(-) > > > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > > index aab35c7..a5aad19 100644 > > --- a/target-i386/cpu.c > > +++ b/target-i386/cpu.c > > @@ -365,8 +365,11 @@ typedef struct x86_def_t { > > uint32_t cpuid_7_0_ebx_features; > > } x86_def_t; > > > > -#define I486_FEATURES (CPUID_FP87 | CPUID_VME | CPUID_PSE) > > -#define PENTIUM_FEATURES (I486_FEATURES | CPUID_DE | CPUID_TSC | \ > > +#define OLD_I486SX_FEATURES 0 > > +#define OLD_I486_FEATURES CPUID_FP87 > > +#define I486SX_FEATURES CPUID_VME /* SX2+ */ > > +#define I486_FEATURES (CPUID_FP87 | CPUID_VME) /* DX4 and some DX2 */ > > +#define PENTIUM_FEATURES (I486_FEATURES | CPUID_PSE | CPUID_DE | > > CPUID_TSC | \ CPUID_MSR | CPUID_MCE | CPUID_CX8 | CPUID_MMX | CPUID_APIC) > > #define PENTIUM2_FEATURES (PENTIUM_FEATURES | CPUID_PAE | CPUID_SEP | \ > > CPUID_MTRR | CPUID_PGE | CPUID_MCA | CPUID_CMOV | CPUID_PAT | \ > > @@ -535,16 +538,46 @@ static x86_def_t builtin_x86_defs[] = { > > .model_id = "Genuine Intel(R) CPU T2600 @ 2.16GHz", > > }, > > { > > + .name = "old486", > > + .level = 0, > > + .vendor = CPUID_VENDOR_INTEL, > > + .family = 4, > > + .model = 1, > > + .stepping = 0, > > + .features = OLD_I486_FEATURES, > > + .xlevel = 0, > > + }, > > + { > > + .name = "old486sx", > > + .level = 0, > > + .vendor = CPUID_VENDOR_INTEL, > > + .family = 4, > > + .model = 2, > > + .stepping = 0, > > + .features = OLD_I486SX_FEATURES, > > + .xlevel = 0, > > + }, > > + { > > .name = "486", > > .level = 1, > > .vendor = CPUID_VENDOR_INTEL, > > .family = 4, > > - .model = 0, > > + .model = 8, > > Such changes have been rejected in the past (e.g., n270 Atom). > I personally wouldn't object to 486 changes, but I guess it should > rather be handled via Igor's CPU static properties that I have in my > review queue: The .model value would be set to 8 but the PC machine > would be changed alongside to set model = 0 for pc-1.4 and earlier. It doesn't relates to property refactoring nor to slim CPU sub-classes conversion either. So it could go in independently. But is this change safe from migration POV? > > > .stepping = 0, > > .features = I486_FEATURES, > > .xlevel = 0, > > },
On 03/25/2013 08:15 AM, Igor Mammedov wrote: >> >> Such changes have been rejected in the past (e.g., n270 Atom). >> I personally wouldn't object to 486 changes, but I guess it should >> rather be handled via Igor's CPU static properties that I have in my >> review queue: The .model value would be set to 8 but the PC machine >> would be changed alongside to set model = 0 for pc-1.4 and earlier. > It doesn't relates to property refactoring nor to slim CPU sub-classes > conversion either. So it could go in independently. > > But is this change safe from migration POV? > Well, given that the CPU model presented is actually closer to a model 8 than a model 0 it probably is... but the real question is what would cause someone to do migration of a 486 CPU model. The n270 issue is problematic, because right now "n270" can't actually run software compiled for N270... -hpa
On Mon, Mar 25, 2013 at 04:15:15PM +0100, Igor Mammedov wrote: > On Mon, 25 Mar 2013 10:39:36 +0100 > Andreas Färber <afaerber@suse.de> wrote: > > > Am 28.02.2013 03:12, schrieb H. Peter Anvin: > > > From: "H. Peter Anvin" <hpa@zytor.com> > > > > > > Add models for 486SX, and pre-CPUID versions of the 486 (DX & SX). > > > Change the model number for the standard 486DX to a model which > > > actually had CPUID. > > > > > > Note: these models are fairly vestigial, for example most of the FPU > > > operations still work; only F*ST[CS]W have been modified to appear as > > > through there is no FPU. > > > > > > This also changes the classic 486 model number to 8 (DX4) which > > > matches the feature set presented. > > > > > > Signed-off-by: H. Peter Anvin <hpa@zytor.com> [...] > > > + }, > > > + { > > > .name = "486", > > > .level = 1, > > > .vendor = CPUID_VENDOR_INTEL, > > > .family = 4, > > > - .model = 0, > > > + .model = 8, > > > > Such changes have been rejected in the past (e.g., n270 Atom). > > I personally wouldn't object to 486 changes, but I guess it should > > rather be handled via Igor's CPU static properties that I have in my > > review queue: The .model value would be set to 8 but the PC machine > > would be changed alongside to set model = 0 for pc-1.4 and earlier. > It doesn't relates to property refactoring nor to slim CPU sub-classes > conversion either. So it could go in independently. > > But is this change safe from migration POV? Migration is exactly the reason we can't include this as-is, and where having static properties would be useful. We need to keep model=0 on the pc-1.3 and older machine-types, and set model=8 only on pc-1.4 and newer. With static properties we can simply set it using the compat_props table; without static properties, we need a compatibility function or global variable to enable the model=0 behavior. > > > > > > .stepping = 0, > > > .features = I486_FEATURES, > > > .xlevel = 0, > > > },
On Mon, Mar 25, 2013 at 11:44:30AM -0700, H. Peter Anvin wrote: > On 03/25/2013 08:15 AM, Igor Mammedov wrote: > >> > >> Such changes have been rejected in the past (e.g., n270 Atom). > >> I personally wouldn't object to 486 changes, but I guess it should > >> rather be handled via Igor's CPU static properties that I have in my > >> review queue: The .model value would be set to 8 but the PC machine > >> would be changed alongside to set model = 0 for pc-1.4 and earlier. > > It doesn't relates to property refactoring nor to slim CPU sub-classes > > conversion either. So it could go in independently. > > > > But is this change safe from migration POV? > > > > Well, given that the CPU model presented is actually closer to a model 8 > than a model 0 it probably is... but the real question is what would > cause someone to do migration of a 486 CPU model. > > The n270 issue is problematic, because right now "n270" can't actually > run software compiled for N270... FWIW, I wouldn't mind too much if the maintainers decide to document 486 and n270 as "migration-unsafe" and then knowingly break live-migration of those CPU models between qemu <= 1.3 and qemu >= 1.4. It's up to the maintainers to choose which way to go.
On 03/25/2013 12:05 PM, Eduardo Habkost wrote: > On Mon, Mar 25, 2013 at 11:44:30AM -0700, H. Peter Anvin wrote: >> On 03/25/2013 08:15 AM, Igor Mammedov wrote: >>>> >>>> Such changes have been rejected in the past (e.g., n270 Atom). >>>> I personally wouldn't object to 486 changes, but I guess it should >>>> rather be handled via Igor's CPU static properties that I have in my >>>> review queue: The .model value would be set to 8 but the PC machine >>>> would be changed alongside to set model = 0 for pc-1.4 and earlier. >>> It doesn't relates to property refactoring nor to slim CPU sub-classes >>> conversion either. So it could go in independently. >>> >>> But is this change safe from migration POV? >>> >> >> Well, given that the CPU model presented is actually closer to a model 8 >> than a model 0 it probably is... but the real question is what would >> cause someone to do migration of a 486 CPU model. >> >> The n270 issue is problematic, because right now "n270" can't actually >> run software compiled for N270... > > FWIW, I wouldn't mind too much if the maintainers decide to document 486 > and n270 as "migration-unsafe" and then knowingly break live-migration > of those CPU models between qemu <= 1.3 and qemu >= 1.4. It's up to the > maintainers to choose which way to go. > The right thing, of course (and I believe that's where things are going) is to unwind these descriptions at the time the VM is created; the migration should implement the machine as it was launched. If that isn't practical, then the right thing to do is probably to have some kind of machine description conversion (so, say, "486" can be converted to "486-1.3" containing the legacy description), but telling people that -cpu n270 is something other than a real N270 that can't run N270 software is user-hostile in the extreme. It needs to be possible to fix bugs.... -hpa
Now, all this said... if the only objection is the change of model number for "486" then I suggest just dropping that. -hpa
On Mon, Mar 25, 2013 at 01:45:47PM -0700, H. Peter Anvin wrote: > On 03/25/2013 12:05 PM, Eduardo Habkost wrote: > > On Mon, Mar 25, 2013 at 11:44:30AM -0700, H. Peter Anvin wrote: > >> On 03/25/2013 08:15 AM, Igor Mammedov wrote: > >>>> > >>>> Such changes have been rejected in the past (e.g., n270 Atom). > >>>> I personally wouldn't object to 486 changes, but I guess it should > >>>> rather be handled via Igor's CPU static properties that I have in my > >>>> review queue: The .model value would be set to 8 but the PC machine > >>>> would be changed alongside to set model = 0 for pc-1.4 and earlier. > >>> It doesn't relates to property refactoring nor to slim CPU sub-classes > >>> conversion either. So it could go in independently. > >>> > >>> But is this change safe from migration POV? > >>> > >> > >> Well, given that the CPU model presented is actually closer to a model 8 > >> than a model 0 it probably is... but the real question is what would > >> cause someone to do migration of a 486 CPU model. > >> > >> The n270 issue is problematic, because right now "n270" can't actually > >> run software compiled for N270... > > > > FWIW, I wouldn't mind too much if the maintainers decide to document 486 > > and n270 as "migration-unsafe" and then knowingly break live-migration > > of those CPU models between qemu <= 1.3 and qemu >= 1.4. It's up to the > > maintainers to choose which way to go. > > > > The right thing, of course (and I believe that's where things are going) > is to unwind these descriptions at the time the VM is created; the > migration should implement the machine as it was launched. This is a possibility, but we are trying to make the CPU code sane and converted to use common QOM/DeviceState infra-structure before doing that. > > If that isn't practical, then the right thing to do is probably to have > some kind of machine description conversion (so, say, "486" can be > converted to "486-1.3" containing the legacy description), but telling > people that -cpu n270 is something other than a real N270 that can't run > N270 software is user-hostile in the extreme. We have considered having versioned CPU model names, and per-machine-tyep aliases (I think I have sent patches to do that a long time ago), but that approach was discarded in favor of the DeviceState static-properties/compat_props mechanism (described below). > > It needs to be possible to fix bugs.... It is possible to fix them today: just write a compat function or add a global variable that is handled by cpu_x86_init(), and call it from the pc-1.3 machine-type init function. See disable_kvm_pv_eoi() and enable_compat_apic_id_mode(), for example. The difference is that this will be much easier once we introduce the static properties: then you just need to add the compat property values to the compat_props field on the machine-type struct.
On 03/25/2013 01:56 PM, Eduardo Habkost wrote: > >> >> It needs to be possible to fix bugs.... > > It is possible to fix them today: just write a compat function or add a > global variable that is handled by cpu_x86_init(), and call it from the > pc-1.3 machine-type init function. See disable_kvm_pv_eoi() and > enable_compat_apic_id_mode(), for example. > > The difference is that this will be much easier once we introduce the > static properties: then you just need to add the compat property values > to the compat_props field on the machine-type struct. > It sounds like this is underway, and since the priority for the 486 bit is very low it is better for that bit to just wait. -hpa
On Wed, Feb 27, 2013 at 06:12:32PM -0800, H. Peter Anvin wrote: > From: "H. Peter Anvin" <hpa@zytor.com> > > Add models for 486SX, and pre-CPUID versions of the 486 (DX & SX). > Change the model number for the standard 486DX to a model which > actually had CPUID. > > Note: these models are fairly vestigial, for example most of the FPU > operations still work; only F*ST[CS]W have been modified to appear as > through there is no FPU. > > This also changes the classic 486 model number to 8 (DX4) which > matches the feature set presented. > > Signed-off-by: H. Peter Anvin <hpa@zytor.com> > --- > target-i386/cpu.c | 39 ++++++++++++++++++++++++++--- > target-i386/fpu_helper.c | 12 +++++++-- > target-i386/misc_helper.c | 15 ++++++++---- > target-i386/translate.c | 62 +++++++++++++++-------------------------------- > 4 files changed, 75 insertions(+), 53 deletions(-) > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > index aab35c7..a5aad19 100644 > --- a/target-i386/cpu.c > +++ b/target-i386/cpu.c > @@ -365,8 +365,11 @@ typedef struct x86_def_t { > uint32_t cpuid_7_0_ebx_features; > } x86_def_t; > > -#define I486_FEATURES (CPUID_FP87 | CPUID_VME | CPUID_PSE) > -#define PENTIUM_FEATURES (I486_FEATURES | CPUID_DE | CPUID_TSC | \ > +#define OLD_I486SX_FEATURES 0 > +#define OLD_I486_FEATURES CPUID_FP87 > +#define I486SX_FEATURES CPUID_VME /* SX2+ */ > +#define I486_FEATURES (CPUID_FP87 | CPUID_VME) /* DX4 and some DX2 */ > +#define PENTIUM_FEATURES (I486_FEATURES | CPUID_PSE | CPUID_DE | CPUID_TSC | \ > CPUID_MSR | CPUID_MCE | CPUID_CX8 | CPUID_MMX | CPUID_APIC) > #define PENTIUM2_FEATURES (PENTIUM_FEATURES | CPUID_PAE | CPUID_SEP | \ > CPUID_MTRR | CPUID_PGE | CPUID_MCA | CPUID_CMOV | CPUID_PAT | \ > @@ -535,16 +538,46 @@ static x86_def_t builtin_x86_defs[] = { > .model_id = "Genuine Intel(R) CPU T2600 @ 2.16GHz", > }, > { > + .name = "old486", > + .level = 0, > + .vendor = CPUID_VENDOR_INTEL, > + .family = 4, > + .model = 1, > + .stepping = 0, > + .features = OLD_I486_FEATURES, > + .xlevel = 0, > + }, > + { > + .name = "old486sx", > + .level = 0, > + .vendor = CPUID_VENDOR_INTEL, > + .family = 4, > + .model = 2, > + .stepping = 0, > + .features = OLD_I486SX_FEATURES, > + .xlevel = 0, > + }, > + { > .name = "486", > .level = 1, > .vendor = CPUID_VENDOR_INTEL, > .family = 4, > - .model = 0, > + .model = 8, > .stepping = 0, > .features = I486_FEATURES, > .xlevel = 0, > }, > { > + .name = "486sx", > + .level = 1, > + .vendor = CPUID_VENDOR_INTEL, > + .family = 4, > + .model = 5, > + .stepping = 0, > + .features = I486SX_FEATURES, > + .xlevel = 0, > + }, > + { > .name = "pentium", > .level = 1, > .vendor = CPUID_VENDOR_INTEL, > diff --git a/target-i386/fpu_helper.c b/target-i386/fpu_helper.c > index 44f3d27..c4c2724 100644 > --- a/target-i386/fpu_helper.c > +++ b/target-i386/fpu_helper.c > @@ -530,12 +530,20 @@ void helper_fldz_FT0(CPUX86State *env) > > uint32_t helper_fnstsw(CPUX86State *env) > { > - return (env->fpus & ~0x3800) | (env->fpstt & 0x7) << 11; > + if (!(env->cpuid_features & CPUID_FP87)) { > + return 0xffff; > + } else { > + return (env->fpus & ~0x3800) | (env->fpstt & 0x7) << 11; > + } > } > > uint32_t helper_fnstcw(CPUX86State *env) > { > - return env->fpuc; > + if (!(env->cpuid_features & CPUID_FP87)) { > + return 0xffff; > + } else { > + return env->fpuc; > + } > } This really looks like Linux kernel specific. I haven't been able to test on a real machine, but the documentation I have found suggest that without and x87 FPU, the FPU instructions are simply ignored. The common way to detect an FPU is therefore to initialize registers to a given value, run fnstsw and fnstcw instructions with the register in arguments and see if they have been modified. The Linux kernel indeed set the initial value of these registers to 0xffff, but I am not sure all codes are doing the same. For me it looks like better to skip such instructions directly in translate.c. As a bonus it seems easy to do that for all FPU instructions. > static void update_fp_status(CPUX86State *env) > diff --git a/target-i386/misc_helper.c b/target-i386/misc_helper.c > index b6d5740..1ff25d1 100644 > --- a/target-i386/misc_helper.c > +++ b/target-i386/misc_helper.c > @@ -122,11 +122,16 @@ void helper_cpuid(CPUX86State *env) > > cpu_svm_check_intercept_param(env, SVM_EXIT_CPUID, 0); > > - cpu_x86_cpuid(env, (uint32_t)EAX, (uint32_t)ECX, &eax, &ebx, &ecx, &edx); > - EAX = eax; > - EBX = ebx; > - ECX = ecx; > - EDX = edx; > + if (!env->cpuid_level) { > + raise_exception_err(env, EXCP06_ILLOP, 0); > + } else { > + cpu_x86_cpuid(env, (uint32_t)EAX, (uint32_t)ECX, > + &eax, &ebx, &ecx, &edx); > + EAX = eax; > + EBX = ebx; > + ECX = ecx; > + EDX = edx; > + } > } > > #if defined(CONFIG_USER_ONLY) > diff --git a/target-i386/translate.c b/target-i386/translate.c > index 112c310..6d8abff 100644 > --- a/target-i386/translate.c > +++ b/target-i386/translate.c > @@ -103,6 +103,8 @@ typedef struct DisasContext { > struct TranslationBlock *tb; > int popl_esp_hack; /* for correct popl with esp base handling */ > int rip_offset; /* only used in x86_64, but left for simplicity */ > + int cpuid_family; > + int cpuid_level; > int cpuid_features; > int cpuid_ext_features; > int cpuid_ext2_features; > @@ -6513,52 +6515,24 @@ static target_ulong disas_insn(CPUX86State *env, DisasContext *s, > if (s->vm86 && s->iopl != 3) { > gen_exception(s, EXCP0D_GPF, pc_start - s->cs_base); > } else { > + uint32_t mask; > gen_pop_T0(s); > + mask = TF_MASK | NT_MASK | IF_MASK; > + if (s->cpuid_family >= 4) { > + mask |= AC_MASK; > + } > + if (s->cpuid_level) { > + mask |= ID_MASK; > + } > if (s->cpl == 0) { > - if (s->dflag) { > - gen_helper_write_eflags(cpu_env, cpu_T[0], > - tcg_const_i32((TF_MASK | AC_MASK | > - ID_MASK | NT_MASK | > - IF_MASK | > - IOPL_MASK))); > - } else { > - gen_helper_write_eflags(cpu_env, cpu_T[0], > - tcg_const_i32((TF_MASK | AC_MASK | > - ID_MASK | NT_MASK | > - IF_MASK | IOPL_MASK) > - & 0xffff)); > - } > - } else { > - if (s->cpl <= s->iopl) { > - if (s->dflag) { > - gen_helper_write_eflags(cpu_env, cpu_T[0], > - tcg_const_i32((TF_MASK | > - AC_MASK | > - ID_MASK | > - NT_MASK | > - IF_MASK))); > - } else { > - gen_helper_write_eflags(cpu_env, cpu_T[0], > - tcg_const_i32((TF_MASK | > - AC_MASK | > - ID_MASK | > - NT_MASK | > - IF_MASK) > - & 0xffff)); > - } > - } else { > - if (s->dflag) { > - gen_helper_write_eflags(cpu_env, cpu_T[0], > - tcg_const_i32((TF_MASK | AC_MASK | > - ID_MASK | NT_MASK))); > - } else { > - gen_helper_write_eflags(cpu_env, cpu_T[0], > - tcg_const_i32((TF_MASK | AC_MASK | > - ID_MASK | NT_MASK) > - & 0xffff)); > - } > - } > + mask |= IF_MASK | IOPL_MASK; > + } else if (s->cpl <= s->iopl) { > + mask |= IF_MASK; > + } > + if (!s->dflag) { > + mask &= 0xffff; > } > + gen_helper_write_eflags(cpu_env, cpu_T[0], tcg_const_i32(mask)); > gen_pop_update(s); > s->cc_op = CC_OP_EFLAGS; > /* abort translation because TF/AC flag may change */ > @@ -7926,6 +7900,8 @@ static inline void gen_intermediate_code_internal(CPUX86State *env, > if (flags & HF_SOFTMMU_MASK) { > dc->mem_index = (cpu_mmu_index(env) + 1) << 2; > } > + dc->cpuid_family = (env->cpuid_version >> 8) & 0x0f; > + dc->cpuid_level = env->cpuid_level; > dc->cpuid_features = env->cpuid_features; > dc->cpuid_ext_features = env->cpuid_ext_features; > dc->cpuid_ext2_features = env->cpuid_ext2_features; The remaining of the patch looks fine to me.
On 03/28/2013 12:15 PM, Aurelien Jarno wrote: > > This really looks like Linux kernel specific. I haven't been able to > test on a real machine, but the documentation I have found suggest that > without and x87 FPU, the FPU instructions are simply ignored. The common > way to detect an FPU is therefore to initialize registers to a given > value, run fnstsw and fnstcw instructions with the register in arguments > and see if they have been modified. > > The Linux kernel indeed set the initial value of these registers to > 0xffff, but I am not sure all codes are doing the same. > > For me it looks like better to skip such instructions directly in > translate.c. As a bonus it seems easy to do that for all FPU > instructions. > At least *some* real-life CPUs returned 0xffff, at the very least the machines with external buses did (e.g. 386 without 387). I don't have access to a live 486SX so I can test if 486SX behaved differenly. -hpa
On 03/28/2013 12:15 PM, Aurelien Jarno wrote: > > This really looks like Linux kernel specific. I haven't been able to > test on a real machine, but the documentation I have found suggest that > without and x87 FPU, the FPU instructions are simply ignored. The common > way to detect an FPU is therefore to initialize registers to a given > value, run fnstsw and fnstcw instructions with the register in arguments > and see if they have been modified. > > The Linux kernel indeed set the initial value of these registers to > 0xffff, but I am not sure all codes are doing the same. > > For me it looks like better to skip such instructions directly in > translate.c. As a bonus it seems easy to do that for all FPU > instructions. > It might have been (and this is from memory, so don't take it for anything) that the register form receives 0xffff, but the memory form is ignored. -hpa
On 03/28/2013 03:12:11 PM, H. Peter Anvin wrote: > On 03/28/2013 12:15 PM, Aurelien Jarno wrote: > > > > This really looks like Linux kernel specific. I haven't been able to > > test on a real machine, but the documentation I have found suggest > that > > without and x87 FPU, the FPU instructions are simply ignored. The > common > > way to detect an FPU is therefore to initialize registers to a given > > value, run fnstsw and fnstcw instructions with the register in > arguments > > and see if they have been modified. > > > > The Linux kernel indeed set the initial value of these registers to > > 0xffff, but I am not sure all codes are doing the same. > > > > For me it looks like better to skip such instructions directly in > > translate.c. As a bonus it seems easy to do that for all FPU > > instructions. > > > > It might have been (and this is from memory, so don't take it for > anything) that the register form receives 0xffff, but the memory form > is > ignored. Speaking of which, Solar Designer recently found a bug where pentium 3 silently ignores the 66 prefix that later became SSE2, and thus the code ran but produced the wrong result: https://twitter.com/solardiz/status/316204216962142209 https://twitter.com/solardiz/status/316207184134410240 But this isn't what QEMU does: https://twitter.com/solardiz/status/316944417871245313 Rob
Qemu is absolutely horrid at modeling corner cases. Rob Landley <rob@landley.net> wrote: >On 03/28/2013 03:12:11 PM, H. Peter Anvin wrote: >> On 03/28/2013 12:15 PM, Aurelien Jarno wrote: >> > >> > This really looks like Linux kernel specific. I haven't been able >to >> > test on a real machine, but the documentation I have found suggest > >> that >> > without and x87 FPU, the FPU instructions are simply ignored. The >> common >> > way to detect an FPU is therefore to initialize registers to a >given >> > value, run fnstsw and fnstcw instructions with the register in >> arguments >> > and see if they have been modified. >> > >> > The Linux kernel indeed set the initial value of these registers to >> > 0xffff, but I am not sure all codes are doing the same. >> > >> > For me it looks like better to skip such instructions directly in >> > translate.c. As a bonus it seems easy to do that for all FPU >> > instructions. >> > >> >> It might have been (and this is from memory, so don't take it for >> anything) that the register form receives 0xffff, but the memory form > >> is >> ignored. > >Speaking of which, Solar Designer recently found a bug where pentium 3 > >silently ignores the 66 prefix that later became SSE2, and thus the >code ran but produced the wrong result: > >https://twitter.com/solardiz/status/316204216962142209 >https://twitter.com/solardiz/status/316207184134410240 > >But this isn't what QEMU does: > >https://twitter.com/solardiz/status/316944417871245313 > >Rob
On 03/25/2013 01:56 PM, Eduardo Habkost wrote: > >> >> It needs to be possible to fix bugs.... > > It is possible to fix them today: just write a compat function or add a > global variable that is handled by cpu_x86_init(), and call it from the > pc-1.3 machine-type init function. See disable_kvm_pv_eoi() and > enable_compat_apic_id_mode(), for example. > > The difference is that this will be much easier once we introduce the > static properties: then you just need to add the compat property values > to the compat_props field on the machine-type struct. > Hi guys, Any movement on this in the past year? -hpa
On Mon, 06 Jan 2014 21:53:50 -0800 "H. Peter Anvin" <hpa@zytor.com> wrote: > On 03/25/2013 01:56 PM, Eduardo Habkost wrote: > > > >> > >> It needs to be possible to fix bugs.... > > > > It is possible to fix them today: just write a compat function or add a > > global variable that is handled by cpu_x86_init(), and call it from the > > pc-1.3 machine-type init function. See disable_kvm_pv_eoi() and > > enable_compat_apic_id_mode(), for example. > > > > The difference is that this will be much easier once we introduce the > > static properties: then you just need to add the compat property values > > to the compat_props field on the machine-type struct. > > > > Hi guys, > > Any movement on this in the past year? currently, it's possible to use compat properties for fixing 'model' field, see f8e6a11aecc9 "target-i386: Set model=6 on qemu64 & qemu32 CPU models" as an example. As for using compat properties for feature bits, we are getting closer to it, there is a discussion on how to proceed with feature bits conversion that will allow it. http://lists.nongnu.org/archive/html/qemu-devel/2013-12/msg02707.html > > -hpa > >
diff --git a/target-i386/cpu.c b/target-i386/cpu.c index aab35c7..a5aad19 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -365,8 +365,11 @@ typedef struct x86_def_t { uint32_t cpuid_7_0_ebx_features; } x86_def_t; -#define I486_FEATURES (CPUID_FP87 | CPUID_VME | CPUID_PSE) -#define PENTIUM_FEATURES (I486_FEATURES | CPUID_DE | CPUID_TSC | \ +#define OLD_I486SX_FEATURES 0 +#define OLD_I486_FEATURES CPUID_FP87 +#define I486SX_FEATURES CPUID_VME /* SX2+ */ +#define I486_FEATURES (CPUID_FP87 | CPUID_VME) /* DX4 and some DX2 */ +#define PENTIUM_FEATURES (I486_FEATURES | CPUID_PSE | CPUID_DE | CPUID_TSC | \ CPUID_MSR | CPUID_MCE | CPUID_CX8 | CPUID_MMX | CPUID_APIC) #define PENTIUM2_FEATURES (PENTIUM_FEATURES | CPUID_PAE | CPUID_SEP | \ CPUID_MTRR | CPUID_PGE | CPUID_MCA | CPUID_CMOV | CPUID_PAT | \ @@ -535,16 +538,46 @@ static x86_def_t builtin_x86_defs[] = { .model_id = "Genuine Intel(R) CPU T2600 @ 2.16GHz", }, { + .name = "old486", + .level = 0, + .vendor = CPUID_VENDOR_INTEL, + .family = 4, + .model = 1, + .stepping = 0, + .features = OLD_I486_FEATURES, + .xlevel = 0, + }, + { + .name = "old486sx", + .level = 0, + .vendor = CPUID_VENDOR_INTEL, + .family = 4, + .model = 2, + .stepping = 0, + .features = OLD_I486SX_FEATURES, + .xlevel = 0, + }, + { .name = "486", .level = 1, .vendor = CPUID_VENDOR_INTEL, .family = 4, - .model = 0, + .model = 8, .stepping = 0, .features = I486_FEATURES, .xlevel = 0, }, { + .name = "486sx", + .level = 1, + .vendor = CPUID_VENDOR_INTEL, + .family = 4, + .model = 5, + .stepping = 0, + .features = I486SX_FEATURES, + .xlevel = 0, + }, + { .name = "pentium", .level = 1, .vendor = CPUID_VENDOR_INTEL, diff --git a/target-i386/fpu_helper.c b/target-i386/fpu_helper.c index 44f3d27..c4c2724 100644 --- a/target-i386/fpu_helper.c +++ b/target-i386/fpu_helper.c @@ -530,12 +530,20 @@ void helper_fldz_FT0(CPUX86State *env) uint32_t helper_fnstsw(CPUX86State *env) { - return (env->fpus & ~0x3800) | (env->fpstt & 0x7) << 11; + if (!(env->cpuid_features & CPUID_FP87)) { + return 0xffff; + } else { + return (env->fpus & ~0x3800) | (env->fpstt & 0x7) << 11; + } } uint32_t helper_fnstcw(CPUX86State *env) { - return env->fpuc; + if (!(env->cpuid_features & CPUID_FP87)) { + return 0xffff; + } else { + return env->fpuc; + } } static void update_fp_status(CPUX86State *env) diff --git a/target-i386/misc_helper.c b/target-i386/misc_helper.c index b6d5740..1ff25d1 100644 --- a/target-i386/misc_helper.c +++ b/target-i386/misc_helper.c @@ -122,11 +122,16 @@ void helper_cpuid(CPUX86State *env) cpu_svm_check_intercept_param(env, SVM_EXIT_CPUID, 0); - cpu_x86_cpuid(env, (uint32_t)EAX, (uint32_t)ECX, &eax, &ebx, &ecx, &edx); - EAX = eax; - EBX = ebx; - ECX = ecx; - EDX = edx; + if (!env->cpuid_level) { + raise_exception_err(env, EXCP06_ILLOP, 0); + } else { + cpu_x86_cpuid(env, (uint32_t)EAX, (uint32_t)ECX, + &eax, &ebx, &ecx, &edx); + EAX = eax; + EBX = ebx; + ECX = ecx; + EDX = edx; + } } #if defined(CONFIG_USER_ONLY) diff --git a/target-i386/translate.c b/target-i386/translate.c index 112c310..6d8abff 100644 --- a/target-i386/translate.c +++ b/target-i386/translate.c @@ -103,6 +103,8 @@ typedef struct DisasContext { struct TranslationBlock *tb; int popl_esp_hack; /* for correct popl with esp base handling */ int rip_offset; /* only used in x86_64, but left for simplicity */ + int cpuid_family; + int cpuid_level; int cpuid_features; int cpuid_ext_features; int cpuid_ext2_features; @@ -6513,52 +6515,24 @@ static target_ulong disas_insn(CPUX86State *env, DisasContext *s, if (s->vm86 && s->iopl != 3) { gen_exception(s, EXCP0D_GPF, pc_start - s->cs_base); } else { + uint32_t mask; gen_pop_T0(s); + mask = TF_MASK | NT_MASK | IF_MASK; + if (s->cpuid_family >= 4) { + mask |= AC_MASK; + } + if (s->cpuid_level) { + mask |= ID_MASK; + } if (s->cpl == 0) { - if (s->dflag) { - gen_helper_write_eflags(cpu_env, cpu_T[0], - tcg_const_i32((TF_MASK | AC_MASK | - ID_MASK | NT_MASK | - IF_MASK | - IOPL_MASK))); - } else { - gen_helper_write_eflags(cpu_env, cpu_T[0], - tcg_const_i32((TF_MASK | AC_MASK | - ID_MASK | NT_MASK | - IF_MASK | IOPL_MASK) - & 0xffff)); - } - } else { - if (s->cpl <= s->iopl) { - if (s->dflag) { - gen_helper_write_eflags(cpu_env, cpu_T[0], - tcg_const_i32((TF_MASK | - AC_MASK | - ID_MASK | - NT_MASK | - IF_MASK))); - } else { - gen_helper_write_eflags(cpu_env, cpu_T[0], - tcg_const_i32((TF_MASK | - AC_MASK | - ID_MASK | - NT_MASK | - IF_MASK) - & 0xffff)); - } - } else { - if (s->dflag) { - gen_helper_write_eflags(cpu_env, cpu_T[0], - tcg_const_i32((TF_MASK | AC_MASK | - ID_MASK | NT_MASK))); - } else { - gen_helper_write_eflags(cpu_env, cpu_T[0], - tcg_const_i32((TF_MASK | AC_MASK | - ID_MASK | NT_MASK) - & 0xffff)); - } - } + mask |= IF_MASK | IOPL_MASK; + } else if (s->cpl <= s->iopl) { + mask |= IF_MASK; + } + if (!s->dflag) { + mask &= 0xffff; } + gen_helper_write_eflags(cpu_env, cpu_T[0], tcg_const_i32(mask)); gen_pop_update(s); s->cc_op = CC_OP_EFLAGS; /* abort translation because TF/AC flag may change */ @@ -7926,6 +7900,8 @@ static inline void gen_intermediate_code_internal(CPUX86State *env, if (flags & HF_SOFTMMU_MASK) { dc->mem_index = (cpu_mmu_index(env) + 1) << 2; } + dc->cpuid_family = (env->cpuid_version >> 8) & 0x0f; + dc->cpuid_level = env->cpuid_level; dc->cpuid_features = env->cpuid_features; dc->cpuid_ext_features = env->cpuid_ext_features; dc->cpuid_ext2_features = env->cpuid_ext2_features;