| Message ID | 158326548299.40452.4030040738160407065.stgit@naples-babu.amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | APIC ID fixes for AMD EPYC CPU model | expand |
On Tue, 03 Mar 2020 13:58:03 -0600 Babu Moger <babu.moger@amd.com> wrote: > Load the model specific handlers if available or else default handlers > will be loaded. Add the model specific handlers if apicid decoding > differs from the standard sequential numbering. > this is still the old version of the patch and hadn't addressed feedback from v4 > Signed-off-by: Babu Moger <babu.moger@amd.com> > --- > target/i386/cpu.c | 34 ++++++++++++++++++++++++++++++++++ > target/i386/cpu.h | 1 + > 2 files changed, 35 insertions(+) > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c > index c75cf744ab..f33d8b77f5 100644 > --- a/target/i386/cpu.c > +++ b/target/i386/cpu.c > @@ -51,6 +51,7 @@ > #include "sysemu/sysemu.h" > #include "sysemu/tcg.h" > #include "hw/qdev-properties.h" > +#include "hw/i386/x86.h" this dependency shouldn't be here, see below > #include "hw/i386/topology.h" > #ifndef CONFIG_USER_ONLY > #include "exec/address-spaces.h" [...] > +void cpu_x86_init_apicid_fns(MachineState *machine) it should be something like: x86_use_epyc_apic_id_encoding(char *cpu_type) try to avoid pulling in unnecessary dependency on Machine into cpu.c > +{ > + X86CPUClass *xcc = X86_CPU_CLASS(object_class_by_name(machine->cpu_type)); > + X86CPUModel *model = xcc->model; > + X86CPUDefinition *def = model->cpudef; > + X86MachineState *x86ms = X86_MACHINE(machine); > + > + if (def) { > + if (def->apicid_from_cpu_idx) { > + x86ms->apicid_from_cpu_idx = def->apicid_from_cpu_idx; > + } > + if (def->topo_ids_from_apicid) { > + x86ms->topo_ids_from_apicid = def->topo_ids_from_apicid; > + } > + if (def->apicid_from_topo_ids) { > + x86ms->apicid_from_topo_ids = def->apicid_from_topo_ids; > + } > + if (def->apicid_pkg_offset) { > + x86ms->apicid_pkg_offset = def->apicid_pkg_offset; > + } > + } > +} It was suggested to move defaults initialization to x86_machine_class_init() as was suggested at [PATCH v4 12/16] hw/i386: Use the apicid handlers from X86MachineState and acked by Eduardo > + > static CPUCaches epyc_cache_info = { > .l1d_cache = &(CPUCacheInfo) { > .type = DATA_CACHE, > diff --git a/target/i386/cpu.h b/target/i386/cpu.h > index 20abbda647..34f0d994ef 100644 > --- a/target/i386/cpu.h > +++ b/target/i386/cpu.h > @@ -1895,6 +1895,7 @@ void cpu_clear_apic_feature(CPUX86State *env); > void host_cpuid(uint32_t function, uint32_t count, > uint32_t *eax, uint32_t *ebx, uint32_t *ecx, uint32_t *edx); > void host_vendor_fms(char *vendor, int *family, int *model, int *stepping); > +void cpu_x86_init_apicid_fns(MachineState *machine); > > /* helper.c */ > bool x86_cpu_tlb_fill(CPUState *cs, vaddr address, int size, >
On Mon, 9 Mar 2020 15:49:22 +0100 Igor Mammedov <imammedo@redhat.com> wrote: > On Tue, 03 Mar 2020 13:58:03 -0600 > Babu Moger <babu.moger@amd.com> wrote: > > > Load the model specific handlers if available or else default handlers > > will be loaded. Add the model specific handlers if apicid decoding > > differs from the standard sequential numbering. > > > > this is still the old version of the patch and hadn't addressed feedback from v4 > > > Signed-off-by: Babu Moger <babu.moger@amd.com> > > --- > > target/i386/cpu.c | 34 ++++++++++++++++++++++++++++++++++ > > target/i386/cpu.h | 1 + > > 2 files changed, 35 insertions(+) > > > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c > > index c75cf744ab..f33d8b77f5 100644 > > --- a/target/i386/cpu.c > > +++ b/target/i386/cpu.c > > @@ -51,6 +51,7 @@ > > #include "sysemu/sysemu.h" > > #include "sysemu/tcg.h" > > #include "hw/qdev-properties.h" > > +#include "hw/i386/x86.h" > this dependency shouldn't be here, see below btw: it does break build of linux-user target (one more reason to avoid machine deps) > > > #include "hw/i386/topology.h" > > #ifndef CONFIG_USER_ONLY > > #include "exec/address-spaces.h" > [...] > > +void cpu_x86_init_apicid_fns(MachineState *machine) > it should be something like: > x86_use_epyc_apic_id_encoding(char *cpu_type) > try to avoid pulling in unnecessary dependency on Machine into cpu.c > > > +{ > > + X86CPUClass *xcc = X86_CPU_CLASS(object_class_by_name(machine->cpu_type)); > > + X86CPUModel *model = xcc->model; > > + X86CPUDefinition *def = model->cpudef; > > + X86MachineState *x86ms = X86_MACHINE(machine); > > + > > + if (def) { > > + if (def->apicid_from_cpu_idx) { > > + x86ms->apicid_from_cpu_idx = def->apicid_from_cpu_idx; > > + } > > + if (def->topo_ids_from_apicid) { > > + x86ms->topo_ids_from_apicid = def->topo_ids_from_apicid; > > + } > > + if (def->apicid_from_topo_ids) { > > + x86ms->apicid_from_topo_ids = def->apicid_from_topo_ids; > > + } > > + if (def->apicid_pkg_offset) { > > + x86ms->apicid_pkg_offset = def->apicid_pkg_offset; > > + } > > + } > > +} > > It was suggested to move defaults initialization to x86_machine_class_init() > > as was suggested at > [PATCH v4 12/16] hw/i386: Use the apicid handlers from X86MachineState > and acked by Eduardo > > > + > > static CPUCaches epyc_cache_info = { > > .l1d_cache = &(CPUCacheInfo) { > > .type = DATA_CACHE, > > diff --git a/target/i386/cpu.h b/target/i386/cpu.h > > index 20abbda647..34f0d994ef 100644 > > --- a/target/i386/cpu.h > > +++ b/target/i386/cpu.h > > @@ -1895,6 +1895,7 @@ void cpu_clear_apic_feature(CPUX86State *env); > > void host_cpuid(uint32_t function, uint32_t count, > > uint32_t *eax, uint32_t *ebx, uint32_t *ecx, uint32_t *edx); > > void host_vendor_fms(char *vendor, int *family, int *model, int *stepping); > > +void cpu_x86_init_apicid_fns(MachineState *machine); > > > > /* helper.c */ > > bool x86_cpu_tlb_fill(CPUState *cs, vaddr address, int size, > > > >
On 3/9/20 9:49 AM, Igor Mammedov wrote: > On Tue, 03 Mar 2020 13:58:03 -0600 > Babu Moger <babu.moger@amd.com> wrote: > >> Load the model specific handlers if available or else default handlers >> will be loaded. Add the model specific handlers if apicid decoding >> differs from the standard sequential numbering. >> > > this is still the old version of the patch and hadn't addressed feedback from v4 Yes. I was confused little bit about it. Will fix it. > >> Signed-off-by: Babu Moger <babu.moger@amd.com> >> --- >> target/i386/cpu.c | 34 ++++++++++++++++++++++++++++++++++ >> target/i386/cpu.h | 1 + >> 2 files changed, 35 insertions(+) >> >> diff --git a/target/i386/cpu.c b/target/i386/cpu.c >> index c75cf744ab..f33d8b77f5 100644 >> --- a/target/i386/cpu.c >> +++ b/target/i386/cpu.c >> @@ -51,6 +51,7 @@ >> #include "sysemu/sysemu.h" >> #include "sysemu/tcg.h" >> #include "hw/qdev-properties.h" >> +#include "hw/i386/x86.h" > this dependency shouldn't be here, see below ok. > >> #include "hw/i386/topology.h" >> #ifndef CONFIG_USER_ONLY >> #include "exec/address-spaces.h" > [...] >> +void cpu_x86_init_apicid_fns(MachineState *machine) > it should be something like: > x86_use_epyc_apic_id_encoding(char *cpu_type) > try to avoid pulling in unnecessary dependency on Machine into cpu.c Ok. > >> +{ >> + X86CPUClass *xcc = X86_CPU_CLASS(object_class_by_name(machine->cpu_type)); >> + X86CPUModel *model = xcc->model; >> + X86CPUDefinition *def = model->cpudef; >> + X86MachineState *x86ms = X86_MACHINE(machine); >> + >> + if (def) { >> + if (def->apicid_from_cpu_idx) { >> + x86ms->apicid_from_cpu_idx = def->apicid_from_cpu_idx; >> + } >> + if (def->topo_ids_from_apicid) { >> + x86ms->topo_ids_from_apicid = def->topo_ids_from_apicid; >> + } >> + if (def->apicid_from_topo_ids) { >> + x86ms->apicid_from_topo_ids = def->apicid_from_topo_ids; >> + } >> + if (def->apicid_pkg_offset) { >> + x86ms->apicid_pkg_offset = def->apicid_pkg_offset; >> + } >> + } >> +} > > It was suggested to move defaults initialization to x86_machine_class_init() ok. We don't need the above changes. I will use the boolean as you suggested and call this function in x86_cpus_init() to initialize the EPYC specific handler. Something similar like this below.. x86_use_epyc_apic_id_encoding(char *cpu_type) { X86CPUClass *xcc = ... cpu_type ... return xcc->model->cpudef->use_epyc_apic_id_encoding } x86_cpus_init() { if (x86_use_epyc_apic_id_encoding(ms->cpu_type)) { x86ms->apicid_from_cpu_idx = ...epyc... x86ms->topo_ids_from_apicid = ...epyc... x86ms->apicid_from_topo_ids = ...epyc... x86ms->apicid_pkg_offset = ...epyc... } } Sounds right? > > as was suggested at > [PATCH v4 12/16] hw/i386: Use the apicid handlers from X86MachineState > and acked by Eduardo > >> + >> static CPUCaches epyc_cache_info = { >> .l1d_cache = &(CPUCacheInfo) { >> .type = DATA_CACHE, >> diff --git a/target/i386/cpu.h b/target/i386/cpu.h >> index 20abbda647..34f0d994ef 100644 >> --- a/target/i386/cpu.h >> +++ b/target/i386/cpu.h >> @@ -1895,6 +1895,7 @@ void cpu_clear_apic_feature(CPUX86State *env); >> void host_cpuid(uint32_t function, uint32_t count, >> uint32_t *eax, uint32_t *ebx, uint32_t *ecx, uint32_t *edx); >> void host_vendor_fms(char *vendor, int *family, int *model, int *stepping); >> +void cpu_x86_init_apicid_fns(MachineState *machine); >> >> /* helper.c */ >> bool x86_cpu_tlb_fill(CPUState *cs, vaddr address, int size, >> >
On Mon, 9 Mar 2020 14:04:39 -0500 Babu Moger <babu.moger@amd.com> wrote: > On 3/9/20 9:49 AM, Igor Mammedov wrote: > > On Tue, 03 Mar 2020 13:58:03 -0600 > > Babu Moger <babu.moger@amd.com> wrote: > > > >> Load the model specific handlers if available or else default handlers > >> will be loaded. Add the model specific handlers if apicid decoding > >> differs from the standard sequential numbering. > >> > > > > this is still the old version of the patch and hadn't addressed feedback from v4 > > Yes. I was confused little bit about it. Will fix it. > > > > >> Signed-off-by: Babu Moger <babu.moger@amd.com> > >> --- > >> target/i386/cpu.c | 34 ++++++++++++++++++++++++++++++++++ > >> target/i386/cpu.h | 1 + > >> 2 files changed, 35 insertions(+) > >> > >> diff --git a/target/i386/cpu.c b/target/i386/cpu.c > >> index c75cf744ab..f33d8b77f5 100644 > >> --- a/target/i386/cpu.c > >> +++ b/target/i386/cpu.c > >> @@ -51,6 +51,7 @@ > >> #include "sysemu/sysemu.h" > >> #include "sysemu/tcg.h" > >> #include "hw/qdev-properties.h" > >> +#include "hw/i386/x86.h" > > this dependency shouldn't be here, see below > > ok. > > > > >> #include "hw/i386/topology.h" > >> #ifndef CONFIG_USER_ONLY > >> #include "exec/address-spaces.h" > > [...] > >> +void cpu_x86_init_apicid_fns(MachineState *machine) > > it should be something like: > > x86_use_epyc_apic_id_encoding(char *cpu_type) > > try to avoid pulling in unnecessary dependency on Machine into cpu.c > > Ok. > > > > >> +{ > >> + X86CPUClass *xcc = X86_CPU_CLASS(object_class_by_name(machine->cpu_type)); > >> + X86CPUModel *model = xcc->model; > >> + X86CPUDefinition *def = model->cpudef; > >> + X86MachineState *x86ms = X86_MACHINE(machine); > >> + > >> + if (def) { > >> + if (def->apicid_from_cpu_idx) { > >> + x86ms->apicid_from_cpu_idx = def->apicid_from_cpu_idx; > >> + } > >> + if (def->topo_ids_from_apicid) { > >> + x86ms->topo_ids_from_apicid = def->topo_ids_from_apicid; > >> + } > >> + if (def->apicid_from_topo_ids) { > >> + x86ms->apicid_from_topo_ids = def->apicid_from_topo_ids; > >> + } > >> + if (def->apicid_pkg_offset) { > >> + x86ms->apicid_pkg_offset = def->apicid_pkg_offset; > >> + } > >> + } > >> +} > > > > It was suggested to move defaults initialization to x86_machine_class_init() > > ok. We don't need the above changes. I will use the boolean as you > suggested and call this function in x86_cpus_init() to initialize the EPYC > specific handler. Something similar like this below.. > > x86_use_epyc_apic_id_encoding(char *cpu_type) > { > X86CPUClass *xcc = ... cpu_type ... > return xcc->model->cpudef->use_epyc_apic_id_encoding > } > > x86_cpus_init() > { > if (x86_use_epyc_apic_id_encoding(ms->cpu_type)) { > x86ms->apicid_from_cpu_idx = ...epyc... > x86ms->topo_ids_from_apicid = ...epyc... > x86ms->apicid_from_topo_ids = ...epyc... > x86ms->apicid_pkg_offset = ...epyc... > } > } > > Sounds right? yes, something like this > > > > > as was suggested at > > [PATCH v4 12/16] hw/i386: Use the apicid handlers from X86MachineState > > and acked by Eduardo > > > >> + > >> static CPUCaches epyc_cache_info = { > >> .l1d_cache = &(CPUCacheInfo) { > >> .type = DATA_CACHE, > >> diff --git a/target/i386/cpu.h b/target/i386/cpu.h > >> index 20abbda647..34f0d994ef 100644 > >> --- a/target/i386/cpu.h > >> +++ b/target/i386/cpu.h > >> @@ -1895,6 +1895,7 @@ void cpu_clear_apic_feature(CPUX86State *env); > >> void host_cpuid(uint32_t function, uint32_t count, > >> uint32_t *eax, uint32_t *ebx, uint32_t *ecx, uint32_t *edx); > >> void host_vendor_fms(char *vendor, int *family, int *model, int *stepping); > >> +void cpu_x86_init_apicid_fns(MachineState *machine); > >> > >> /* helper.c */ > >> bool x86_cpu_tlb_fill(CPUState *cs, vaddr address, int size, > >> > > >
diff --git a/target/i386/cpu.c b/target/i386/cpu.c index c75cf744ab..f33d8b77f5 100644 --- a/target/i386/cpu.c +++ b/target/i386/cpu.c @@ -51,6 +51,7 @@ #include "sysemu/sysemu.h" #include "sysemu/tcg.h" #include "hw/qdev-properties.h" +#include "hw/i386/x86.h" #include "hw/i386/topology.h" #ifndef CONFIG_USER_ONLY #include "exec/address-spaces.h" @@ -1614,6 +1615,16 @@ typedef struct X86CPUDefinition { FeatureWordArray features; const char *model_id; CPUCaches *cache_info; + + /* Apic id specific handlers */ + uint32_t (*apicid_from_cpu_idx)(X86CPUTopoInfo *topo_info, + unsigned cpu_index); + void (*topo_ids_from_apicid)(apic_id_t apicid, X86CPUTopoInfo *topo_info, + X86CPUTopoIDs *topo_ids); + apic_id_t (*apicid_from_topo_ids)(X86CPUTopoInfo *topo_info, + const X86CPUTopoIDs *topo_ids); + uint32_t (*apicid_pkg_offset)(X86CPUTopoInfo *topo_info); + /* * Definitions for alternative versions of CPU model. * List is terminated by item with version == 0. @@ -1654,6 +1665,29 @@ static const X86CPUVersionDefinition *x86_cpu_def_get_versions(X86CPUDefinition return def->versions ?: default_version_list; } +void cpu_x86_init_apicid_fns(MachineState *machine) +{ + X86CPUClass *xcc = X86_CPU_CLASS(object_class_by_name(machine->cpu_type)); + X86CPUModel *model = xcc->model; + X86CPUDefinition *def = model->cpudef; + X86MachineState *x86ms = X86_MACHINE(machine); + + if (def) { + if (def->apicid_from_cpu_idx) { + x86ms->apicid_from_cpu_idx = def->apicid_from_cpu_idx; + } + if (def->topo_ids_from_apicid) { + x86ms->topo_ids_from_apicid = def->topo_ids_from_apicid; + } + if (def->apicid_from_topo_ids) { + x86ms->apicid_from_topo_ids = def->apicid_from_topo_ids; + } + if (def->apicid_pkg_offset) { + x86ms->apicid_pkg_offset = def->apicid_pkg_offset; + } + } +} + static CPUCaches epyc_cache_info = { .l1d_cache = &(CPUCacheInfo) { .type = DATA_CACHE, diff --git a/target/i386/cpu.h b/target/i386/cpu.h index 20abbda647..34f0d994ef 100644 --- a/target/i386/cpu.h +++ b/target/i386/cpu.h @@ -1895,6 +1895,7 @@ void cpu_clear_apic_feature(CPUX86State *env); void host_cpuid(uint32_t function, uint32_t count, uint32_t *eax, uint32_t *ebx, uint32_t *ecx, uint32_t *edx); void host_vendor_fms(char *vendor, int *family, int *model, int *stepping); +void cpu_x86_init_apicid_fns(MachineState *machine); /* helper.c */ bool x86_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
Load the model specific handlers if available or else default handlers will be loaded. Add the model specific handlers if apicid decoding differs from the standard sequential numbering. Signed-off-by: Babu Moger <babu.moger@amd.com> --- target/i386/cpu.c | 34 ++++++++++++++++++++++++++++++++++ target/i386/cpu.h | 1 + 2 files changed, 35 insertions(+)