Re: [Qemu-devel] [PATCH 1/2] target-arm: Split NO_MIGRATE into ALIAS and NO_RAW

[Greg Bellows]

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

On 10 December 2014 at 22:01, Greg Bellows <address@hidden> wrote:
>
>
> On 9 December 2014 at 13:46, Peter Maydell <address@hidden> wrote:
>>
>>      /* TimerValue views: a 32 bit downcounting view of the underlying
>> state */
>>      { .name = "CNTP_TVAL", .cp = 15, .crn = 14, .crm = 2, .opc1 = 0,
>> .opc2 = 0,
>> -      .type = ARM_CP_NO_MIGRATE | ARM_CP_IO, .access = PL1_RW | PL0_R,
>> +      .type = ARM_CP_NO_RAW | ARM_CP_IO, .access = PL1_RW | PL0_R,
>
>
> I realize there is no raw offset or raw*fn, but this register is marked
> NO_RAW and yet it would satisfy the later patch's raw_accessors_valid check?
> It feels like something is missing here. There are other case of this as
> well.

Not everything that passes raw_accessors_valid is actually a workable
register to do a raw access on. In particular, if (as here) there are
plain read/write accessors which have side effects then a raw access
attempt will try to use them and do something unintended.

We could add raw accessors to all of these but it would be a bit
pointless because they'll never be used. (The state is accessed via
the upcounter views, and supporting getting at it via the downcounter
views would be pretty painful.)

Sure, not suggesting adding more code, in fact this was just to note the case that affects the next patch.  Lets defer to it.

 

I'm just trying to add an easy assert() for some cases of programmer
error; others of them aren't so easily detected.

>>  static const ARMCPRegInfo vmsa_cp_reginfo[] = {
>>      { .name = "DFSR", .cp = 15, .crn = 5, .crm = 0, .opc1 = 0, .opc2 = 0,
>> -      .access = PL1_RW, .type = ARM_CP_NO_MIGRATE,
>> +      .access = PL1_RW, .type = ARM_CP_ALIAS,
>
>
> Not necessarily related to this change, but there may be a bug here.
> Clearly, the NS bank gets handled by the ESR_EL1 registration.  In the case
> of the secure bank, the expectation is that the ESR_EL3 registration takes
> care of it but it is only registered as part of the v8 reg set. In which
> case, I don't think that the secure bank will get migrated on v7 with EL3
> enabled.

Sounds plausible, but as you say not related to this change.
Want to send a patch?

Yeah, let me look closer and put one together if this is indeed an issue.

 

> It's not always the case in the code, but wouldn't it also be true that any
> register marked ARM_CP_CONST should also be ARM_CP_RAW?

Do you mean ARM_CP_NO_RAW? It's actually OK to do a raw access

Yeah typo, NO_RAW

 

to a CP_CONST register (though a little pointless) -- the case
is handled in raw_read/raw_write. I think some CONST registers
are also marked NO_RAW (previously NO_MIGRATE), so we're not
entirely consistent here. I think I was trying (when I originally
marked stuff NO_MIGRATE) to make a distinction between "ID
register which we in principle want to migrate in case the
destination has a different CPU and should fail migration"
and "dummy or wildcarded register which shouldn't be migrated".

The inconsistency is what threw me off here.  IMO, it is better to remain consistent.  If it is pointless then we should just choose one way or the other.

 

thanks
-- PMM