Re: [Qemu-devel] [PATCH target-arm v1 7/7] dma/pl330: implement dmaadnh instruction

[Peter Maydell]

On 11 February 2014 06:32, Peter Crosthwaite
<address@hidden> wrote:
> Implement the missing DMAADNH instruction. This is a minor variant
> of the DMAADDH instruction, so factor out to a common implementation
> for both (dmaadxh).
>
> Signed-off-by: Peter Crosthwaite <address@hidden>
> ---
>
>  hw/dma/pl330.c | 19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)
>
> diff --git a/hw/dma/pl330.c b/hw/dma/pl330.c
> index fe437cb..7acb2c4 100644
> --- a/hw/dma/pl330.c
> +++ b/hw/dma/pl330.c
> @@ -601,10 +601,12 @@ static inline void pl330_fault(PL330Chan *ch, uint32_t 
> flags)
>   *   LEN - number of elements in ARGS array
>   */
>
> -static void pl330_dmaaddh(PL330Chan *ch, uint8_t opcode, uint8_t *args, int 
> len)
> +static void pl330_dmaadxh(PL330Chan *ch, uint8_t *args, bool ra, bool neg)
>  {
> -    uint16_t im = (((uint16_t)args[1]) << 8) | ((uint16_t)args[0]);
> -    uint8_t ra = (opcode >> 1) & 1;
> +    uint32_t im = (((uint16_t)args[1]) << 8) | ((uint16_t)args[0]);

These casts look kind of weird now that you've changed the type of 'im'.
In fact, you don't need them at all since the usual integer promotions
will take them up to 'int' which we know is 32 bits for us, and we're
not trying to shift into bit 31...

> +    if (neg) {
> +        im |= 0xffff << 16;
> +    }

...speaking of which, you want a 'U' suffix on the constant, otherwise
you're doing a signed shift into the sign bit, which is undefined behaviour.

I checked the docs, and this is the correct behaviour -- we're one-extending
the immediate, not sign-extending it. Maybe this could use a brief comment:
    /* One-extend the unsigned 16 bit value to 32 bits */

thanks
-- PMM