| Message ID | 1355180372-6525-3-git-send-email-afaerber@suse.de |
|---|---|
| State | New |
| Headers | show |
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 >
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
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
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
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
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, };
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(-)