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

[sundeep subbaraya]

Hi Philippe,

On Thu, Sep 14, 2017 at 6:43 PM, Peter Maydell <address@hidden> wrote:

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").

CPU is also one of the bus masters in AHB matrix (Fig 6.1 in page 248).

>
> 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.

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.

Thanks,

Sundeep

 

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

thanks
-- PMM