| Message ID | 1490608598-11197-10-git-send-email-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 27/03/17 10:56, Andrew Cooper wrote: > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > --- > CC: Jan Beulich <JBeulich@suse.com> > CC: George Dunlap <george.dunlap@eu.citrix.com> > CC: Ian Jackson <Ian.Jackson@eu.citrix.com> > CC: Wei Liu <wei.liu2@citrix.com> > --- > tools/tests/x86_emulator/x86_emulate.c | 41 ++++++++++++++++++++++++---------- > 1 file changed, 29 insertions(+), 12 deletions(-) > > diff --git a/tools/tests/x86_emulator/x86_emulate.c b/tools/tests/x86_emulator/x86_emulate.c > index cea0595..2c49954 100644 > --- a/tools/tests/x86_emulator/x86_emulate.c > +++ b/tools/tests/x86_emulator/x86_emulate.c > @@ -73,20 +73,37 @@ int emul_test_cpuid( > : "a" (leaf), "c" (subleaf)); > Oh, s > /* > - * The emulator doesn't itself use MOVBE, so we can always run the > - * respective tests. > + * Some instructions and features can be emulated without specific > + * hardware support. These features are unconditionally reported here, > + * for testing and fuzzing-coverage purposes. But similarly to my question in patch 10 -- is there any chance that the emulator will ever be called with a cpuid callback that returns 'false" for these? If so, isn't there therefore a chance that there will be some sort of bug which only triggers if these bits are set to 'false'? -George
>>> On 27.03.17 at 11:56, <andrew.cooper3@citrix.com> wrote: > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > --- > CC: Jan Beulich <JBeulich@suse.com> > CC: George Dunlap <george.dunlap@eu.citrix.com> > CC: Ian Jackson <Ian.Jackson@eu.citrix.com> > CC: Wei Liu <wei.liu2@citrix.com> > --- > tools/tests/x86_emulator/x86_emulate.c | 41 > ++++++++++++++++++++++++---------- > 1 file changed, 29 insertions(+), 12 deletions(-) > > diff --git a/tools/tests/x86_emulator/x86_emulate.c > b/tools/tests/x86_emulator/x86_emulate.c > index cea0595..2c49954 100644 > --- a/tools/tests/x86_emulator/x86_emulate.c > +++ b/tools/tests/x86_emulator/x86_emulate.c > @@ -73,20 +73,37 @@ int emul_test_cpuid( > : "a" (leaf), "c" (subleaf)); > > /* > - * The emulator doesn't itself use MOVBE, so we can always run the > - * respective tests. > + * Some instructions and features can be emulated without specific > + * hardware support. These features are unconditionally reported here, > + * for testing and fuzzing-coverage purposes. > */ > - if ( leaf == 1 ) > - res->c |= 1U << 22; > - > - /* > - * The emulator doesn't itself use ADCX/ADOX/RDPID, so we can always > run > - * the respective tests. > - */ > - if ( leaf == 7 && subleaf == 0 ) > + switch ( leaf ) > { > - res->b |= 1U << 19; > - res->c |= 1U << 22; > + case 1: > + res->c |= 1U << 22; /* MOVBE */ > + break; > + > + case 7: > + switch ( subleaf ) > + { > + case 0: > + res->b |= 1U << 11; /* rtm */ Upper case? > + res->b |= 1U << 19; /* ADCX/ADOX */ > + res->b |= 1U << 20; /* STAC/CLAC */ SMAP? > + res->b |= 1U << 24; /* CLWB */ > + > + res->c |= 1U << 22; /* RDPID */ > + break; > + } > + break; > + > + case 0x80000001: > + res->c |= 1U << 4; /* cr8_legacy */ I think this one is AMD-only, just like ... > + if ( ctxt->vendor == X86_VENDOR_AMD ) > + res->c |= 1U << 7; /* misalignsse */ ... this. And what about LAHF_LM, LZCNT, and CLFLUSH{,OPT}? Jan
>>> On 27.03.17 at 13:20, <george.dunlap@citrix.com> wrote: > On 27/03/17 10:56, Andrew Cooper wrote: >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >> --- >> CC: Jan Beulich <JBeulich@suse.com> >> CC: George Dunlap <george.dunlap@eu.citrix.com> >> CC: Ian Jackson <Ian.Jackson@eu.citrix.com> >> CC: Wei Liu <wei.liu2@citrix.com> >> --- >> tools/tests/x86_emulator/x86_emulate.c | 41 ++++++++++++++++++++++++---------- >> 1 file changed, 29 insertions(+), 12 deletions(-) >> >> diff --git a/tools/tests/x86_emulator/x86_emulate.c > b/tools/tests/x86_emulator/x86_emulate.c >> index cea0595..2c49954 100644 >> --- a/tools/tests/x86_emulator/x86_emulate.c >> +++ b/tools/tests/x86_emulator/x86_emulate.c >> @@ -73,20 +73,37 @@ int emul_test_cpuid( >> : "a" (leaf), "c" (subleaf)); >> Oh, s >> /* >> - * The emulator doesn't itself use MOVBE, so we can always run the >> - * respective tests. >> + * Some instructions and features can be emulated without specific >> + * hardware support. These features are unconditionally reported here, >> + * for testing and fuzzing-coverage purposes. > > But similarly to my question in patch 10 -- is there any chance that the > emulator will ever be called with a cpuid callback that returns 'false" > for these? If so, isn't there therefore a chance that there will be > some sort of bug which only triggers if these bits are set to 'false'? I think I've suggested before that the cpuid hook should actually return void, as it can't possibly fail (now that CPUID faulting is being handled in generic code). Jan
On 27/03/17 13:13, Jan Beulich wrote: >>>> On 27.03.17 at 13:20, <george.dunlap@citrix.com> wrote: >> On 27/03/17 10:56, Andrew Cooper wrote: >>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >>> --- >>> CC: Jan Beulich <JBeulich@suse.com> >>> CC: George Dunlap <george.dunlap@eu.citrix.com> >>> CC: Ian Jackson <Ian.Jackson@eu.citrix.com> >>> CC: Wei Liu <wei.liu2@citrix.com> >>> --- >>> tools/tests/x86_emulator/x86_emulate.c | 41 ++++++++++++++++++++++++---------- >>> 1 file changed, 29 insertions(+), 12 deletions(-) >>> >>> diff --git a/tools/tests/x86_emulator/x86_emulate.c >> b/tools/tests/x86_emulator/x86_emulate.c >>> index cea0595..2c49954 100644 >>> --- a/tools/tests/x86_emulator/x86_emulate.c >>> +++ b/tools/tests/x86_emulator/x86_emulate.c >>> @@ -73,20 +73,37 @@ int emul_test_cpuid( >>> : "a" (leaf), "c" (subleaf)); >>> Oh, s >>> /* >>> - * The emulator doesn't itself use MOVBE, so we can always run the >>> - * respective tests. >>> + * Some instructions and features can be emulated without specific >>> + * hardware support. These features are unconditionally reported here, >>> + * for testing and fuzzing-coverage purposes. >> >> But similarly to my question in patch 10 -- is there any chance that the >> emulator will ever be called with a cpuid callback that returns 'false" >> for these? If so, isn't there therefore a chance that there will be >> some sort of bug which only triggers if these bits are set to 'false'? > > I think I've suggested before that the cpuid hook should actually > return void, as it can't possibly fail (now that CPUID faulting is > being handled in generic code). This isn't about failing so much as it is about reporting the presence / absence of hardware features. With this patch, cpuid unconditionally advertises the presence of a number of features (MOVBE, rtm, ADCX/ADOX, &c) because the emulation will work even if the features aren't actually present in hardware. I'm suggesting that we may want to make sure that we test *both* the "feature is present" path, *and* the "feature is missing" path. -George
On 27/03/17 13:09, Jan Beulich wrote: >>>> On 27.03.17 at 11:56, <andrew.cooper3@citrix.com> wrote: >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >> --- >> CC: Jan Beulich <JBeulich@suse.com> >> CC: George Dunlap <george.dunlap@eu.citrix.com> >> CC: Ian Jackson <Ian.Jackson@eu.citrix.com> >> CC: Wei Liu <wei.liu2@citrix.com> >> --- >> tools/tests/x86_emulator/x86_emulate.c | 41 >> ++++++++++++++++++++++++---------- >> 1 file changed, 29 insertions(+), 12 deletions(-) >> >> diff --git a/tools/tests/x86_emulator/x86_emulate.c >> b/tools/tests/x86_emulator/x86_emulate.c >> index cea0595..2c49954 100644 >> --- a/tools/tests/x86_emulator/x86_emulate.c >> +++ b/tools/tests/x86_emulator/x86_emulate.c >> @@ -73,20 +73,37 @@ int emul_test_cpuid( >> : "a" (leaf), "c" (subleaf)); >> >> /* >> - * The emulator doesn't itself use MOVBE, so we can always run the >> - * respective tests. >> + * Some instructions and features can be emulated without specific >> + * hardware support. These features are unconditionally reported here, >> + * for testing and fuzzing-coverage purposes. >> */ >> - if ( leaf == 1 ) >> - res->c |= 1U << 22; >> - >> - /* >> - * The emulator doesn't itself use ADCX/ADOX/RDPID, so we can always >> run >> - * the respective tests. >> - */ >> - if ( leaf == 7 && subleaf == 0 ) >> + switch ( leaf ) >> { >> - res->b |= 1U << 19; >> - res->c |= 1U << 22; >> + case 1: >> + res->c |= 1U << 22; /* MOVBE */ >> + break; >> + >> + case 7: >> + switch ( subleaf ) >> + { >> + case 0: >> + res->b |= 1U << 11; /* rtm */ > Upper case? I was trying to visually separate the instructions from the features. > >> + res->b |= 1U << 19; /* ADCX/ADOX */ >> + res->b |= 1U << 20; /* STAC/CLAC */ > SMAP? > >> + res->b |= 1U << 24; /* CLWB */ >> + >> + res->c |= 1U << 22; /* RDPID */ >> + break; >> + } >> + break; >> + >> + case 0x80000001: >> + res->c |= 1U << 4; /* cr8_legacy */ > I think this one is AMD-only, just like ... > >> + if ( ctxt->vendor == X86_VENDOR_AMD ) >> + res->c |= 1U << 7; /* misalignsse */ > ... this. The difference is that cr8_legacy is a straight "is this specific encoding valid to use", while misalignsse causes rather larger changes in how SSE arguments get handled. > And what about LAHF_LM, LZCNT, and CLFLUSH{,OPT}? This list was only the instructions I had encountered. I can add all of these. ~Andrew
On 27/03/17 13:56, George Dunlap wrote: > On 27/03/17 13:13, Jan Beulich wrote: >>>>> On 27.03.17 at 13:20, <george.dunlap@citrix.com> wrote: >>> On 27/03/17 10:56, Andrew Cooper wrote: >>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >>>> --- >>>> CC: Jan Beulich <JBeulich@suse.com> >>>> CC: George Dunlap <george.dunlap@eu.citrix.com> >>>> CC: Ian Jackson <Ian.Jackson@eu.citrix.com> >>>> CC: Wei Liu <wei.liu2@citrix.com> >>>> --- >>>> tools/tests/x86_emulator/x86_emulate.c | 41 ++++++++++++++++++++++++---------- >>>> 1 file changed, 29 insertions(+), 12 deletions(-) >>>> >>>> diff --git a/tools/tests/x86_emulator/x86_emulate.c >>> b/tools/tests/x86_emulator/x86_emulate.c >>>> index cea0595..2c49954 100644 >>>> --- a/tools/tests/x86_emulator/x86_emulate.c >>>> +++ b/tools/tests/x86_emulator/x86_emulate.c >>>> @@ -73,20 +73,37 @@ int emul_test_cpuid( >>>> : "a" (leaf), "c" (subleaf)); >>>> Oh, s >>>> /* >>>> - * The emulator doesn't itself use MOVBE, so we can always run the >>>> - * respective tests. >>>> + * Some instructions and features can be emulated without specific >>>> + * hardware support. These features are unconditionally reported here, >>>> + * for testing and fuzzing-coverage purposes. >>> But similarly to my question in patch 10 -- is there any chance that the >>> emulator will ever be called with a cpuid callback that returns 'false" >>> for these? If so, isn't there therefore a chance that there will be >>> some sort of bug which only triggers if these bits are set to 'false'? >> I think I've suggested before that the cpuid hook should actually >> return void, as it can't possibly fail (now that CPUID faulting is >> being handled in generic code). > This isn't about failing so much as it is about reporting the presence / > absence of hardware features. With this patch, cpuid unconditionally > advertises the presence of a number of features (MOVBE, rtm, ADCX/ADOX, > &c) because the emulation will work even if the features aren't actually > present in hardware. I'm suggesting that we may want to make sure that > we test *both* the "feature is present" path, *and* the "feature is > missing" path. I have some plans to make this happen, but it isn't easy with the existing infrastructure. In the meantime, It is more important to get better coverage. ~Andrew
On 27/03/17 14:03, Andrew Cooper wrote: > On 27/03/17 13:56, George Dunlap wrote: >> On 27/03/17 13:13, Jan Beulich wrote: >>>>>> On 27.03.17 at 13:20, <george.dunlap@citrix.com> wrote: >>>> On 27/03/17 10:56, Andrew Cooper wrote: >>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >>>>> --- >>>>> CC: Jan Beulich <JBeulich@suse.com> >>>>> CC: George Dunlap <george.dunlap@eu.citrix.com> >>>>> CC: Ian Jackson <Ian.Jackson@eu.citrix.com> >>>>> CC: Wei Liu <wei.liu2@citrix.com> >>>>> --- >>>>> tools/tests/x86_emulator/x86_emulate.c | 41 ++++++++++++++++++++++++---------- >>>>> 1 file changed, 29 insertions(+), 12 deletions(-) >>>>> >>>>> diff --git a/tools/tests/x86_emulator/x86_emulate.c >>>> b/tools/tests/x86_emulator/x86_emulate.c >>>>> index cea0595..2c49954 100644 >>>>> --- a/tools/tests/x86_emulator/x86_emulate.c >>>>> +++ b/tools/tests/x86_emulator/x86_emulate.c >>>>> @@ -73,20 +73,37 @@ int emul_test_cpuid( >>>>> : "a" (leaf), "c" (subleaf)); >>>>> Oh, s >>>>> /* >>>>> - * The emulator doesn't itself use MOVBE, so we can always run the >>>>> - * respective tests. >>>>> + * Some instructions and features can be emulated without specific >>>>> + * hardware support. These features are unconditionally reported here, >>>>> + * for testing and fuzzing-coverage purposes. >>>> But similarly to my question in patch 10 -- is there any chance that the >>>> emulator will ever be called with a cpuid callback that returns 'false" >>>> for these? If so, isn't there therefore a chance that there will be >>>> some sort of bug which only triggers if these bits are set to 'false'? >>> I think I've suggested before that the cpuid hook should actually >>> return void, as it can't possibly fail (now that CPUID faulting is >>> being handled in generic code). >> This isn't about failing so much as it is about reporting the presence / >> absence of hardware features. With this patch, cpuid unconditionally >> advertises the presence of a number of features (MOVBE, rtm, ADCX/ADOX, >> &c) because the emulation will work even if the features aren't actually >> present in hardware. I'm suggesting that we may want to make sure that >> we test *both* the "feature is present" path, *and* the "feature is >> missing" path. > > I have some plans to make this happen, but it isn't easy with the > existing infrastructure. In the meantime, It is more important to get > better coverage. That sounds reasonable. Acked-by: George Dunlap <george.dunlap@citrix.com>
On 27/03/17 13:13, Jan Beulich wrote: >>>> On 27.03.17 at 13:20, <george.dunlap@citrix.com> wrote: >> On 27/03/17 10:56, Andrew Cooper wrote: >>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >>> --- >>> CC: Jan Beulich <JBeulich@suse.com> >>> CC: George Dunlap <george.dunlap@eu.citrix.com> >>> CC: Ian Jackson <Ian.Jackson@eu.citrix.com> >>> CC: Wei Liu <wei.liu2@citrix.com> >>> --- >>> tools/tests/x86_emulator/x86_emulate.c | 41 ++++++++++++++++++++++++---------- >>> 1 file changed, 29 insertions(+), 12 deletions(-) >>> >>> diff --git a/tools/tests/x86_emulator/x86_emulate.c >> b/tools/tests/x86_emulator/x86_emulate.c >>> index cea0595..2c49954 100644 >>> --- a/tools/tests/x86_emulator/x86_emulate.c >>> +++ b/tools/tests/x86_emulator/x86_emulate.c >>> @@ -73,20 +73,37 @@ int emul_test_cpuid( >>> : "a" (leaf), "c" (subleaf)); >>> Oh, s >>> /* >>> - * The emulator doesn't itself use MOVBE, so we can always run the >>> - * respective tests. >>> + * Some instructions and features can be emulated without specific >>> + * hardware support. These features are unconditionally reported here, >>> + * for testing and fuzzing-coverage purposes. >> But similarly to my question in patch 10 -- is there any chance that the >> emulator will ever be called with a cpuid callback that returns 'false" >> for these? If so, isn't there therefore a chance that there will be >> some sort of bug which only triggers if these bits are set to 'false'? > I think I've suggested before that the cpuid hook should actually > return void, as it can't possibly fail (now that CPUID faulting is > being handled in generic code). I've been considering this quite a lot recently. One the one hand, the introspection hook for CPUID really ought to be using X86EMUL_RETRY. On the other, we really are (ab)using the existing cpuid() hook for two different purposes. There really is a conceptual difference between issuing a cpuid() call as part of emulating a CPUID instruction, and using it to find out whether other instructions are permitted. The latter is synonymous to having or not having the requisite piece of silicon, and isn't something which can fail. In light of the new struct cpuid_policy, I was considering exposing that to the emulator, so the feature checks are straight bit tests. This would separate the two different purposes we have, and should reduce the size of x86_emulate() a little (At the moment, it is 1/4 MB compiled, or about 1/6th of the entire hypervisor.) Furthermore, the soon-to-appear struct msr_policy would help resolve our speculative MSR read issues in a similar way. ~Andrew
>>> On 27.03.17 at 15:01, <andrew.cooper3@citrix.com> wrote: > On 27/03/17 13:09, Jan Beulich wrote: >>>>> On 27.03.17 at 11:56, <andrew.cooper3@citrix.com> wrote: >>> --- a/tools/tests/x86_emulator/x86_emulate.c >>> +++ b/tools/tests/x86_emulator/x86_emulate.c >>> @@ -73,20 +73,37 @@ int emul_test_cpuid( >>> : "a" (leaf), "c" (subleaf)); >>> >>> /* >>> - * The emulator doesn't itself use MOVBE, so we can always run the >>> - * respective tests. >>> + * Some instructions and features can be emulated without specific >>> + * hardware support. These features are unconditionally reported here, >>> + * for testing and fuzzing-coverage purposes. >>> */ >>> - if ( leaf == 1 ) >>> - res->c |= 1U << 22; >>> - >>> - /* >>> - * The emulator doesn't itself use ADCX/ADOX/RDPID, so we can always >>> run >>> - * the respective tests. >>> - */ >>> - if ( leaf == 7 && subleaf == 0 ) >>> + switch ( leaf ) >>> { >>> - res->b |= 1U << 19; >>> - res->c |= 1U << 22; >>> + case 1: >>> + res->c |= 1U << 22; /* MOVBE */ >>> + break; >>> + >>> + case 7: >>> + switch ( subleaf ) >>> + { >>> + case 0: >>> + res->b |= 1U << 11; /* rtm */ >> Upper case? > > I was trying to visually separate the instructions from the features. Oh, I see. Fine with me then, i.e. I'd adjust ... >>> + res->b |= 1U << 19; /* ADCX/ADOX */ >>> + res->b |= 1U << 20; /* STAC/CLAC */ >> SMAP? ... this to "smap?" >>> + res->b |= 1U << 24; /* CLWB */ >>> + >>> + res->c |= 1U << 22; /* RDPID */ >>> + break; >>> + } >>> + break; >>> + >>> + case 0x80000001: >>> + res->c |= 1U << 4; /* cr8_legacy */ >> I think this one is AMD-only, just like ... >> >>> + if ( ctxt->vendor == X86_VENDOR_AMD ) >>> + res->c |= 1U << 7; /* misalignsse */ >> ... this. > > The difference is that cr8_legacy is a straight "is this specific > encoding valid to use", while misalignsse causes rather larger changes > in how SSE arguments get handled. True. >> And what about LAHF_LM, LZCNT, and CLFLUSH{,OPT}? > > This list was only the instructions I had encountered. I can add all of > these. Yes please (subject to the other sub-thread). Jan
>>> On 27.03.17 at 15:03, <andrew.cooper3@citrix.com> wrote: > On 27/03/17 13:56, George Dunlap wrote: >> On 27/03/17 13:13, Jan Beulich wrote: >>>>>> On 27.03.17 at 13:20, <george.dunlap@citrix.com> wrote: >>>> On 27/03/17 10:56, Andrew Cooper wrote: >>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >>>>> --- >>>>> CC: Jan Beulich <JBeulich@suse.com> >>>>> CC: George Dunlap <george.dunlap@eu.citrix.com> >>>>> CC: Ian Jackson <Ian.Jackson@eu.citrix.com> >>>>> CC: Wei Liu <wei.liu2@citrix.com> >>>>> --- >>>>> tools/tests/x86_emulator/x86_emulate.c | 41 ++++++++++++++++++++++++---------- >>>>> 1 file changed, 29 insertions(+), 12 deletions(-) >>>>> >>>>> diff --git a/tools/tests/x86_emulator/x86_emulate.c >>>> b/tools/tests/x86_emulator/x86_emulate.c >>>>> index cea0595..2c49954 100644 >>>>> --- a/tools/tests/x86_emulator/x86_emulate.c >>>>> +++ b/tools/tests/x86_emulator/x86_emulate.c >>>>> @@ -73,20 +73,37 @@ int emul_test_cpuid( >>>>> : "a" (leaf), "c" (subleaf)); >>>>> Oh, s >>>>> /* >>>>> - * The emulator doesn't itself use MOVBE, so we can always run the >>>>> - * respective tests. >>>>> + * Some instructions and features can be emulated without specific >>>>> + * hardware support. These features are unconditionally reported here, >>>>> + * for testing and fuzzing-coverage purposes. >>>> But similarly to my question in patch 10 -- is there any chance that the >>>> emulator will ever be called with a cpuid callback that returns 'false" >>>> for these? If so, isn't there therefore a chance that there will be >>>> some sort of bug which only triggers if these bits are set to 'false'? >>> I think I've suggested before that the cpuid hook should actually >>> return void, as it can't possibly fail (now that CPUID faulting is >>> being handled in generic code). >> This isn't about failing so much as it is about reporting the presence / >> absence of hardware features. With this patch, cpuid unconditionally >> advertises the presence of a number of features (MOVBE, rtm, ADCX/ADOX, >> &c) because the emulation will work even if the features aren't actually >> present in hardware. I'm suggesting that we may want to make sure that >> we test *both* the "feature is present" path, *and* the "feature is >> missing" path. > > I have some plans to make this happen, but it isn't easy with the > existing infrastructure. In the meantime, It is more important to get > better coverage. Can't we simply grab enough bits of input data to cover the ones of interest here, store them away, and use that instead of the hard coded 1s here? Jan
>>> On 27.03.17 at 15:37, <andrew.cooper3@citrix.com> wrote: > On 27/03/17 13:13, Jan Beulich wrote: >>>>> On 27.03.17 at 13:20, <george.dunlap@citrix.com> wrote: >>> On 27/03/17 10:56, Andrew Cooper wrote: >>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >>>> --- >>>> CC: Jan Beulich <JBeulich@suse.com> >>>> CC: George Dunlap <george.dunlap@eu.citrix.com> >>>> CC: Ian Jackson <Ian.Jackson@eu.citrix.com> >>>> CC: Wei Liu <wei.liu2@citrix.com> >>>> --- >>>> tools/tests/x86_emulator/x86_emulate.c | 41 > ++++++++++++++++++++++++---------- >>>> 1 file changed, 29 insertions(+), 12 deletions(-) >>>> >>>> diff --git a/tools/tests/x86_emulator/x86_emulate.c >>> b/tools/tests/x86_emulator/x86_emulate.c >>>> index cea0595..2c49954 100644 >>>> --- a/tools/tests/x86_emulator/x86_emulate.c >>>> +++ b/tools/tests/x86_emulator/x86_emulate.c >>>> @@ -73,20 +73,37 @@ int emul_test_cpuid( >>>> : "a" (leaf), "c" (subleaf)); >>>> Oh, s >>>> /* >>>> - * The emulator doesn't itself use MOVBE, so we can always run the >>>> - * respective tests. >>>> + * Some instructions and features can be emulated without specific >>>> + * hardware support. These features are unconditionally reported here, >>>> + * for testing and fuzzing-coverage purposes. >>> But similarly to my question in patch 10 -- is there any chance that the >>> emulator will ever be called with a cpuid callback that returns 'false" >>> for these? If so, isn't there therefore a chance that there will be >>> some sort of bug which only triggers if these bits are set to 'false'? >> I think I've suggested before that the cpuid hook should actually >> return void, as it can't possibly fail (now that CPUID faulting is >> being handled in generic code). > > I've been considering this quite a lot recently. One the one hand, the > introspection hook for CPUID really ought to be using X86EMUL_RETRY. > > On the other, we really are (ab)using the existing cpuid() hook for two > different purposes. There really is a conceptual difference between > issuing a cpuid() call as part of emulating a CPUID instruction, and > using it to find out whether other instructions are permitted. The > latter is synonymous to having or not having the requisite piece of > silicon, and isn't something which can fail. There may be a semantic difference, but a conceptual one? CPUID insns can't fail either (with CPUID faulting out of the picture). Jan
On 27/03/17 14:42, Jan Beulich wrote: >>>> On 27.03.17 at 15:03, <andrew.cooper3@citrix.com> wrote: >> On 27/03/17 13:56, George Dunlap wrote: >>> On 27/03/17 13:13, Jan Beulich wrote: >>>>>>> On 27.03.17 at 13:20, <george.dunlap@citrix.com> wrote: >>>>> On 27/03/17 10:56, Andrew Cooper wrote: >>>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >>>>>> --- >>>>>> CC: Jan Beulich <JBeulich@suse.com> >>>>>> CC: George Dunlap <george.dunlap@eu.citrix.com> >>>>>> CC: Ian Jackson <Ian.Jackson@eu.citrix.com> >>>>>> CC: Wei Liu <wei.liu2@citrix.com> >>>>>> --- >>>>>> tools/tests/x86_emulator/x86_emulate.c | 41 ++++++++++++++++++++++++---------- >>>>>> 1 file changed, 29 insertions(+), 12 deletions(-) >>>>>> >>>>>> diff --git a/tools/tests/x86_emulator/x86_emulate.c >>>>> b/tools/tests/x86_emulator/x86_emulate.c >>>>>> index cea0595..2c49954 100644 >>>>>> --- a/tools/tests/x86_emulator/x86_emulate.c >>>>>> +++ b/tools/tests/x86_emulator/x86_emulate.c >>>>>> @@ -73,20 +73,37 @@ int emul_test_cpuid( >>>>>> : "a" (leaf), "c" (subleaf)); >>>>>> Oh, s >>>>>> /* >>>>>> - * The emulator doesn't itself use MOVBE, so we can always run the >>>>>> - * respective tests. >>>>>> + * Some instructions and features can be emulated without specific >>>>>> + * hardware support. These features are unconditionally reported here, >>>>>> + * for testing and fuzzing-coverage purposes. >>>>> But similarly to my question in patch 10 -- is there any chance that the >>>>> emulator will ever be called with a cpuid callback that returns 'false" >>>>> for these? If so, isn't there therefore a chance that there will be >>>>> some sort of bug which only triggers if these bits are set to 'false'? >>>> I think I've suggested before that the cpuid hook should actually >>>> return void, as it can't possibly fail (now that CPUID faulting is >>>> being handled in generic code). >>> This isn't about failing so much as it is about reporting the presence / >>> absence of hardware features. With this patch, cpuid unconditionally >>> advertises the presence of a number of features (MOVBE, rtm, ADCX/ADOX, >>> &c) because the emulation will work even if the features aren't actually >>> present in hardware. I'm suggesting that we may want to make sure that >>> we test *both* the "feature is present" path, *and* the "feature is >>> missing" path. >> I have some plans to make this happen, but it isn't easy with the >> existing infrastructure. In the meantime, It is more important to get >> better coverage. > Can't we simply grab enough bits of input data to cover the ones > of interest here, store them away, and use that instead of the > hard coded 1s here? Yes, but I also want to cover hiding features which are present on the CPU as well, to test those early #UD paths. ~Andrew
diff --git a/tools/tests/x86_emulator/x86_emulate.c b/tools/tests/x86_emulator/x86_emulate.c index cea0595..2c49954 100644 --- a/tools/tests/x86_emulator/x86_emulate.c +++ b/tools/tests/x86_emulator/x86_emulate.c @@ -73,20 +73,37 @@ int emul_test_cpuid( : "a" (leaf), "c" (subleaf)); /* - * The emulator doesn't itself use MOVBE, so we can always run the - * respective tests. + * Some instructions and features can be emulated without specific + * hardware support. These features are unconditionally reported here, + * for testing and fuzzing-coverage purposes. */ - if ( leaf == 1 ) - res->c |= 1U << 22; - - /* - * The emulator doesn't itself use ADCX/ADOX/RDPID, so we can always run - * the respective tests. - */ - if ( leaf == 7 && subleaf == 0 ) + switch ( leaf ) { - res->b |= 1U << 19; - res->c |= 1U << 22; + case 1: + res->c |= 1U << 22; /* MOVBE */ + break; + + case 7: + switch ( subleaf ) + { + case 0: + res->b |= 1U << 11; /* rtm */ + res->b |= 1U << 19; /* ADCX/ADOX */ + res->b |= 1U << 20; /* STAC/CLAC */ + res->b |= 1U << 24; /* CLWB */ + + res->c |= 1U << 22; /* RDPID */ + break; + } + break; + + case 0x80000001: + res->c |= 1U << 4; /* cr8_legacy */ + + if ( ctxt->vendor == X86_VENDOR_AMD ) + res->c |= 1U << 7; /* misalignsse */ + + break; } return X86EMUL_OKAY;
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: George Dunlap <george.dunlap@eu.citrix.com> CC: Ian Jackson <Ian.Jackson@eu.citrix.com> CC: Wei Liu <wei.liu2@citrix.com> --- tools/tests/x86_emulator/x86_emulate.c | 41 ++++++++++++++++++++++++---------- 1 file changed, 29 insertions(+), 12 deletions(-)