On 10 December 2014 at 16:46, Peter Maydell <peter.mayd...@linaro.org>
wrote:
> On 10 December 2014 at 22:01, Greg Bellows <greg.bell...@linaro.org>
> wrote:
> >
> >
> > On 9 December 2014 at 13:46, Peter Maydell <peter.mayd...@linaro.org>
> 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
>
