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

[Greg Bellows]

On 10 December 2014 at 16:50, Peter Maydell <address@hidden> wrote:

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 you called the routine on PMCCNTR, for instance, this routine would return true and if you then called raw_read or raw_write you would hit the assert, correct?  This may be contrived, but I believe there are cases that the comment is incorrect.

 

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

I can go either way and understand the thought behind keeping the check near the code. Given that the function is tied to those functions it may just need to be more clear.  Maybe even call out which asserts you are concerned about.

 

-- PMM