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

[Peter Maydell]

On 14 September 2017 at 05:36, Philippe Mathieu-Daudé <address@hidden> 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