Re: [Qemu-devel] [PATCH 4/7] target/openrisc: implement shadow registers

[Stafford Horne]

On Tue, Apr 18, 2017 at 01:11:53AM -0700, Richard Henderson wrote:
> On 04/16/2017 04:23 PM, Stafford Horne wrote:
> > Shadow registers are part of the openrisc spec along with sr[cid], as
> > part of the fast context switching feature.  When exceptions occur,
> > instead of having to save registers to the stack if enabled the CID will
> > increment and a new set of registers will be available.
> > 
> > This patch only implements shadow registers which can be used as extra
> > scratch registers via the mfspr and mtspr if required.  This is
> > implemented in a way where it would be easy to add on the fast context
> > switching, currently cid is hardcoded to 0.
> 
> I'm not especially keen on this half-conversion.
> If CID cannot be changed, then
> 
> > -    target_ulong gpr[32];     /* General registers */
> > +    target_ulong shadow_gpr[16][32]; /* Shadow registers */
> > +    target_ulong * gpr;       /* General registers (backed by shadow) */
> 
> this pointer should not be necessary.  Just use a union, or even just
> 
>     target_ulong gpr[32];
>     target_ulong shadow_gpr[15][32];
> 
> for now.
> 
> Alternately, add accessor functions that take the whole ENV (which would be
> able to read CID, when needed).  C.f.
> 
>     uint64_t cpu_alpha_load_gr(CPUAlphaState *env, unsigned reg);
>     void cpu_alpha_store_gr(CPUAlphaState *env, unsigned reg, uint64_t val);

Actually, I started off writing it this way, but there were a lot of places
that used env->gpr.  I had some regex's to replace everything, but in the
end I thought using the gpr pointer was just easier.

Using the union would work as well, but it wouldnt help to allow switching
of cid.  My idea with the pointer was that if cid was updates I could
update the pointer at that time.

Ill rework this to use the accessor functions.

-Stafford

> If/when CID can be changed, then we can talk about various ways
> that this can be modeled within TCG.
> 
> 
> r~