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

[Jan Bobek <jan.bobek@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 1830 bytes --]

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?

> 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?

> However the series is looking pretty good so far. Looking forward to the
> next iteration.

Once again, thanks a lot for the review, Alex!

-Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]