| Message ID | w4sjne6fu3.wl%peter@chubb.wattle.id.au |
|---|---|
| State | New |
| Headers | show |
On 30 September 2011 01:20, Peter Chubb <peter.chubb@nicta.com.au> 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
>>>>> "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
On 30 September 2011 10:23, Peter Chubb <peter.chubb@nicta.com.au> wrote: >>>>>> "Peter" == Peter Maydell <peter.maydell@linaro.org> writes: > 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. Unfortunately there isn't really a good infrastructure for this kind of error. At the moment we have a mix of hw_error(), printing to stderr, printing to stderr only if debug macros were enabled at compile time, and silently ignoring things, all of which have obvious drawbacks. -- PMM
Index: qemu-working/hw/arm_timer.c =================================================================== --- qemu-working.orig/hw/arm_timer.c 2011-09-29 12:40:24.657526348 +1000 +++ qemu-working/hw/arm_timer.c 2011-09-30 10:15:00.724397699 +1000 @@ -163,64 +163,108 @@ static arm_timer_state *arm_timer_init(u s->freq = freq; s->control = TIMER_CTRL_IE; bh = qemu_bh_new(arm_timer_tick, s); s->timer = ptimer_init(bh); vmstate_register(NULL, -1, &vmstate_arm_timer, s); return s; } /* ARM PrimeCell SP804 dual timer module. - Docs for this device don't seem to be publicly available. This - implementation is based on guesswork, the linux kernel sources and the - Integrator/CP timer modules. */ + * Docs at + * http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0271d/index.html +*/ typedef struct { SysBusDevice busdev; MemoryRegion iomem; arm_timer_state *timer[2]; int level[2]; qemu_irq irq; } sp804_state; +/* + * 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}; + /* Merge the IRQs from the two component devices. */ static void sp804_set_irq(void *opaque, int irq, int level) { sp804_state *s = (sp804_state *)opaque; s->level[irq] = level; qemu_set_irq(s->irq, s->level[0] || s->level[1]); } static uint64_t sp804_read(void *opaque, target_phys_addr_t offset, unsigned size) { sp804_state *s = (sp804_state *)opaque; - /* ??? Don't know the PrimeCell ID for this device. */ if (offset < 0x20) { return arm_timer_read(s->timer[0], offset); - } else { + } + if (offset < 0x40) { return arm_timer_read(s->timer[1], offset - 0x20); } + + /* + * Ids are packed into a word, then accessed one byte per word. + */ + /* TimerPeriphID */ + if (offset >= 0xfe0 && offset <= 0xfec) + return sp804_id[(offset - 0xfe0) >> 2]; + /* PrimeCellID */ + if (offset >= 0xff0 && offset <= 0xffc) + return sp804_PrimeCellID[(offset - 0xff0) >> 2] + + switch(offset){ + /* Integration Test control registers, which we won't support */ + case 0xf00: /* TimerITCR */ + case 0xf04: /* TimerITOP (strictly write only but..) */ + return 0; + + } + + hw_error("sp804_read: Bad offset %x\n", (int)offset); + return 0; } static void sp804_write(void *opaque, target_phys_addr_t offset, uint64_t value, unsigned size) { sp804_state *s = (sp804_state *)opaque; if (offset < 0x20) { arm_timer_write(s->timer[0], offset, value); - } else { + return; + } + + if (offset < 0x40) { arm_timer_write(s->timer[1], offset - 0x20, value); - } + return; + } + + /* Technically we could be writing to the Test Registers, but not likely */ + hw_error("sp804_write: Bad offset %x\n", (int)offset); } static const MemoryRegionOps sp804_ops = { .read = sp804_read, .write = sp804_write, .endianness = DEVICE_NATIVE_ENDIAN, }; static const VMStateDescription vmstate_sp804 = { .name = "sp804", @@ -262,36 +306,37 @@ typedef struct { } icp_pit_state; static uint64_t icp_pit_read(void *opaque, target_phys_addr_t offset, unsigned size) { icp_pit_state *s = (icp_pit_state *)opaque; int n; /* ??? Don't know the PrimeCell ID for this device. */ n = offset >> 8; + if (n > 3) { - hw_error("sp804_read: Bad timer %d\n", n); + hw_error("icp_pit_read: Bad timer %d\n", n); } return arm_timer_read(s->timer[n], offset & 0xff); } static void icp_pit_write(void *opaque, target_phys_addr_t offset, uint64_t value, unsigned size) { icp_pit_state *s = (icp_pit_state *)opaque; int n; n = offset >> 8; if (n > 3) { - hw_error("sp804_write: Bad timer %d\n", n); + hw_error("icp_pit_write: Bad timer %d\n", n); } arm_timer_write(s->timer[n], offset & 0xff, value); } static const MemoryRegionOps icp_pit_ops = { .read = icp_pit_read, .write = icp_pit_write, .endianness = DEVICE_NATIVE_ENDIAN, };