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

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

[H. Peter Anvin]

Low priority ping on this patchset...?

	-hpa

Re: [Qemu-devel] [RFC PATCH 3/3] mc146818rtc: export the timezone information

[Paolo Bonzini]

Il 28/02/2013 03:12, H. Peter Anvin ha scritto:
> From: "H. Peter Anvin" <hpa@zytor.com>
> 
> There is no standard method for storing timezone information
> associated with the classic PC/AT RTC, however, there are standard
> methods in ACPI (Time and Alarm Device) and EFI (GetTime/SetTime) for
> getting this information.
> 
> Since these are abstract methods, it is qreally firmware-specific how
> it is stored, however, since Qemu initializes the RTC in the virtual
> environment that information needs to come from Qemu in the first
> place.
> 
> Non-PC platforms that use the MC146181 RTC may have their own
> firmware-specific methods as well.
> 
> The most logical place to stash this information is in the RTC CMOS;
> not only is it logically co-located with the relevant information, but
> it is also very easy to access from ACPI bytecode.  Thus, save the
> timezone information in two bytes in CMOS that have no known standard
> definition, but are yet within the 64 bytes that even the most basic
> RTC CMOS implementations including the original MC146181 support.
> 
> Note: all timezones currently in use in the world are on 15-minutes
> boundaries, which would allow this information to be stored in a
> single signed byte.  However, both EFI and ACPI use a minute-granular
> interface (specified as -1440 to +1440 with 2047 used to mean
> "unknown", this requires a minimum of 12 bits to represent); this
> follows that model.

Interesting, do you have SeaBIOS and/or OVMF patches for this?

Paolo

> Signed-off-by: H. Peter Anvin <hpa@zytor.com>
> Cc: "Kevin O'Connor" <kevin@koconnor.net>
> Cc: David Woodhouse <dwmw2@infradead.org>
> ---
>  hw/mc146818rtc.c      | 6 ++++++
>  hw/mc146818rtc_regs.h | 2 ++
>  2 files changed, 8 insertions(+)
> 
> diff --git a/hw/mc146818rtc.c b/hw/mc146818rtc.c
> index 2fb11f6..72541dd 100644
> --- a/hw/mc146818rtc.c
> +++ b/hw/mc146818rtc.c
> @@ -681,6 +681,7 @@ static void rtc_set_date_from_host(ISADevice *dev)
>  {
>      RTCState *s = DO_UPCAST(RTCState, dev, dev);
>      struct tm tm;
> +    int minuteseast;
>  
>      qemu_get_timedate(&tm, 0);
>  
> @@ -690,6 +691,11 @@ static void rtc_set_date_from_host(ISADevice *dev)
>  
>      /* set the CMOS date */
>      rtc_set_cmos(s, &tm);
> +
> +    /* Set the timezone information as a signed 16-bit number of minutes */
> +    minuteseast = ((int64_t)s->base_rtc - (int64_t)mktime(&tm)) / 60;
> +    s->cmos_data[RTC_TIMEZONE_L] = (uint8_t)(minuteseast);
> +    s->cmos_data[RTC_TIMEZONE_H] = (uint8_t)(minuteseast >> 8);
>  }
>  
>  static int rtc_post_load(void *opaque, int version_id)
> diff --git a/hw/mc146818rtc_regs.h b/hw/mc146818rtc_regs.h
> index ccdee42..7dd5e0d 100644
> --- a/hw/mc146818rtc_regs.h
> +++ b/hw/mc146818rtc_regs.h
> @@ -47,6 +47,8 @@
>  /* PC cmos mappings */
>  #define RTC_CENTURY              0x32
>  #define RTC_IBM_PS2_CENTURY_BYTE 0x37
> +#define RTC_TIMEZONE_L           0x3e
> +#define RTC_TIMEZONE_H           0x3f
>  
>  #define REG_A_UIP 0x80
>  
> 

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

[Paolo Bonzini]

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

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

[Andreas Färber]

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

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

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

[Andreas Färber]

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

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

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

[Igor Mammedov]

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,
> >      },

Re: [Qemu-devel] [RFC PATCH 3/3] mc146818rtc: export the timezone information

[H. Peter Anvin]

On 03/25/2013 02:05 AM, Paolo Bonzini wrote:
> 
> Interesting, do you have SeaBIOS and/or OVMF patches for this?
> 

Not at this point.

	-hpa

Re: [Qemu-devel] [RFC PATCH 3/3] mc146818rtc: export the timezone information

[Paolo Bonzini]

> On 03/25/2013 02:05 AM, Paolo Bonzini wrote:
> > Interesting, do you have SeaBIOS and/or OVMF patches for this?
> 
> Not at this point.

I guess you could repost 1 and 2 without the RFC tag, then.

Paolo

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

[H. Peter Anvin]

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

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

[Eduardo Habkost]

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,
> > >      },

-- 
Eduardo

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

[Eduardo Habkost]

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.

-- 
Eduardo

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

[H. Peter Anvin]

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

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

[H. Peter Anvin]

Now, all this said... if the only objection is the change of model
number for "486" then I suggest just dropping that.

	-hpa

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

[Eduardo Habkost]

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.

-- 
Eduardo

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

[H. Peter Anvin]

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

Re: [Qemu-devel] [RFC PATCH 2/3] target-i386: Raise #UD on accessing non-existent control registers

[Aurelien Jarno]

On Wed, Feb 27, 2013 at 06:12:33PM -0800, H. Peter Anvin wrote:
> From: "H. Peter Anvin" <hpa@zytor.com>
> 
> If we touch control registers that don't exist, either read or write,
> raise the #UD exception (undefined opcode).
> 
> This is useful for testing booting on old CPUs.
> 
> CR4 is assumed to exist if and only if there are CPU features other
> than the FPU defined (typically at least VME).
> 
> Signed-off-by: H. Peter Anvin <hpa@zytor.com>
> ---
>  target-i386/misc_helper.c | 25 ++++++++++++++++++++++---
>  1 file changed, 22 insertions(+), 3 deletions(-)
> 
> diff --git a/target-i386/misc_helper.c b/target-i386/misc_helper.c
> index 1ff25d1..6da3f32 100644
> --- a/target-i386/misc_helper.c
> +++ b/target-i386/misc_helper.c
> @@ -154,9 +154,18 @@ target_ulong helper_read_crN(CPUX86State *env, int reg)
>  
>      cpu_svm_check_intercept_param(env, SVM_EXIT_READ_CR0 + reg, 0);
>      switch (reg) {
> -    default:
> +    case 0:
> +    case 2:
> +    case 3:
>          val = env->cr[reg];
>          break;
> +    case 4:
> +        if (env->cpuid_features <= CPUID_FP87) {
> +            raise_exception_err(env, EXCP06_ILLOP, 0);
> +        } else {
> +            val = env->cr[reg];
> +        }
> +        break;
>      case 8:
>          if (!(env->hflags2 & HF2_VINTR_MASK)) {
>              val = cpu_get_apic_tpr(env->apic_state);
> @@ -164,6 +173,9 @@ target_ulong helper_read_crN(CPUX86State *env, int reg)
>              val = env->v_tpr;
>          }
>          break;
> +    default:
> +        raise_exception_err(env, EXCP06_ILLOP, 0);
> +        break;
>      }
>      return val;
>  }
> @@ -175,11 +187,18 @@ void helper_write_crN(CPUX86State *env, int reg, target_ulong t0)
>      case 0:
>          cpu_x86_update_cr0(env, t0);
>          break;
> +    case 2:
> +        env->cr[reg] = t0;
> +        break;
>      case 3:
>          cpu_x86_update_cr3(env, t0);
>          break;
>      case 4:
> -        cpu_x86_update_cr4(env, t0);
> +        if (env->cpuid_features <= CPUID_FP87) {
> +            raise_exception_err(env, EXCP06_ILLOP, 0);
> +        } else {
> +            cpu_x86_update_cr4(env, t0);
> +        }
>          break;
>      case 8:
>          if (!(env->hflags2 & HF2_VINTR_MASK)) {
> @@ -188,7 +207,7 @@ void helper_write_crN(CPUX86State *env, int reg, target_ulong t0)
>          env->v_tpr = t0 & 0x0f;
>          break;
>      default:
> -        env->cr[reg] = t0;
> +        raise_exception_err(env, EXCP06_ILLOP, 0);
>          break;
>      }
>  }

Reviewed-by: Aurelien Jarno <aurelien@aurel32.net>


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

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" <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.


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

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

[H. Peter Anvin]

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

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

[H. Peter Anvin]

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

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

[Rob Landley]

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

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

[H. Peter Anvin]

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

-- 
Sent from my mobile phone. Please excuse brevity and lack of formatting.

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

[H. Peter Anvin]

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

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

[Igor Mammedov]

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
> 
> 


-- 
Regards,
  Igor