Message ID | 1366657220-776-6-git-send-email-ehabkost@redhat.com |
---|---|
State | New |
Headers | show |
Am 22.04.2013 21:00, schrieb Eduardo Habkost: > FEAT_7_0_EBX uses ECX as input, so we have to take that into account > when reporting feature word values. > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > --- > target-i386/cpu.c | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > index 110ef98..314931e 100644 > --- a/target-i386/cpu.c > +++ b/target-i386/cpu.c > @@ -151,8 +151,10 @@ static const char *cpuid_7_0_ebx_feature_name[] = { > > typedef struct FeatureWordInfo { > const char **feat_names; > - uint32_t cpuid_eax; /* Input EAX for CPUID */ > - int cpuid_reg; /* R_* register constant */ > + uint32_t cpuid_eax; /* Input EAX for CPUID */ > + bool cpuid_needs_ecx; /* CPUID instruction uses ECX as input */ Why do we need this needs_ field here? eax and reg just seem to be reindented and the comment reworded for reg. It just seems to be passed through to has_cpuid_input_ecx, which I neither see defined nor checked - that data structure already existed elsewhere as such? Andreas > + uint32_t cpuid_ecx; /* Input ECX value for CPUID */ > + int cpuid_reg; /* output register (R_* constant) */ > } FeatureWordInfo; > > static FeatureWordInfo feature_word_info[FEATURE_WORDS] = { > @@ -186,7 +188,9 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = { > }, > [FEAT_7_0_EBX] = { > .feat_names = cpuid_7_0_ebx_feature_name, > - .cpuid_eax = 7, .cpuid_reg = R_EBX, > + .cpuid_eax = 7, > + .cpuid_needs_ecx = true, .cpuid_ecx = 0, > + .cpuid_reg = R_EBX, > }, > }; > >
On Fri, May 03, 2013 at 05:16:46PM +0200, Andreas Färber wrote: > Am 22.04.2013 21:00, schrieb Eduardo Habkost: > > FEAT_7_0_EBX uses ECX as input, so we have to take that into account > > when reporting feature word values. > > > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > > --- > > target-i386/cpu.c | 10 +++++++--- > > 1 file changed, 7 insertions(+), 3 deletions(-) > > > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > > index 110ef98..314931e 100644 > > --- a/target-i386/cpu.c > > +++ b/target-i386/cpu.c > > @@ -151,8 +151,10 @@ static const char *cpuid_7_0_ebx_feature_name[] = { > > > > typedef struct FeatureWordInfo { > > const char **feat_names; > > - uint32_t cpuid_eax; /* Input EAX for CPUID */ > > - int cpuid_reg; /* R_* register constant */ > > + uint32_t cpuid_eax; /* Input EAX for CPUID */ > > > + bool cpuid_needs_ecx; /* CPUID instruction uses ECX as input */ > > Why do we need this needs_ field here? eax and reg just seem to be > reindented and the comment reworded for reg. We need it so we know if ECX is used as input on that CPUID leaf, in other words: so we know if "cpuid-input-ecx" needs to be set on X86CPUFeatureWordInfo or not. "cpuid-input-ecx" is optional because most CPUID leafs don't depend on input ECX value, only EAX. cpuid_needs_ecx has a similar meaning to KVM_CPUID_FLAG_SIGNIFCANT_INDEX on kvm_cpuid_entry2.flags. > > It just seems to be passed through to has_cpuid_input_ecx, which I > neither see defined nor checked - that data structure already existed > elsewhere as such? X86CPUFeatureWordInfo is generated from the QAPI schema. > > Andreas > > > + uint32_t cpuid_ecx; /* Input ECX value for CPUID */ > > + int cpuid_reg; /* output register (R_* constant) */ > > } FeatureWordInfo; > > > > static FeatureWordInfo feature_word_info[FEATURE_WORDS] = { > > @@ -186,7 +188,9 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = { > > }, > > [FEAT_7_0_EBX] = { > > .feat_names = cpuid_7_0_ebx_feature_name, > > - .cpuid_eax = 7, .cpuid_reg = R_EBX, > > + .cpuid_eax = 7, > > + .cpuid_needs_ecx = true, .cpuid_ecx = 0, > > + .cpuid_reg = R_EBX, > > }, > > }; > > > > > > > -- > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany > GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Am 03.05.2013 17:54, schrieb Eduardo Habkost: > On Fri, May 03, 2013 at 05:16:46PM +0200, Andreas Färber wrote: >> Am 22.04.2013 21:00, schrieb Eduardo Habkost: >>> FEAT_7_0_EBX uses ECX as input, so we have to take that into account >>> when reporting feature word values. >>> >>> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> Thanks for the explanations, applied to qom-cpu: https://github.com/afaerber/qemu-cpu/commits/qom-cpu Andreas
diff --git a/target-i386/cpu.c b/target-i386/cpu.c index 110ef98..314931e 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -151,8 +151,10 @@ static const char *cpuid_7_0_ebx_feature_name[] = { typedef struct FeatureWordInfo { const char **feat_names; - uint32_t cpuid_eax; /* Input EAX for CPUID */ - int cpuid_reg; /* R_* register constant */ + uint32_t cpuid_eax; /* Input EAX for CPUID */ + bool cpuid_needs_ecx; /* CPUID instruction uses ECX as input */ + uint32_t cpuid_ecx; /* Input ECX value for CPUID */ + int cpuid_reg; /* output register (R_* constant) */ } FeatureWordInfo; static FeatureWordInfo feature_word_info[FEATURE_WORDS] = { @@ -186,7 +188,9 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = { }, [FEAT_7_0_EBX] = { .feat_names = cpuid_7_0_ebx_feature_name, - .cpuid_eax = 7, .cpuid_reg = R_EBX, + .cpuid_eax = 7, + .cpuid_needs_ecx = true, .cpuid_ecx = 0, + .cpuid_reg = R_EBX, }, };
FEAT_7_0_EBX uses ECX as input, so we have to take that into account when reporting feature word values. Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> --- target-i386/cpu.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)