Re: [Qemu-ppc] [RFC PATCH 12/17] PowerPC: booke64: Add DO_KVM kernel hooks

[Benjamin Herrenschmidt]

On Wed, 2012-07-04 at 16:29 +0200, Alexander Graf wrote:
 
> > +#ifdef CONFIG_KVM_BOOKE_HV
> > +#define KVM_BOOKE_HV_MFSPR(reg, spr)                               \
> > +   BEGIN_FTR_SECTION                                       \
> > +           mfspr   reg, spr;                               \
> > +   END_FTR_SECTION_IFSET(CPU_FTR_EMB_HV)
> > +#else
> > +#define KVM_BOOKE_HV_MFSPR(reg, spr)
> > +#endif
> 
> Bleks - this is ugly. Do we really need to open-code the #ifdef here?
> Can't the feature section code determine that the feature is disabled
> and just always not include the code?

You can't but in any case I don't see the point of the conditional here,
we'll eventually have to load srr1 no ? We can move the load up to here
in all cases or can't we ? If really not, we could have it inside DO_KVM
and be done with it no ?

> > +
> > /* Exception prolog code for all exceptions */
> > -#define EXCEPTION_PROLOG(n, type, srr0, srr1, addition)                    
> >     \
> > +#define EXCEPTION_PROLOG(n, intnum, type, srr0, srr1, addition)            
> >     \
> >     mtspr   SPRN_SPRG_##type##_SCRATCH,r13; /* get spare registers */   \
> >     mfspr   r13,SPRN_SPRG_PACA;     /* get PACA */                      \
> >     std     r10,PACA_EX##type+EX_R10(r13);                              \
> >     std     r11,PACA_EX##type+EX_R11(r13);                              \
> >     mfcr    r10;                    /* save CR */                       \
> > +   KVM_BOOKE_HV_MFSPR(r11,srr1);                                       \
> > +   DO_KVM  intnum,srr1;                                                \
> 
> So if DO_KVM already knows srr1, why explicitly do something with it
> the line above, and not in DO_KVM itself?

Yeah that or just move things around in the prolog.

> >     addition;                       /* additional code for that exc. */ \
> >     std     r1,PACA_EX##type+EX_R1(r13); /* save old r1 in the PACA */  \
> >     stw     r10,PACA_EX##type+EX_CR(r13); /* save old CR in the PACA */ \
> > @@ -69,17 +82,21 @@
> >     ld      r1,PACA_MC_STACK(r13);                                      \
> >     subi    r1,r1,SPECIAL_EXC_FRAME_SIZE;
> > 
> > -#define NORMAL_EXCEPTION_PROLOG(n, addition)                               
> >     \
> > -   EXCEPTION_PROLOG(n, GEN, SPRN_SRR0, SPRN_SRR1, addition##_GEN(n))
> > +#define NORMAL_EXCEPTION_PROLOG(n, intnum, addition)                       
> >     \
> > +   EXCEPTION_PROLOG(n, intnum, GEN, SPRN_SRR0, SPRN_SRR1,              \
> 
> We would we want to pass in 2 numbers? Let's please confine this onto
> a single ID per interrupt vector. Either we use the hardcoded ones
> available here in the KVM code or we use the KVM ones instead of the
> hardcoded ones here. But not both please. Just because it's like that
> on 32bit doesn't count as an excuse :).

Right. Also I already objected to the explicit passing of the srr's
anyway.

Cheers,
Ben.