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

[Peter Chubb <peter.chubb@nicta.com.au>]

>>>>> "Peter" == Peter Maydell <peter.maydell@linaro.org> writes:

Peter> On 30 September 2011 01:20, Peter Chubb
Peter> <peter.chubb@nicta.com.au> wrote:
>> Thanks Peter!
>> 
>> Here's a reworked patch.

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

Thanks for the heads-up. I'll edit the patch in Quilt.

>> +/* + * 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};

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

OK, and thanks.

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

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

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

At word offsets.

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

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

Thanks.

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

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

Is there a better `tell the programmer s/he's done something stupid'
error function?  The plxxx.c files all used hw_error() for bad
offsets.


I shan't be able to get to this again until Tuesday my time.  Expect
another patch then.

Peter C
--
Dr Peter Chubb  http://www.gelato.unsw.edu.au  peterc AT gelato.unsw.edu.au
http://www.ertos.nicta.com.au           ERTOS within National ICT Australia