Re: [Qemu-ppc] [QEMU-PPC] [PATCH 2/3] target/ppc: Don't require private l1d cache on POWER8 for cap_ppc_safe_cache

[Greg Kurz]

On Mon, 14 May 2018 10:59:03 +1000
Suraj Jitindar Singh <address@hidden> wrote:

> On Fri, 2018-05-11 at 20:22 +0200, Greg Kurz wrote:
> > On Fri, 11 May 2018 12:21:11 -0300
> > Murilo Opsfelder Araujo <address@hidden> wrote:
> >   
> > > On Fri, May 11, 2018 at 04:25:08PM +1000, Suraj Jitindar Singh
> > > wrote:  
> > > > For cap_ppc_safe_cache to be set to workaround, we require both a
> > > > l1d
> > > > cache flush instruction and private l1d cache.
> > > > 
> > > > On POWER8 don't require private l1d cache. This means a guest on
> > > > a
> > > > POWER8 machine can make use of the cache flush workarounds.
> > > > 

BTW, some more background could help to understand. Why is this for
POWER8 only ? All variants of POWER8 ? What about other families ?

> > > > Signed-off-by: Suraj Jitindar Singh <address@hidden>
> > > > ---
> > > >  target/ppc/kvm.c | 10 +++++++++-
> > > >  1 file changed, 9 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> > > > index 2c0c34e125..7b33119b1a 100644
> > > > --- a/target/ppc/kvm.c
> > > > +++ b/target/ppc/kvm.c
> > > > @@ -2414,9 +2414,17 @@ bool kvmppc_has_cap_mmu_hash_v3(void)
> > > >  
> > > >  static int parse_cap_ppc_safe_cache(struct kvm_ppc_cpu_char c)
> > > >  {
> > > > +    PowerPCCPUClass *pcc = kvm_ppc_get_host_cpu_class();
> > > > +    bool l1d_thread_priv_req = true;
> > > > +
> > > > +    if (pcc->pvr_match(pcc, CPU_POWERPC_POWER8_BASE)) {
> > > > +        l1d_thread_priv_req = false;
> > > > +    }
> > > > +    
> > > 
> > > Hi, Suraj.
> > > 
> > > Why not use ppc_pvr_match_power8()?
> > >   
> > 
> > ppc_pvr_match_power8() is the implementation of
> > PowerPCCPUClass::pvr_match()
> > for the POWER8 family, hence always returns true if passed
> > CPU_POWERPC_POWER8_*
> > 
> > static bool ppc_pvr_match_power8(PowerPCCPUClass *pcc, uint32_t pvr)
> > {
> >     if ((pvr & CPU_POWERPC_POWER_SERVER_MASK) ==
> > CPU_POWERPC_POWER8NVL_BASE) {
> >         return true;
> >     }
> >     if ((pvr & CPU_POWERPC_POWER_SERVER_MASK) ==
> > CPU_POWERPC_POWER8E_BASE) {
> >         return true;
> >     }
> >     if ((pvr & CPU
> > _POWERPC_POWER_SERVER_MASK) ==
> > CPU_POWERPC_POWER8_BASE) {
> >         return true;
> >     }
> >     return false;
> > }
> > 
> > It's the other way round, we want to guess that the host CPU belongs
> > to the
> > POWER8 family because only its pvr_match() returns true IIUC.
> > 
> > Suraj,
> > 
> > So this is ok for POWER8, POWER8E and POWER8NVL ? If so, it would be
> > more appropriate to call pcc->pvr_match() for each pvr value IMHO.  
> 
> I will have the same effect.
> 

Sure, but CPU_POWERPC_POWER8_BASE only is confusing... what is it supposed
to mean ? For PVR 0x004D0000 only ? What about PVR values of older models
0x004B0000 (POWER8E) or newer models 0x004C0000 (POWER8NVL) ?

> How about
> 
> if (pcc->pvr_match(pcc, (CPU_POWERPC_POWER_SERVER_MASK &
> CPU_POWERPC_POWER8_BASE)) {
> 
> ?
> 

Not sure it's clearer.

IIUC you want to guess if the host CPU class belongs to the
POWER8 family. Why not introducing an helper in translate_init.c ?

bool ppc_cpu_family_is_power8(PowerPCCPUClass *pcc)
{
    PowerPCCPUClass *pcc_family = ppc_cpu_get_family_class(pcc);

    return pcc_family->class_init == ppc_POWER8_cpu_family_class_init;
}

and do:

if (ppc_cpu_family_is_power8(pcc)) {

or alternatively, thinking again about Murilo's suggestion, make
ppc_pvr_match_power8() extern and do:

if (ppc_pvr_match_power8(pcc, pcc->pvr)) {

> >   
> > > >      if (~c.behaviour & c.behaviour_mask &
> > > > H_CPU_BEHAV_L1D_FLUSH_PR) {
> > > >          return 2;
> > > > -    } else if ((c.character & c.character_mask &
> > > > H_CPU_CHAR_L1D_THREAD_PRIV) &&
> > > > +    } else if ((!l1d_thread_priv_req ||
> > > > +                c.character & c.character_mask &
> > > > H_CPU_CHAR_L1D_THREAD_PRIV) &&
> > > >                 (c.character & c.character_mask
> > > >                  & (H_CPU_CHAR_L1D_FLUSH_ORI30 |
> > > > H_CPU_CHAR_L1D_FLUSH_TRIG2))) {
> > > >          return 1;
> > > > -- 
> > > > 2.13.6
> > > > 
> > > >     
> > 
> >