|
From: | Greg Bellows |
Subject: | Re: [Qemu-devel] [PATCH 2/2] target-arm: Add checks that cpreg raw accesses are handled |
Date: | Wed, 10 Dec 2014 17:18:30 -0600 |
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
[Prev in Thread] | Current Thread | [Next in Thread] |