Re: [Qemu-devel] [PATCH 2/7] target-arm: Clean up handling of AArch64 PSTATE

[Christoffer Dall]

On Tue, Dec 17, 2013 at 12:15:54AM +0000, Peter Maydell wrote:
> On 16 December 2013 23:39, Christoffer Dall <address@hidden> wrote:
> > On Thu, Nov 28, 2013 at 01:33:17PM +0000, Peter Maydell wrote
> >> --- a/target-arm/cpu.h
> >> +++ b/target-arm/cpu.h
> >> @@ -113,8 +113,13 @@ typedef struct CPUARMState {
> >>      /* Regs for A64 mode.  */
> >>      uint64_t xregs[32];
> >>      uint64_t pc;
> >> -    /* TODO: pstate doesn't correspond to an architectural register;
> >> -     * it would be better modelled as the underlying fields.
> >> +    /* PSTATE isn't an architectural register for ARMv8. However, it is
> >> +     * convenient for us to assemble the underlying state into a 32 bit 
> >> format
> >> +     * identical to the architectural format used for the SPSR. (This is 
> >> also
> >> +     * what the Linux kernel's 'pstate' field in signal handlers and KVM's
> >> +     * 'pstate' register are.) NZCV are kept in their split fields; 
> >> aarch64
> >> +     * is an inverted split version of PSTATE.nRW (aka M[4]); other bits 
> >> are
> >> +     * stored here when in AArch64.
> >
> > I really don't understand what you are trying to say beginning with
> > aarch64 is an inverted split version...
> 
> When PSTATE.nRW == 1, aarch64 == 0; when PSTATE.nRW == 0,
> aarch64 == 1. That is, aarch64 is the nRW bit inverted. Some descriptions
> of the SPSR format call the nRW bit the 4th bit of the mode field, M[4].
> 

ah, got it, thanks.

> > Which other bits are stored
> > where in AArch64?
> 
> All the bits not listed specifically in the first half of the sentence, ie
> everything except nzcv and nRW, are stored here, ie in the
> "uint32_t pstate" field this comment is documenting.

ok, it makes sense with the above.

I think this could be written slightly more clearly for the uninitiated,
but maybe I'm just not qemu-savy enough.

> 
> > Also what is the rationale behind keeping NZCV in their split fields?
> 
> TCG generated code is faster: there are some neat sequences for
> generating the correct flags results from arithmetic and logic ops
> which you can use (and which are the rationale for the rather odd
> definitions of the cpu_NF/ZF/CF/VF). aarch64 we keep separate
> partly for historical reasons and partly because there's a whole
> load of places in the C code that want to test it.
> 

ok, I sort of figured when I read the pstate_read/write functions, but
thanks for the clarification.

> >>       */
> >>      uint32_t pstate;
> >>      uint32_t aarch64; /* 1 if CPU is in aarch64 state; inverse of 
> >> PSTATE.nRW */
> >> +/* Return the current PSTATE value. For the moment we don't support 
> >> 32<->64 bit
> >> + * interprocessing, so we don't attempt to sync with the cpsr state used 
> >> by
> >> + * the 32 bit decoder.
> >> + */
> >> +static inline uint32_t pstate_read(CPUARMState *env)
> >> +{
> >> +    int ZF;
> >> +
> >> +    ZF = (env->ZF == 0);
> >
> > So the comment on the ZF field means "if th ZF field is zero, then the
> > pstate.Z field is set, meaning the result of an op was zero".  Crystal
> > clear.
> 
> Yes. If you're generating TCG ops this means you can calculate
> the correct value of cpu_ZF by just copying the result into cpu_ZF
> if it's a 32 bit op. 64 bit code is not quite as nice but it's not
> terrible.
> 
ok, I think the comment on the ZF field cna be misread in a number of
ways, but it has been around forever, so it was good enough for others I
guess.

Thanks for explaining.
-Christoffer