On 19.09.2017 19:47, Richard Henderson wrote:
> On 09/19/2017 09:26 AM, David Hildenbrand wrote:
>> + const uint8_t fc = env->regs[0] & 0x7fULL;
>
> Don't mask here...
Bit 56 is the mod bit (see variable "mod") and is checked inside the
switch(). The function code really is just composed of bit 57 - 63, so
this is correct.
>
>> + if (fc >= 128 || !test_be_bit(fc, subfunc)) {
... however the fc >= 128 case can be dropped, as this can in fact never
happen (due to the masking).
>> + cpu_restore_state(cs, ra);
>> + program_interrupt(env, PGM_SPECIFICATION, 4);
>> + return 0;
>> + }
>
> ... because you are indeed supposed to test bit 56. But you've already
> ensured
> that won't be set above.
>
> Do you in fact need to set bit 63 in subfunc, since that itself is query?
> Certainly you won't get past this test, regardless...
Yes, it always has to be set and is guaranteed by s390_get_feat_block().
So test_be_bit(fc, subfunc) will always allow bit 0 (query).
Especially due to the statement:
"When a bit is one, the corresponding function is installed; otherwise,
the function is not installed."
Query is always installed.
>
>
>> +static ExitStatus op_msa(DisasContext *s, DisasOps *o)
>> +{
>> + int r1 = have_field(s->fields, r1) ? get_field(s->fields, r1) : 0;
>> + int r2 = have_field(s->fields, r2) ? get_field(s->fields, r2) : 0;
>> + int r3 = have_field(s->fields, r3) ? get_field(s->fields, r3) : 0;
>> + TCGv_i32 t_r1, t_r2, t_r3, type;
>> +
>> + switch (s->insn->data) {
>> + case S390_FEAT_TYPE_KMCTR:
>> + if (r3 & 1 || !r3) {
>> + gen_program_exception(s, PGM_SPECIFICATION);
>> + return EXIT_NORETURN;
>> + }
>> + /* FALL THROUGH */
>> + case S390_FEAT_TYPE_PPNO:
>> + case S390_FEAT_TYPE_KMF:
>> + case S390_FEAT_TYPE_KMC:
>> + case S390_FEAT_TYPE_KMO:
>> + case S390_FEAT_TYPE_KM:
>> + if (r1 & 1 || !r1) {
>> + gen_program_exception(s, PGM_SPECIFICATION);
>> + return EXIT_NORETURN;
>> + }
>> + /* FALL THROUGH */
>> + case S390_FEAT_TYPE_KMAC:
>> + case S390_FEAT_TYPE_KIMD:
>> + case S390_FEAT_TYPE_KLMD:
>> + if (r2 & 1 || !r2) {
>> + gen_program_exception(s, PGM_SPECIFICATION);
>> + return EXIT_NORETURN;
>> + }
>
> Even though it would be silly to do so, considering that r0+r1 are implicit
> arguments to these insns, I see nothing that insists that r[123] are not 0,
> only that they are even.
>
E.g. for KMAC (7-187):
"A specification exception is recognized and no other
action is taken if any of the following occurs:
[...]
The R2 field designates an odd-numbered regis-
ter or general register 0."
Or KMCTR (7-103):
"A specification exception is recognized and no other
action is taken if any of the following occurs:
[...]
The R1 R2, or R3 field designates an odd-num-
bered register or general register 0."
I implemented kvm-unit-tests that test all of these conditions.
Especially they also pass on KVM, so I assume they are correct (as these
instructions are fully interpreted by HW).
https://www.spinics.net/lists/kvm/msg155690.html
In the test included is
a) checking that the mod bit must not be set for certain instructions
b) r1-3 either odd or 0 for certain instructions
c) that query is always indicated
Thanks for the review!
>
> r~
>
--
Thanks,
David
