Re: [Qemu-devel] [RFC PATCH 1/3] target-i386: Add 486sx, old486, and old486sx CPU models

[Aurelien Jarno]

On Wed, Feb 27, 2013 at 06:12:32PM -0800, H. Peter Anvin wrote:
> From: "H. Peter Anvin" <address@hidden>
> 
> 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 <address@hidden>
> ---
>  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.


-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
address@hidden                 http://www.aurel32.net