Re: [Qemu-arm] [Qemu devel v8 PATCH 2/5] msf2: Microsemi Smartfusion2 System Register block

[Philippe Mathieu-Daudé]

> > > > +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").
>
> CPU is also one of the bus masters in AHB matrix (Fig 6.1 in page 248).

I was worried about Fabric access but Peter remembered me QEMU doesn't 
model it ;)

> > > 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 sounds good to me and will fix later by rearranging registers in enum
> sorted
> based on type (RO, RW, etc.,) and use those ranges in switch case.

The Register API (hw/register.h) is helpful to check such properties but 
might turn your code harder to read.

> > 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?
>
> Peter, some of the registers are not allowed to access by CPU and are only
> set during
> device programming. For those registers Philippe is suggesting to log
> "Illegal AHB access"
> when guest is trying to read/write.

I was worried about accessing those registers when the flash 
WriteProtect bit is set, however the eNVM flash library is not Open 
Source, it is unlikely a guest access those registers, and if it is the 
library then MicroSemi already tried it on real hardware.
There is probably no need to worry about "Illegal AHB access" :)

> > On the other hand, in the write function we should probably not allow
> > writes to registers documented as read only.

Surely.

trace_msf2_sysreg_write(offset, s->regs[offset], ret);

switch(reg) {
case register_supported1:
case register_supported2:
case register_supported3:
           s->regs[offset] = ret;
           break;
case RO:
case RO-P:
case RO-U:
           qemu_log_mask(LOG_GUEST_ERROR, "Illegal write access on RO...
           break;
default:
           qemu_log_mask(LOG_UNIMP, "...
           break;
}