Re: [PATCH 1/7] x86emul: don't special case fetching the immediate of PUSH

["Jan Beulich" <JBeulich@suse.com>]

>>> On 11.08.16 at 14:58, <andrew.cooper3@citrix.com> wrote:
> On 11/08/16 13:03, Jan Beulich wrote:
>> These immediates follow the standard patterns in all modes, so they're
>> better fetched by the generic source operand handling code.
>>
>> To facilitate testing, instead of adding yet another of these pretty
>> convoluted individual test cases, simply introduce another blowfish run
>> with -mno-accumulate-outgoing-args (the additional -Dstatic is to
>> keep the compiler from converting the calling convention to
>> "regparm(3)", which I did observe it does).
>>
>> To make this introduction of a new blowfish pass (and potential further
>> ones later one) have less impact on the readability of the final code,
>> abstract all such "binary blob" executions via a table to iterate
>> through.
>>
>> The resulting native code execution adjustment also uncovered a lack of
>> clobbers on the asm() in the 64-bit case, which is being fixed at once.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> It would be helpful if the blob refactoring was in a separate patch to
> the emulator bugfix.

There's no bug fix here really, which is why I didn't think splitting
it was strictly necessary. Can be done, but if at all possible I'd
prefer not to spend extra time on disentangling the two, the more
that in this case I'm convinced the reviewing of the split changes
wouldn't be any easier than that of them being lumped together.

>> @@ -983,20 +993,27 @@ int main(int argc, char **argv)
>>               (regs.eax != 2) || (regs.edx != 1) )
>>              goto fail;
>>          printf("okay\n");
>> -    }
>>  
>> -    printf("%-40s", "Testing blowfish native execution...");    
>> -    asm volatile (
>> +        if ( ctxt.addr_size != sizeof(void *) * CHAR_BIT )
>> +            continue;
> 
> This wants to be at the top of the loop, or we will attempt to emulate
> 64bit code in a 32bit build of the test before hitting this condition.

Certainly not: We do want to test 32-bit code in a 64-bit build,
and 64-bit code simply mustn't be included in a 32-bit build (hence
the __x86_64__ conditionals inside the blobs[] initializer). Quite
the opposite - not running 32-bit code natively here on a 64-bit
system is solely because that would require extra setup (but not
much value), not because it's impossible.

And btw., I did run the changed test on both 64-bit and 32-bit
setups, so it is proven in practice that what you think needs to
be prevented can't happen.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel