Re: [Qemu-devel] [PATCH v2 14/22] target-mips: add Addressing and PC-relative instructions

[Leon Alrae <leon.alrae@imgtec.com>]

On 20/06/2014 21:50, Aurelien Jarno wrote:
> The patch subject is a bit misleading, as it also includes the AUI family.

Thanks for pointing this out.

>> +#if defined(TARGET_MIPS64)
>> +        case R6_OPC_LDPC: /* bits 18 and 19 are part of immediate */
>> +        case R6_OPC_LDPC + (1 << 16):
>> +        case R6_OPC_LDPC + (2 << 16):
>> +        case R6_OPC_LDPC + (3 << 16):
>> +            check_mips_64(ctx);
>> +            offset = (((int32_t)ctx->opcode << 14)) >> 11;
> 
> This will overflow the 32-bits type. I guess you want:
> 
>                offset = (((int32_t)ctx->opcode << 13)) >> 10;

I think original code is correct (LDPC offset's size is 18 bits so it
won't overflow). However, I just noticed that the comment is misleading
(there should be 'bits 16 and 17' instead of 'bits 18 and 19').

> I do wonder if we shouldn't use sextract32() instead of open coding that
> now that it is available:
> 
>                offset = sextract32(ctx->opcode, 0, 19) << 3;

This looks better, thanks for the suggestion (but since the offset's
size is 18, third argument will be 18, not 19).

>> +            addr = addr_add(ctx, (ctx->pc & ~0x7), offset);
> 
> Why do we need to mask the low 3 bits of the PC? It doesn't appear in
> the manual version I have (MD00087 version 6.00).


It doesn't appear in LDPC pseudo-code, but few lines below there is a
restriction: "LDPC is naturally aligned, by specification".
For load doubleword we need to make the address aligned to 8-byte boundary.

You can also refer to MIPS64 Volume-I (MD00083 version 6.01):
5.1.3.1 PC relative loads (Release 6)
"LDPC: Loads a 64-bit doubleword from a PC relative address, formed by
adding the PC, aligned to 8-bytes by masking off the low 3 bits, to a
sign-extended 18-bit immediate, shifted left by 3 bits, for a 21-bit span."

>> +#if defined(TARGET_MIPS64)
>> +        case OPC_DAHI:
>> +            check_insn(ctx, ISA_MIPS32R6);
>> +            check_mips_64(ctx);
>> +            if (rs != 0) {
>> +                tcg_gen_addi_i64(cpu_gpr[rs], cpu_gpr[rs], (int64_t)imm << 32);
> 
> Small nitpicking: even if it is guarded by #ifdef, in theory the _tl
> type should be used there, to match the register type.

I'll correct it.

Thanks,
Leon