Re: [Qemu-ppc] [RFC 10/17] pseries: Rewrite CAS PVR compatibility logic

[David Gibson]

On Thu, Nov 10, 2016 at 11:54:13AM -0600, Michael Roth wrote:
> Quoting David Gibson (2016-10-30 06:12:01)
> > During boot, PAPR guests negotiate CPU model support with the
> > ibm,client-architecture-support mechanism.  The logic to implement this in
> > qemu is very convoluted.  This cleans it up to be cleaner, using the new
> > ppc_check_compat() call.
> > 
> > The new logic for choosing a compatibility mode is:
> >     1. If the guest lists the CPU's real PVR as supported *AND* no
> >        maximum compatibility mode has been requested on the command line
> >        then we use "raw" mode - the CPU acts with full capabilities.
> >     2. Otherwise, we pick the most recent compatibility mode which is
> >        both supported by the CPU, and is advertised as supported by the
> >        guest.
> > I think the original code approximated the same thing, but it's hard to be
> > sure, and I think it had some weird edge cases.
> > 
> > Signed-off-by: David Gibson <address@hidden>
> > ---
> >  hw/ppc/spapr_hcall.c | 107 
> > +++++++++++++++++----------------------------------
> >  hw/ppc/trace-events  |   2 +-
> >  2 files changed, 37 insertions(+), 72 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> > index d93f580..3bd6d06 100644
> > --- a/hw/ppc/spapr_hcall.c
> > +++ b/hw/ppc/spapr_hcall.c
> > @@ -895,98 +895,63 @@ static void do_set_compat(CPUState *cs, void *arg)
> >      ppc_set_compat(cpu, s->compat_pvr, &s->err);
> >  }
> > 
> > -#define get_compat_level(cpuver) ( \
> > -    ((cpuver) == CPU_POWERPC_LOGICAL_2_05) ? 2050 : \
> > -    ((cpuver) == CPU_POWERPC_LOGICAL_2_06) ? 2060 : \
> > -    ((cpuver) == CPU_POWERPC_LOGICAL_2_06_PLUS) ? 2061 : \
> > -    ((cpuver) == CPU_POWERPC_LOGICAL_2_07) ? 2070 : 0)
> > -
> > -static void cas_handle_compat_cpu(PowerPCCPUClass *pcc, uint32_t pvr,
> > -                                  unsigned max_lvl, unsigned *compat_lvl,
> > -                                  unsigned *compat_pvr)
> > -{
> > -    unsigned lvl = get_compat_level(pvr);
> > -    bool is205, is206, is207;
> > -
> > -    if (!lvl) {
> > -        return;
> > -    }
> > -
> > -    /* If it is a logical PVR, try to determine the highest level */
> > -    is205 = (pcc->pcr_supported & PCR_COMPAT_2_05) &&
> > -            (lvl == get_compat_level(CPU_POWERPC_LOGICAL_2_05));
> > -    is206 = (pcc->pcr_supported & PCR_COMPAT_2_06) &&
> > -            ((lvl == get_compat_level(CPU_POWERPC_LOGICAL_2_06)) ||
> > -             (lvl == get_compat_level(CPU_POWERPC_LOGICAL_2_06_PLUS)));
> > -    is207 = (pcc->pcr_supported & PCR_COMPAT_2_07) &&
> > -            (lvl == get_compat_level(CPU_POWERPC_LOGICAL_2_07));
> > -
> > -    if (is205 || is206 || is207) {
> > -        if (!max_lvl) {
> > -            /* User did not set the level, choose the highest */
> > -            if (*compat_lvl <= lvl) {
> > -                *compat_lvl = lvl;
> > -                *compat_pvr = pvr;
> > -            }
> > -        } else if (max_lvl >= lvl) {
> > -            /* User chose the level, don't set higher than this */
> > -            *compat_lvl = lvl;
> > -            *compat_pvr = pvr;
> > -        }
> > -    }
> > -}
> > -
> > -static target_ulong h_client_architecture_support(PowerPCCPU *cpu_,
> > +static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
> >                                                    sPAPRMachineState *spapr,
> >                                                    target_ulong opcode,
> >                                                    target_ulong *args)
> >  {
> >      target_ulong list = ppc64_phys_to_real(args[0]);
> >      target_ulong ov_table;
> > -    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu_);
> >      CPUState *cs;
> > -    bool cpu_match = false;
> > -    unsigned old_compat_pvr = cpu_->compat_pvr;
> > -    unsigned compat_lvl = 0, compat_pvr = 0;
> > -    unsigned max_lvl = get_compat_level(cpu_->max_compat);
> > -    int counter;
> > +    bool explicit_match = false; /* Matched the CPU's real PVR */
> > +    uint32_t max_compat = cpu->max_compat;
> > +    uint32_t best_compat = 0;
> > +    int i;
> >      sPAPROptionVector *ov5_guest, *ov5_cas_old, *ov5_updates;
> > 
> > -    /* Parse PVR list */
> > -    for (counter = 0; counter < 512; ++counter) {
> > +    /*
> > +     * We scan the supplied table of PVRs looking for two things
> > +     *   1. Is our real CPU PVR in the list?
> > +     *   2. What's the "best" listed logical PVR
> > +     */
> > +    for (i = 0; i < 512; ++i) {
> >          uint32_t pvr, pvr_mask;
> > 
> > -        pvr_mask = ldl_be_phys(&address_space_memory, list);
> > -        list += 4;
> >          pvr = ldl_be_phys(&address_space_memory, list);
> > -        list += 4;
> > -
> > -        trace_spapr_cas_pvr_try(pvr);
> > -        if (!max_lvl &&
> > -            ((cpu_->env.spr[SPR_PVR] & pvr_mask) == (pvr & pvr_mask))) {
> > -            cpu_match = true;
> > -            compat_pvr = 0;
> > -        } else if (pvr == cpu_->compat_pvr) {
> > -            cpu_match = true;
> > -            compat_pvr = cpu_->compat_pvr;
> > -        } else if (!cpu_match) {
> > -            cas_handle_compat_cpu(pcc, pvr, max_lvl, &compat_lvl, 
> > &compat_pvr);
> > -        }
> > -        /* Terminator record */
> > +        pvr_mask = ldl_be_phys(&address_space_memory, list + 4);
> > +        list += 8;
> > +
> >          if (~pvr_mask & pvr) {
> > -            break;
> > +            break; /* Terminator record */
> >          }
> > +
> > +        if ((cpu->env.spr[SPR_PVR] & pvr_mask) == (pvr & pvr_mask)) {
> > +            explicit_match = true;
> 
> The previous code only did this check if max_lvl/max_compat was set. I'm
> not sure if it's possible in practice, but in theory this PVR might also
> happen to be the next best_compat PVR, but since we test positive here
> we may not not set best_compat accordingly.

I guess it's theoretically possible.  But that would only happen if
the guest advertised a pvr+mask which covered both a valid real PVR
and a valid compatibility PVR.  Since the logical PVRs are in a wholly
different range and don't look like real PVR values at all, I think
that's vanishingly unlikely.

> 
> > +        } else {
> > +            if (ppc_check_compat(cpu, pvr, best_compat, max_compat)) {
> > +                best_compat = pvr;
> > +            }
> > +        }
> > +    }
> > +
> > +    if (!max_compat && explicit_match) {
> > +        /* If the guest explicitly supports the CPU, *and* user hasn't
> > +         * requested a compatibility mode, use "raw" mode */
> > +        best_compat = 0;
> > +    } else if (best_compat == 0) {
> > +        /* Didn't find any supported compat modes */
> > +        /* FIXME: what's the right error here? */
> > +        return H_HARDWARE;
> >      }
> > 
> >      /* Parsing finished */
> > -    trace_spapr_cas_pvr(cpu_->compat_pvr, cpu_match,
> > -                        compat_pvr, pcc->pcr_mask);
> > +    trace_spapr_cas_pvr(cpu->compat_pvr, explicit_match, best_compat);
> > 
> >      /* Update CPUs */
> > -    if (old_compat_pvr != compat_pvr) {
> > +    if (cpu->compat_pvr != best_compat) {
> >          CPU_FOREACH(cs) {
> >              SetCompatState s = {
> > -                .compat_pvr = compat_pvr,
> > +                .compat_pvr = best_compat,
> >                  .err = NULL,
> >              };
> > 
> > diff --git a/hw/ppc/trace-events b/hw/ppc/trace-events
> > index 2297ead..604ac92 100644
> > --- a/hw/ppc/trace-events
> > +++ b/hw/ppc/trace-events
> > @@ -15,7 +15,7 @@ spapr_cas_continue(unsigned long n) "Copy changes to the 
> > guest: %ld bytes"
> > 
> >  # hw/ppc/spapr_hcall.c
> >  spapr_cas_pvr_try(uint32_t pvr) "%x"
> > -spapr_cas_pvr(uint32_t cur_pvr, bool cpu_match, uint32_t new_pvr, uint64_t 
> > pcr) "current=%x, cpu_match=%u, new=%x, compat flags=%"PRIx64
> > +spapr_cas_pvr(uint32_t cur_pvr, bool explicit_match, uint32_t new_pvr) 
> > "current=%x, explicit_match=%u, new=%x"
> > 
> >  # hw/ppc/spapr_iommu.c
> >  spapr_iommu_put(uint64_t liobn, uint64_t ioba, uint64_t tce, uint64_t ret) 
> > "liobn=%"PRIx64" ioba=0x%"PRIx64" tce=0x%"PRIx64" ret=%"PRId64
> 

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature