Re: [Qemu-devel] [PATCH] [ARM] Fix sp804 dual-timer

[Peter Maydell]

On 30 September 2011 01:20, Peter Chubb <address@hidden> wrote:
> Thanks Peter!
>
> Here's a reworked patch.

NB: when you resend patches it's better to send them as completely
fresh emails (via git-send-email or equivalent) because otherwise they're
more of a pain to apply (you end up with random email chatter in the
commit message, usually).

> +/*
> + * sp804_id should be:
> + * union {
> + * struct {
> + *   uint32_t PartNumber:12; == 0x804
> + *   uint32_t DesignerID:8; == 'A'
> + *   uint32_t Revision:4; == 1
> + *   uint32_t Configurations:6; == 0
> + * };
> + * uint8_t bytes[4];
> + * };
> + * but that gets into too many byte-ordering and packing issues.
> + */
> +static const uint8_t sp804_id[] = {0x04, 0x18, 0x14, 0};
> +static const uint8_t sp804_PrimeCellID[] = {0xB1, 0x05, 0xF0, 0x0D};

I disagree with "should be" -- yes, semantically the ID registers
have a number of subfields but for practical purposes they're just
bytes; so I don't think that comment is necessary.
There's no need to split the two sets of ID registers into
different arrays, either -- it just complicates the code.
Also you have the primecell ID values in the wrong order (check
the 'register summary' table in the docs). You want:

static const uint8_t sp804_id[] = { 0x04, 0x18, 0x14, 0, 0x0d, 0xf0,
0x05, 0xb1 };

> +    /*
> +     * Ids are packed into a word, then accessed one byte per word.
> +     */

No, they're just a set of byte registers.

> +    /* TimerPeriphID */
> +    if (offset >= 0xfe0 && offset <= 0xfec)
> +           return sp804_id[(offset - 0xfe0) >> 2];

Coding style requires braces on if statements (plus your indentation
is wrong). If you run your patch through scripts/checkpatch.pl it
will catch this sort of thing.

> +    hw_error("sp804_read: Bad offset %x\n", (int)offset);
> +    return 0;

hw_error() is a fatal error -- don't use it for conditions that
can be triggered by a malicious guest. (And since it's noreturn
there's not much point putting any code after it...)

-- PMM