Patchwork [RFC,qom-cpu,v2,2/2] target-i386: Turn Haswell into subclass of SandyBridge

login
register
mail settings
Submitter Andreas Färber
Date Dec. 10, 2012, 10:59 p.m.
Message ID <1355180372-6525-3-git-send-email-afaerber@suse.de>
Download mbox | patch
Permalink /patch/205051/
State New
Headers show

Comments

Andreas Färber - Dec. 10, 2012, 10:59 p.m.
ehabkost: "When adding the Haswell CPU model, I intended to make it
  a superset of the features present on the SandyBridge model"

Inherit from SandyBridge to keep only the delta for Haswell.

Signed-off-by: Andreas Färber <afaerber@suse.de>
Cc: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.c |   24 ++----------------------
 1 Datei geändert, 2 Zeilen hinzugefügt(+), 22 Zeilen entfernt(-)
Eduardo Habkost - Dec. 12, 2012, 12:45 p.m.
On Mon, Dec 10, 2012 at 11:59:32PM +0100, Andreas Färber wrote:
>   ehabkost: "When adding the Haswell CPU model, I intended to make it
>   a superset of the features present on the SandyBridge model"
> 
> Inherit from SandyBridge to keep only the delta for Haswell.

Most CPUs have a superset of the features of their predecessors. Are you
simply using SandyBridge->Haswell as an example, or you think their
relationship is special somehow?

I believe we don't want to make externally-visible class inheritance,
but probably just reuse constans or init functions internally. A Haswell
CPU is not a type of SandyBridge CPU, it just happens to contain a
superset of the features present in SandyBridge.

I mean: Haswell also has a superset of features of 486, but we don't
want to make the hierarchy look like the following, do we?

- X86CPU
  -> X86IntelCPU
     -> 486
        -> pentium
           -> pentium2
              -> pentium3
                 -> Conroe
                    -> Penryn
                       -> Nehalem
                          -> Westmere
                             -> SandyBridge
                                -> Haswell

> 
> Signed-off-by: Andreas Färber <afaerber@suse.de>
> Cc: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  target-i386/cpu.c |   24 ++----------------------
>  1 Datei geändert, 2 Zeilen hinzugefügt(+), 22 Zeilen entfernt(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index c59c6a5..ffd160a 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -814,39 +814,19 @@ static void haswell_cpu_class_init(ObjectClass *oc, void *data)
>  {
>      X86CPUClass *xcc = X86_CPU_CLASS(oc);
>  
> -    xcc->level = 0xd;
> -    xcc->vendor1 = CPUID_VENDOR_INTEL_1;
> -    xcc->vendor2 = CPUID_VENDOR_INTEL_2;
> -    xcc->vendor3 = CPUID_VENDOR_INTEL_3;
> -    xcc->family = 6;
>      xcc->model = 60;
> -    xcc->stepping = 1;
> -    xcc->features = CPUID_SSE2 | CPUID_SSE | CPUID_FXSR | CPUID_MMX |
> -             CPUID_CLFLUSH | CPUID_PSE36 | CPUID_PAT | CPUID_CMOV | CPUID_MCA |
> -             CPUID_PGE | CPUID_MTRR | CPUID_SEP | CPUID_APIC | CPUID_CX8 |
> -             CPUID_MCE | CPUID_PAE | CPUID_MSR | CPUID_TSC | CPUID_PSE |
> -             CPUID_DE | CPUID_FP87;
> -    xcc->ext_features = CPUID_EXT_AVX | CPUID_EXT_XSAVE | CPUID_EXT_AES |
> -             CPUID_EXT_POPCNT | CPUID_EXT_X2APIC | CPUID_EXT_SSE42 |
> -             CPUID_EXT_SSE41 | CPUID_EXT_CX16 | CPUID_EXT_SSSE3 |
> -             CPUID_EXT_PCLMULQDQ | CPUID_EXT_SSE3 |
> -             CPUID_EXT_TSC_DEADLINE_TIMER | CPUID_EXT_FMA | CPUID_EXT_MOVBE |
> -             CPUID_EXT_PCID;
> -    xcc->ext2_features = CPUID_EXT2_LM | CPUID_EXT2_RDTSCP | CPUID_EXT2_NX |
> -             CPUID_EXT2_SYSCALL;
> -    xcc->ext3_features = CPUID_EXT3_LAHF_LM;
> +    xcc->ext_features |= CPUID_EXT_FMA | CPUID_EXT_MOVBE | CPUID_EXT_PCID;
>      xcc->cpuid_7_0_ebx_features = CPUID_7_0_EBX_FSGSBASE | CPUID_7_0_EBX_BMI1 |
>              CPUID_7_0_EBX_HLE | CPUID_7_0_EBX_AVX2 | CPUID_7_0_EBX_SMEP |
>              CPUID_7_0_EBX_BMI2 | CPUID_7_0_EBX_ERMS | CPUID_7_0_EBX_INVPCID |
>              CPUID_7_0_EBX_RTM;
> -    xcc->xlevel = 0x8000000A;
>      pstrcpy(xcc->model_id, sizeof(xcc->model_id),
>              "Intel Core Processor (Haswell)");
>  }
>  
>  static const TypeInfo haswell_cpu_type_info = {
>      .name = TYPE("Haswell"),
> -    .parent = TYPE_X86_CPU,
> +    .parent = TYPE("SandyBridge"),
>      .class_init = haswell_cpu_class_init,
>  };
>  
> -- 
> 1.7.10.4
>
Andreas Färber - Dec. 12, 2012, 2:47 p.m.
Am 12.12.2012 13:45, schrieb Eduardo Habkost:
> On Mon, Dec 10, 2012 at 11:59:32PM +0100, Andreas Färber wrote:
>>   ehabkost: "When adding the Haswell CPU model, I intended to make it
>>   a superset of the features present on the SandyBridge model"
>>
>> Inherit from SandyBridge to keep only the delta for Haswell.
> 
> Most CPUs have a superset of the features of their predecessors. Are you
> simply using SandyBridge->Haswell as an example, or you think their
> relationship is special somehow?
> 
> I believe we don't want to make externally-visible class inheritance,
> but probably just reuse constans or init functions internally. A Haswell
> CPU is not a type of SandyBridge CPU, it just happens to contain a
> superset of the features present in SandyBridge.
> 
> I mean: Haswell also has a superset of features of 486, but we don't
> want to make the hierarchy look like the following, do we?

I don't see why we would want to use a #define-based inheritence as
suggested for the PPRO when we have QOM. QOM inheritence reduces lines
of code significantly compared to just taking the values from elsewhere.

For the Haswell you said what I quoted, for the other models I said I
need your or someone's help to verify whether a hierarchy such as below
is semantically right or just a coincidence. I was at least considering
an abstract intel-/amd-*-cpu to avoid repeating the three value
assignments over and over.

At this time I believe the parents of a type are not (yet) exposed via
QMP, just the "type" string property.

Andreas

> - X86CPU
>   -> X86IntelCPU
>      -> 486
>         -> pentium
>            -> pentium2
>               -> pentium3
>                  -> Conroe
>                     -> Penryn
>                        -> Nehalem
>                           -> Westmere
>                              -> SandyBridge
>                                 -> Haswell
Eduardo Habkost - Dec. 12, 2012, 3:05 p.m.
On Wed, Dec 12, 2012 at 03:47:49PM +0100, Andreas Färber wrote:
> Am 12.12.2012 13:45, schrieb Eduardo Habkost:
> > On Mon, Dec 10, 2012 at 11:59:32PM +0100, Andreas Färber wrote:
> >>   ehabkost: "When adding the Haswell CPU model, I intended to make it
> >>   a superset of the features present on the SandyBridge model"
> >>
> >> Inherit from SandyBridge to keep only the delta for Haswell.
> > 
> > Most CPUs have a superset of the features of their predecessors. Are you
> > simply using SandyBridge->Haswell as an example, or you think their
> > relationship is special somehow?
> > 
> > I believe we don't want to make externally-visible class inheritance,
> > but probably just reuse constans or init functions internally. A Haswell
> > CPU is not a type of SandyBridge CPU, it just happens to contain a
> > superset of the features present in SandyBridge.
> > 
> > I mean: Haswell also has a superset of features of 486, but we don't
> > want to make the hierarchy look like the following, do we?
> 
> I don't see why we would want to use a #define-based inheritence as
> suggested for the PPRO when we have QOM. QOM inheritence reduces lines
> of code significantly compared to just taking the values from elsewhere.

The reuse doesn't need to be #define-based (although maybe a
#define-based approach would work too), it could be function-call-based.

> 
> For the Haswell you said what I quoted, for the other models I said I
> need your or someone's help to verify whether a hierarchy such as below
> is semantically right or just a coincidence. I was at least considering
> an abstract intel-/amd-*-cpu to avoid repeating the three value
> assignments over and over.

Creating X86IntelCPU and X86AMDCPU classes make sense to me, because
Haswell is a kind of Intel CPu. Making Haswell a subclass of 486 (like
below) wouldn't.

> 
> At this time I believe the parents of a type are not (yet) exposed via
> QMP, just the "type" string property.

Even if they are not exposed externally, it's a confusing usage of
inheritance for me. I mean: a Haswell CPU is not a type of 486 CPU, it's
simply a different kind of CPU that happens to have a superset of the
486 features.

> 
> Andreas
> 
> > - X86CPU
> >   -> X86IntelCPU
> >      -> 486
> >         -> pentium
> >            -> pentium2
> >               -> pentium3
> >                  -> Conroe
> >                     -> Penryn
> >                        -> Nehalem
> >                           -> Westmere
> >                              -> SandyBridge
> >                                 -> Haswell
> 
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Andreas Färber - Dec. 12, 2012, 5:29 p.m.
Am 12.12.2012 16:05, schrieb Eduardo Habkost:
> On Wed, Dec 12, 2012 at 03:47:49PM +0100, Andreas Färber wrote:
>> Am 12.12.2012 13:45, schrieb Eduardo Habkost:
>>> On Mon, Dec 10, 2012 at 11:59:32PM +0100, Andreas Färber wrote:
>>>>   ehabkost: "When adding the Haswell CPU model, I intended to make it
>>>>   a superset of the features present on the SandyBridge model"
>>>>
>>>> Inherit from SandyBridge to keep only the delta for Haswell.
>>>
>>> Most CPUs have a superset of the features of their predecessors. Are you
>>> simply using SandyBridge->Haswell as an example, or you think their
>>> relationship is special somehow?
>>>
>>> I believe we don't want to make externally-visible class inheritance,
>>> but probably just reuse constans or init functions internally. A Haswell
>>> CPU is not a type of SandyBridge CPU, it just happens to contain a
>>> superset of the features present in SandyBridge.
>>>
>>> I mean: Haswell also has a superset of features of 486, but we don't
>>> want to make the hierarchy look like the following, do we?
>>
>> I don't see why we would want to use a #define-based inheritence as
>> suggested for the PPRO when we have QOM. QOM inheritence reduces lines
>> of code significantly compared to just taking the values from elsewhere.
> 
> The reuse doesn't need to be #define-based (although maybe a
> #define-based approach would work too), it could be function-call-based.
> 
>>
>> For the Haswell you said what I quoted, for the other models I said I
>> need your or someone's help to verify whether a hierarchy such as below
>> is semantically right or just a coincidence. I was at least considering
>> an abstract intel-/amd-*-cpu to avoid repeating the three value
>> assignments over and over.
> 
> Creating X86IntelCPU and X86AMDCPU classes make sense to me, because
> Haswell is a kind of Intel CPu. Making Haswell a subclass of 486 (like
> below) wouldn't.
> 
>>
>> At this time I believe the parents of a type are not (yet) exposed via
>> QMP, just the "type" string property.
> 
> Even if they are not exposed externally, it's a confusing usage of
> inheritance for me. I mean: a Haswell CPU is not a type of 486 CPU, it's
> simply a different kind of CPU that happens to have a superset of the
> 486 features.

I concur with your is-a argument. My patch took a pragmatic approach.

I'd like to wait for some more review comments on how to share code
between models then - I remember Paul, Anthony and Alex discussing the
x86 models a while back on IRC, CC'ing. (Summary: reading CPU models
from config files has been dropped, we only have built-in models left -
now how to design name-class mappings below X86CPU)

Andreas

>>> - X86CPU
>>>   -> X86IntelCPU
>>>      -> 486
>>>         -> pentium
>>>            -> pentium2
>>>               -> pentium3
>>>                  -> Conroe
>>>                     -> Penryn
>>>                        -> Nehalem
>>>                           -> Westmere
>>>                              -> SandyBridge
>>>                                 -> Haswell
Alexander Graf - Dec. 12, 2012, 6:21 p.m.
On 12/12/2012 06:29 PM, Andreas Färber wrote:
> Am 12.12.2012 16:05, schrieb Eduardo Habkost:
>> On Wed, Dec 12, 2012 at 03:47:49PM +0100, Andreas Färber wrote:
>>> Am 12.12.2012 13:45, schrieb Eduardo Habkost:
>>>> On Mon, Dec 10, 2012 at 11:59:32PM +0100, Andreas Färber wrote:
>>>>>    ehabkost: "When adding the Haswell CPU model, I intended to make it
>>>>>    a superset of the features present on the SandyBridge model"
>>>>>
>>>>> Inherit from SandyBridge to keep only the delta for Haswell.
>>>> Most CPUs have a superset of the features of their predecessors. Are you
>>>> simply using SandyBridge->Haswell as an example, or you think their
>>>> relationship is special somehow?
>>>>
>>>> I believe we don't want to make externally-visible class inheritance,
>>>> but probably just reuse constans or init functions internally. A Haswell
>>>> CPU is not a type of SandyBridge CPU, it just happens to contain a
>>>> superset of the features present in SandyBridge.
>>>>
>>>> I mean: Haswell also has a superset of features of 486, but we don't
>>>> want to make the hierarchy look like the following, do we?
>>> I don't see why we would want to use a #define-based inheritence as
>>> suggested for the PPRO when we have QOM. QOM inheritence reduces lines
>>> of code significantly compared to just taking the values from elsewhere.
>> The reuse doesn't need to be #define-based (although maybe a
>> #define-based approach would work too), it could be function-call-based.
>>
>>> For the Haswell you said what I quoted, for the other models I said I
>>> need your or someone's help to verify whether a hierarchy such as below
>>> is semantically right or just a coincidence. I was at least considering
>>> an abstract intel-/amd-*-cpu to avoid repeating the three value
>>> assignments over and over.
>> Creating X86IntelCPU and X86AMDCPU classes make sense to me, because
>> Haswell is a kind of Intel CPu. Making Haswell a subclass of 486 (like
>> below) wouldn't.
>>
>>> At this time I believe the parents of a type are not (yet) exposed via
>>> QMP, just the "type" string property.
>> Even if they are not exposed externally, it's a confusing usage of
>> inheritance for me. I mean: a Haswell CPU is not a type of 486 CPU, it's
>> simply a different kind of CPU that happens to have a superset of the
>> 486 features.
> I concur with your is-a argument. My patch took a pragmatic approach.

Considering that x86 is very straight forward in its CPU definitions, I 
think we can easily make this a flat model based on CPUID features + a 
few variables.

If you really want to model something, I would rather say

386
   -> 386SX
   -> 386DX
opteron-G3
   -> phenom-9550
   -> phenom-9560

Any inheritance above that level sounds excessive to me.


Alex

> I'd like to wait for some more review comments on how to share code
> between models then - I remember Paul, Anthony and Alex discussing the
> x86 models a while back on IRC, CC'ing. (Summary: reading CPU models
> from config files has been dropped, we only have built-in models left -
> now how to design name-class mappings below X86CPU)
>
> Andreas
>
>>>> - X86CPU
>>>>    ->  X86IntelCPU
>>>>       ->  486
>>>>          ->  pentium
>>>>             ->  pentium2
>>>>                ->  pentium3
>>>>                   ->  Conroe
>>>>                      ->  Penryn
>>>>                         ->  Nehalem
>>>>                            ->  Westmere
>>>>                               ->  SandyBridge
>>>>                                  ->  Haswell

Patch

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index c59c6a5..ffd160a 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -814,39 +814,19 @@  static void haswell_cpu_class_init(ObjectClass *oc, void *data)
 {
     X86CPUClass *xcc = X86_CPU_CLASS(oc);
 
-    xcc->level = 0xd;
-    xcc->vendor1 = CPUID_VENDOR_INTEL_1;
-    xcc->vendor2 = CPUID_VENDOR_INTEL_2;
-    xcc->vendor3 = CPUID_VENDOR_INTEL_3;
-    xcc->family = 6;
     xcc->model = 60;
-    xcc->stepping = 1;
-    xcc->features = CPUID_SSE2 | CPUID_SSE | CPUID_FXSR | CPUID_MMX |
-             CPUID_CLFLUSH | CPUID_PSE36 | CPUID_PAT | CPUID_CMOV | CPUID_MCA |
-             CPUID_PGE | CPUID_MTRR | CPUID_SEP | CPUID_APIC | CPUID_CX8 |
-             CPUID_MCE | CPUID_PAE | CPUID_MSR | CPUID_TSC | CPUID_PSE |
-             CPUID_DE | CPUID_FP87;
-    xcc->ext_features = CPUID_EXT_AVX | CPUID_EXT_XSAVE | CPUID_EXT_AES |
-             CPUID_EXT_POPCNT | CPUID_EXT_X2APIC | CPUID_EXT_SSE42 |
-             CPUID_EXT_SSE41 | CPUID_EXT_CX16 | CPUID_EXT_SSSE3 |
-             CPUID_EXT_PCLMULQDQ | CPUID_EXT_SSE3 |
-             CPUID_EXT_TSC_DEADLINE_TIMER | CPUID_EXT_FMA | CPUID_EXT_MOVBE |
-             CPUID_EXT_PCID;
-    xcc->ext2_features = CPUID_EXT2_LM | CPUID_EXT2_RDTSCP | CPUID_EXT2_NX |
-             CPUID_EXT2_SYSCALL;
-    xcc->ext3_features = CPUID_EXT3_LAHF_LM;
+    xcc->ext_features |= CPUID_EXT_FMA | CPUID_EXT_MOVBE | CPUID_EXT_PCID;
     xcc->cpuid_7_0_ebx_features = CPUID_7_0_EBX_FSGSBASE | CPUID_7_0_EBX_BMI1 |
             CPUID_7_0_EBX_HLE | CPUID_7_0_EBX_AVX2 | CPUID_7_0_EBX_SMEP |
             CPUID_7_0_EBX_BMI2 | CPUID_7_0_EBX_ERMS | CPUID_7_0_EBX_INVPCID |
             CPUID_7_0_EBX_RTM;
-    xcc->xlevel = 0x8000000A;
     pstrcpy(xcc->model_id, sizeof(xcc->model_id),
             "Intel Core Processor (Haswell)");
 }
 
 static const TypeInfo haswell_cpu_type_info = {
     .name = TYPE("Haswell"),
-    .parent = TYPE_X86_CPU,
+    .parent = TYPE("SandyBridge"),
     .class_init = haswell_cpu_class_init,
 };