Re: [Qemu-devel] [PATCH 2/2] target-arm: Add checks that cpreg raw accesses are handled

[Peter Maydell]

On 10 December 2014 at 22:26, Greg Bellows <address@hidden> wrote:
>
>
> On 9 December 2014 at 13:46, Peter Maydell <address@hidden> wrote:
>> +static bool raw_accessors_valid(const ARMCPRegInfo *ri)
>> +{
>> +    /* Return true if a raw access on this register is OK (ie will not
>> +     * fall into the assert in raw_read() or raw_write())
>> +     */
>
>
> I believe this comment is somewhat misleading as there are registers that
> would return true from this function yet still hit the aforementioned
> asserts.

Really? I think it is misleading (really it will return false if
a raw access is definitely not valid, but may return true even if
a raw access is still a bad idea), but I don't think there are any
cases that would return true and then hit the assert.

>>
>> +    if (ri->type & ARM_CP_CONST) {
>> +        return true;
>> +    }
>
>
> I realize CONST registers would not likely go through the raw functions but
> these too make the above comment incorrect.

No, read_raw_cp_reg and write_raw_cp_reg both handle CONST,
so it's correct to return true here.

>> +    return (ri->raw_writefn || ri->writefn || ri->fieldoffset) &&
>> +        (ri->raw_readfn || ri->readfn || ri->fieldoffset);
>>
>> +}
>> +
>
>
> Unless we are going to use this function elsewhere, wouldn't it be better to
> just inline the code?  The name is otherwise misleading and the comment may
> cause this to be incorrectly used elsewhere in the future.  Maybe instead
> just update the call location with something like:
>
> if (!(r2->type & (ARM_CP_NO_RAW | ARM_CP_CONST)) {
>     assert((ri->raw_writefn || ri->writefn || ri->fieldoffset) &&
>            (ri->raw_readfn || ri->readfn || ri->fieldoffset);
> }

I wanted to keep the logic checking for validity next to the logic
that it matches in read/write_raw_cp_reg(). Otherwise the two are
a long way apart in the file and it's not obvious why we check what
we do. (I guess it wasn't obvious anyway, so the comment needs work.)

-- PMM