Re: [Qemu-devel] [PATCH 1/6] target/mips: MXU: Add missing opcodes/decoding for LX* instructions

["Janeczek, Craig" <jancraig@amazon.com>]

Yes, both the comment and enum should be updated.

-----Original Message-----
From: Aleksandar Markovic <amarkovic@wavecomp.com> 
Sent: Thursday, December 27, 2018 2:23 PM
To: Janeczek, Craig <jancraig@amazon.com>; Stefan Markovic <smarkovic@wavecomp.com>; Aleksandar Markovic <aleksandar.markovic@rt-rk.com>; qemu-devel@nongnu.org
Subject: Re: [PATCH 1/6] target/mips: MXU: Add missing opcodes/decoding for LX* instructions

> From: Janeczek, Craig <jancraig@amazon.com>
> Sent: Thursday, December 27, 2018 7:50 PM
> To: Aleksandar Markovic; Stefan Markovic; Aleksandar Markovic; qemu-devel@nongnu.org
> Subject: RE: [PATCH 1/6] target/mips: MXU: Add missing opcodes/decoding for LX* instructions
> 
> Yes built a binary testing these instructions and ran it against HW along with a qemu binary with your > patches. The opcodes align with what I mentioned in the email.
> 

I truly appreciate your help.

Just to clarify details, beside correcting comments, does this mean that this code segment:

/*
 * MXU pool 16
 */
enum {
    OPC_MXU_D32SARW  = 0x00,
    OPC_MXU_S32ALN   = 0x01,
    OPC_MXU_S32ALNI  = 0x02,
    OPC_MXU_S32NOR   = 0x03,
    OPC_MXU_S32AND   = 0x04,
    OPC_MXU_S32OR    = 0x05,
    OPC_MXU_S32XOR   = 0x06,
    OPC_MXU_S32LUI   = 0x07,
};

should be corrected to be:

/*
 * MXU pool 16
 */
enum {
    OPC_MXU_D32SARW  = 0x00,
    OPC_MXU_S32ALN   = 0x01,
    OPC_MXU_S32ALNI  = 0x02,
    OPC_MXU_S32LUI   = 0x03,
    OPC_MXU_S32NOR   = 0x04,
    OPC_MXU_S32AND   = 0x05,
    OPC_MXU_S32OR    = 0x06,
    OPC_MXU_S32XOR   = 0x07,
};

and all things will line up for "pool 16"?

Sincerely,
Aleksandar

> -----Original Message-----
> From: Aleksandar Markovic <amarkovic@wavecomp.com>
> Subject: Re: [PATCH 1/6] target/mips: MXU: Add missing opcodes/decoding for LX* instructions
> 
> > > @@ -1663,12 +1663,21 @@ enum {
> > >    *          │                               20..18
> > >    *          ├─ 100111 ─ OPC_MXU__POOL16 ─┬─ 000 ─ OPC_MXU_D32SARW
> > >    *          │                            ├─ 001 ─ OPC_MXU_S32ALN
> > > - *          ├─ 101000 ─ OPC_MXU_LXB      ├─ 010 ─ OPC_MXU_S32ALNI
> > > - *          ├─ 101001 ─ <not assigned>   ├─ 011 ─ OPC_MXU_S32NOR
> > > - *          ├─ 101010 ─ OPC_MXU_S16LDD   ├─ 100 ─ OPC_MXU_S32AND
> > > - *          ├─ 101011 ─ OPC_MXU_S16STD   ├─ 101 ─ OPC_MXU_S32OR
> > > - *          ├─ 101100 ─ OPC_MXU_S16LDI   ├─ 110 ─ OPC_MXU_S32XOR
> > > - *          ├─ 101101 ─ OPC_MXU_S16SDI   └─ 111 ─ OPC_MXU_S32LUI
> > > + *          │                            ├─ 010 ─ OPC_MXU_S32ALNI
> > > + *          │                            ├─ 011 ─ OPC_MXU_S32NOR
> > > + *          │                            ├─ 100 ─ OPC_MXU_S32AND
> > > + *          │                            ├─ 101 ─ OPC_MXU_S32OR
> > > + *          │                            ├─ 110 ─ OPC_MXU_S32XOR
> > > + *          │                            └─ 111 ─ OPC_MXU_S32LUI
> > The opcodes for pool 16 do not look correct. I think the minor bits should look like the following.
> >
> > ┬─ 000 ─ OPC_MXU_D32SARW
> > ├─ 001 ─ OPC_MXU_S32ALN
> > ├─ 010 ─ OPC_MXU_S32ALNI
> > ├─ 011 ─ OPC_MXU_S32LUI
> > ├─ 100 ─ OPC_MXU_S32NOR
> > ├─ 101 ─ OPC_MXU_S32AND
> > ├─ 110 ─ OPC_MXU_S32OR
> > └─ 111 ─ OPC_MXU_S32XOR
> >
> 
> Hi, Craig!
> 
> My primary source for opcodes was the latest Ingenic documentation, from Ingenic's site: (dated June > 2017)
> 
> ftp://ftp.ingenic.com/SOC/M200/X1000_M200_XBurst_ISA_MXU_PM.pdf
> 
> and on page 109 there is a table in the middle of the page that contains codes that are in agreement > with what is currently in QEMU.
> 
> However, I searched more, and in a repository that seems to be derived from Android NDK r9d binutils > tree, in file
> 
> https://github.com/apportable/binutils/blob/master/opcodes/mxu-opc.c
> 
> opcodes are as you said.
> 
> I guess the definitive answer would be to involve tests on hardware, no?
> 
> Thanks for bringing this up!
> 
> Aleksandar
> 
> 
> > > + *          │
> > > + *          │                               7..5
> > > + *          ├─ 101000 ─ OPC_MXU__POOL17 ─┬─ 000 ─ OPC_MXU_LXB
> > > + *          │                            ├─ 001 ─ OPC_MXU_LXH
> > > + *          ├─ 101001 ─ <not assigned>   ├─ 011 ─ OPC_MXU_LXW
> > > + *          ├─ 101010 ─ OPC_MXU_S16LDD   ├─ 100 ─ OPC_MXU_LXBU
> > > + *          ├─ 101011 ─ OPC_MXU_S16STD   └─ 101 ─ OPC_MXU_LXHU
> > > + *          ├─ 101100 ─ OPC_MXU_S16LDI
> > > + *          ├─ 101101 ─ OPC_MXU_S16SDI
> > >    *          ├─ 101110 ─ OPC_MXU_S32M2I
> > >    *          ├─ 101111 ─ OPC_MXU_S32I2M
> > >    *          ├─ 110000 ─ OPC_MXU_D32SLL