Re: [PATCH v2 3/5] target/arm: add descriptions of CLIDR_EL1, CCSIDR_EL1, CTR_EL0 to cpu.h

[Leif Lindholm]

On Tue, Dec 15, 2020 at 13:23:58 +0100, Laurent Desnogues wrote:
> Hello,
> 
> On Tue, Dec 15, 2020 at 12:51 PM Leif Lindholm <leif@nuviainc.com> wrote:
> >
> > Signed-off-by: Leif Lindholm <leif@nuviainc.com>
> > ---
> >  target/arm/cpu.h | 24 ++++++++++++++++++++++++
> >  1 file changed, 24 insertions(+)
> >
> > diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> > index fadd1a47df..90ba707b64 100644
> > --- a/target/arm/cpu.h
> > +++ b/target/arm/cpu.h
> > @@ -1736,6 +1736,30 @@ FIELD(V7M_FPCCR, ASPEN, 31, 1)
> >  /*
> >   * System register ID fields.
> >   */
> > +FIELD(CLIDR_EL1, CTYPE1, 0, 3)
> > +FIELD(CLIDR_EL1, CTYPE2, 3, 3)
> > +FIELD(CLIDR_EL1, CTYPE3, 6, 3)
> > +FIELD(CLIDR_EL1, CTYPE4, 9, 3)
> > +FIELD(CLIDR_EL1, CTYPE5, 12, 3)
> > +FIELD(CLIDR_EL1, CTYPE6, 15, 3)
> > +FIELD(CLIDR_EL1, CTYPE7, 18, 3)
> > +FIELD(CLIDR_EL1, LOUIS, 21, 3)
> > +FIELD(CLIDR_EL1, LOC, 24, 3)
> > +FIELD(CLIDR_EL1, LOUU, 27, 3)
> > +FIELD(CLIDR_EL1, ICB, 30, 3)
> > +
> > +FIELD(CCSIDR_EL1, LINESIZE, 0, 3)
> > +FIELD(CCSIDR_EL1, ASSOCIATIVITY, 3, 21)
> > +FIELD(CCSIDR_EL1, NUMSETS, 32, 24)
> 
> The positions and sizes of the ASSOCIATIVITY and NUMSETS CCSIDR fields
> depend on whether the ARMv8.3-CCIDX extension is implemented or not.
> If we really want to define the fields this way, we perhaps should
> define two sets.  Or at the very least, add a comment stating this
> definition is for ARMv8.3-CCIDX.

Urgh, sorry for this.
I added the fields only to make the CPU definition more readable, so I
think we don't need to worry about runtime handling of this?
But I don't think it makes sense to add only the one form.
Should I use CCIDX_CCSIDR_EL1 for these ones and add

/* When FEAT_CCIDX is not implemented */
FIELD(CCSIDR_EL1, LINESIZE, 0, 3)
FIELD(CCSIDR_EL1, ASSOCIATIVITY, 3, 10)
FIELD(CCSIDR_EL1, NUMSETS, 13, 15)

with a comment that
/* When FEAT_CCIDX is implemented */
for the former set
?

> > +FIELD(CTR_EL0,  IMINLINE, 0, 4)
> > +FIELD(CTR_EL0,  L1IP, 14, 2)
> > +FIELD(CTR_EL0,  DMINLINE, 16, 4)
> > +FIELD(CTR_EL0,  ERG, 20, 4)
> > +FIELD(CTR_EL0,  CWG, 24, 4)
> > +FIELD(CTR_EL0,  IDC, 28, 1)
> > +FIELD(CTR_EL0,  DIC, 29, 1)
> 
> There's a missing field:  TminLine which starts at bit 32.

Ack, oops.

> If
> implemented, that would require to make ctr a 64-bit integer.

As far as I can tell, this will be safe with existing code - should I
fold in a patch extending the register?

Regards,

Leif

> Thanks,
> 
> Laurent
> 
> > +
> >  FIELD(MIDR_EL1, REVISION, 0, 4)
> >  FIELD(MIDR_EL1, PARTNUM, 4, 12)
> >  FIELD(MIDR_EL1, ARCHITECTURE, 16, 4)
> > --
> > 2.20.1
> >
> >