Re: [Qemu-devel] [RISU PATCH v3 04/18] risugen_x86_constraints: add module

[Jan Bobek]

On 7/12/19 10:24 AM, Richard Henderson wrote:
> On 7/12/19 12:32 AM, Jan Bobek wrote:
>> +sub vex($%)
>> +{
>> +    my ($insn, %vex) = @_;
>> +    my $regidw = $is_x86_64 ? 4 : 3;
>> +
>> +    # There is no point in randomizing other VEX fields, since
>> +    # VEX.R/.X/.B are encoded automatically by risugen_x86_asm, and
>> +    # VEX.M/.P are opcodes.
>> +    $vex{l} = randint(width => 1) ? 256 : 128 unless defined $vex{l};
> 
> VEX.L is sort-of opcode-like as well.  It certainly differentiates AVX1 vs
> AVX2, and so probably should be constrained somehow.  I can't think of what's
> the best way to do that at the moment, since our existing --xstate=foo isn't 
> right.
> 
> Perhaps just a FIXME comment for now?

So, the instructions that use VEX.L specify it in the !constraints
block in the config file. Originally, I thought some instructions are
supposed to ignore it (denoted by LIG in the Intel manual -- it's the
scalar instructions like ADDSS), so it might be worth randomizing.
However, when I later read the manual pages of some of these
instructions, it said they are supposed to be encoded with VEX.L=0
anyway. I didn't check every single one of them, but right now they
are all encoded with VEX.L=0, so I suppose this line can be removed
and we can rely on the caller (the !constraints block) to always
specify it.

>> +sub modrm_($%)
>> +{
>> +    my ($insn, %args) = @_;
>> +    my $regidw = $is_x86_64 ? 4 : 3;
>> +
>> +    my %modrm = ();
>> +    if (defined $args{reg}) {
>> +        # This makes the config file syntax a bit more accommodating
>> +        # in cases where MODRM.REG is an opcode extension field.
>> +        $modrm{reg} = $args{reg};
>> +    } else {
>> +        $modrm{reg} = randint(width => $regidw);
>> +    }
>> +
>> +    # There is also a displacement-only form, but we don't know
>> +    # absolute address of the memblock, so we cannot test it.
> 
> 32-bit mode has displacement-only, aka absolute; 64-bit replaces that with
> rip-relative.  But agreed that the first is impossible to test and the second
> is difficult.
> 
>> +sub modrm($%)
>> +{
>> +    my ($insn, %args) = @_;
>> +    modrm_($insn, indexk => 'index', %args);
>> +}
> 
> How are you avoiding %rsp as index?
> I saw you die for that in the previous patch...

See write_mem_getoffset in risugen_x86.pm. I felt there's a better
place for it there, since that's when we actually need to write to it,
so the problem is more exposed.

-Jan

> 
> r~
>

signature.asc
Description: OpenPGP digital signature