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
signature.asc
Description: OpenPGP digital signature
