Re: [Qemu-devel] [RISU RFC PATCH v2 02/14] risugen_x86_asm: add module

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

[-- Attachment #1.1: Type: text/plain, Size: 2344 bytes --]

On 7/3/19 11:37 AM, Richard Henderson wrote:
> On 7/1/19 6:35 AM, Jan Bobek wrote:
>> +    VEX_V_UNUSED => 0b1111,
> 
> I think perhaps this is a mistake.  Yes, that's what goes in the field, but
> what goes in the field is ~(logical_value).
> 
> While for general RISU-ish operation, ~(random_number) is just as random as
> +(random_number), the difference will be if we ever want to explicitly emit
> with this interface a specific vex instruction which also requires the v-register.

See below.

>> +sub rex_encode(%)
>> +{
>> +    my (%args) = @_;
>> +
>> +    $args{w} = 0 unless defined $args{w};
>> +    $args{r} = 0 unless defined $args{r};
>> +    $args{x} = 0 unless defined $args{x};
>> +    $args{b} = 0 unless defined $args{b};
>> +
>> +    return (value => 0x40
>> +            | (($args{w} ? 1 : 0) << 3)
>> +            | (($args{r} ? 1 : 0) << 2)
>> +            | (($args{x} ? 1 : 0) << 1)
>> +            | ($args{b} ? 1 : 0),
>> +            len => 1);
>> +}
> 
> Does
> 
> 	(defined $args{w} && $args{w}) << 3
> 
> work?  That seems tidier to me than splitting these conditions.

It does, I will change it. Thanks!

>> +        return (value => (0xC4 << 16)
>> +                | (($args{r} ? 1 : 0) << 15)
>> +                | (($args{x} ? 1 : 0) << 14)
>> +                | (($args{b} ? 1 : 0) << 13)
> 
> Further down in vex_encode, and along the lines of VEX_V_UNUSED, this appears
> to be actively wrong, since these bits are encoded as inverses.  What this
> *really* means is that because of that, rex_encode and vex_encode will not
> encode the same registers for a given instruction.  Which really does feel
> bug-like, random inputs or no.

So, vex_encode, rex_encode and friends were meant to be really
low-level functions; they literally just encode the bits from what you
pass in, without any concern for what the fields even mean. In that
spirit, write_insn itself never did much of error-checking.

I have added quite a lot of code to risugen_x86_asm in v3; most
importantly, there are now asm_insn_* functions which are more
high-level, in that you pass in the logical values and they care of
error checks and encoding. I also removed write_insn and all the
encoding-related symbolic constants from the public interface of the
module.

-Jan


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