Re: [Qemu-devel] [RISU v2 00/11] Support for i386/x86_64 with vector extensions

[Richard Henderson]

On 5/21/19 11:28 AM, Jan Bobek wrote:
> On 5/20/19 8:30 AM, Alex Bennée wrote:
>>
>> I'm not sure where my test went wrong but I guess it's around xfeatures.
>> The code says required argument but risu doesn't seem to stop me not
>> specifying it. I suspect we should default to the most minimal x86_64 we
>> can and explicitly enable extra features.
> 
> The argument is indeed required, that's taken care of by getopt: to
> test that, one can simply specify --xfeatures as the last option on
> the command-line. However, we don't check if the value successfully
> parses into an integer; is it at all possible that --xfeatures
> inadvertently swallowed the next part of your command-line? I shall
> add this check in v3.
> 
> In any case, we currently default to SSE; this seems reasonable given
> that it's an extension dating back some 20 years and pre-dates x86_64
> by 4 years (1999 vs. 2003). Opinions?

SSE2 is a mandatory part of the x86_64 ABI.

I sincerely doubt we care about testing 32-bit that does not have SSE, but even
then this patch set will not fail, as the kernel will not include the SSE
registers into the signal frame.  It would be the actual test cases for SSE
instructions that would SIGILL when run on a 32-bit guest w/o SSE.

>> Storing xfeat in the stream is a good idea so people don't mix up their
>> dumps but we probably need more validation when running in master mode
>> that the feature you have enabled is actually available on the
>> processor. Otherwise you'll potentially end up generating test streams
>> on HW with no support and just get a bunch of undef noise ;-)
> 
> Correct me if I'm mistaken, but I believe this should be enforced by
> xsave_feature_buf. There's a call to __get_cpuid_count to retrieve
> location of a given XSAVE feature in memory, which is asserted to
> complete successfully. I assume if the feature were not present, the
> assertion would fail. I guess there's a point to be made about
> release builds, in which the assert may have been optimized out; shall
> I turn it into an error message instead?

No, the assert is really an assert, because we have also masked the --xfeatures
value against the set of features stored in the signal frame.  If the kernel
reports a feature in the signal frame for which there is no cpuid leaf, then
something is very confused somewhere.

I am not sure that we can validate that the features in the signal frame match
the --xfeatures value, because I *think* that features are omitted from the
XSAVE data structure when they are in init state.  E.g. when we have not yet
exercised the feature.

This caveat is definitely true of ARM SVE, and I found that if I asserted that
all of the SVE state was in the signal frame that the generated RISU test which
uses memory would fail the 1st checkpoint, because no SVE instructions had yet
been executed.

A careful reading of the XSAVE documentation, plus some experimentation, is
definitely required.  Maybe hand-craft a test case using XRSTOR, giving it a
save area that specifies all features to be reset to init state.


r~