On 14 September 2017 at 05:36, Philippe Mathieu-Daudé <f4...@amsat.org> wrote: > On 09/07/2017 04:24 PM, Subbaraya Sundeep wrote: >> +static uint64_t msf2_sysreg_read(void *opaque, hwaddr offset, >> + unsigned size) >> +{ >> + MSF2SysregState *s = opaque; >> + uint32_t ret = 0; >> + >> + offset >>= 2; >> + if (offset < ARRAY_SIZE(s->regs)) { > > > This comment is controversial, I'll let Peter nod. > > The SYSREG behaves differently regarding which bus access it (CPU, AHB). > You are implementing CPU access to the SYSREG, the registers have different > permissions when accessed by the CPU. (see the SmartFusion2 User Guide: > Table 21-1 "Register Types" and Table 21-2 "Register Map"). > > I'd think of this stub: > > switch(reg) { > case register_supported1: > case register_supported2: > case register_supported3: > ret = s->regs[offset]; > trace_msf2_sysreg_read(offset, ret); > break; > case RO-U: > qemu_log_mask(LOG_GUEST_ERROR, "Illegal AHB access 0x%08"... > break; > case W1P: > qemu_log_mask(LOG_GUEST_ERROR, "Illegal read access ... > break; > case RW: > case RW-P: > case RO: > case RO-P: > default: > ret = s->regs[offset]; > qemu_log_mask(LOG_UNIMP, "... > break; > }
This doesn't look entirely right, since this is the read interface. Shouldn't we be allowing pretty much all of these register types except maybe W1P to do a read? On the other hand, in the write function we should probably not allow writes to registers documented as read only. thanks -- PMM