Re: [Qemu-devel] [RFC 09/17] ppc: Validate compatibility modes when setting

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 3895 bytes --]

On Mon, Oct 31, 2016 at 04:55:42PM +1100, Alexey Kardashevskiy wrote:
> On 30/10/16 22:12, David Gibson wrote:
> > Current ppc_set_compat() will attempt to set any compatiblity mode
> > specified, regardless of whether it's available on the CPU.  The caller is
> > expected to make sure it is setting a possible mode, which is awkwward
> > because most of the information to make that decision is at the CPU level.
> > 
> > This begins to clean this up by introducing a ppc_check_compat() function
> > which will determine if a given compatiblity mode is supported on a CPU
> > (and also whether it lies within specified minimum and maximum compat
> > levels, which will be useful later).  It also contains an assertion that
> > the CPU has a "virtual hypervisor"[1], that is, that the guest isn't
> > permitted to execute hypervisor privilege code.  Without that, the guest
> > would own the PCR and so could override any mode set here.  Only machine
> > types which use a virtual hypervisor (i.e. 'pseries') should use
> > ppc_check_compat().
> > 
> > ppc_set_compat() is modified to validate the compatibility mode it is given
> > and fail if it's not available on this CPU.
> > 
> > [1] Or user-only mode, which also obviously doesn't allow access to the
> > hypervisor privileged PCR.  We don't use that now, but could in future.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  target-ppc/compat.c | 41 +++++++++++++++++++++++++++++++++++++++++
> >  target-ppc/cpu.h    |  2 ++
> >  2 files changed, 43 insertions(+)
> > 
> > diff --git a/target-ppc/compat.c b/target-ppc/compat.c
> > index 66529a6..1059555 100644
> > --- a/target-ppc/compat.c
> > +++ b/target-ppc/compat.c
> > @@ -28,29 +28,37 @@
> >  typedef struct {
> >      uint32_t pvr;
> >      uint64_t pcr;
> > +    uint64_t pcr_level;
> >      int max_threads;
> >  } CompatInfo;
> >  
> >  static const CompatInfo compat_table[] = {
> > +    /*
> > +     * Ordered from oldest to newest - the code relies on this
> > +     */
> >      { /* POWER6, ISA2.05 */
> >          .pvr = CPU_POWERPC_LOGICAL_2_05,
> >          .pcr = PCR_COMPAT_2_07 | PCR_COMPAT_2_06 | PCR_COMPAT_2_05
> >                 | PCR_TM_DIS | PCR_VSX_DIS,
> > +        .pcr_level = PCR_COMPAT_2_05,
> >          .max_threads = 2,
> >      },
> >      { /* POWER7, ISA2.06 */
> >          .pvr = CPU_POWERPC_LOGICAL_2_06,
> >          .pcr = PCR_COMPAT_2_07 | PCR_COMPAT_2_06 | PCR_TM_DIS,
> > +        .pcr_level = PCR_COMPAT_2_06,
> >          .max_threads = 4,
> >      },
> >      {
> >          .pvr = CPU_POWERPC_LOGICAL_2_06_PLUS,
> >          .pcr = PCR_COMPAT_2_07 | PCR_COMPAT_2_06 | PCR_TM_DIS,
> > +        .pcr_level = PCR_COMPAT_2_06,
> >          .max_threads = 4,
> >      },
> >      { /* POWER8, ISA2.07 */
> >          .pvr = CPU_POWERPC_LOGICAL_2_07,
> >          .pcr = PCR_COMPAT_2_07,
> > +        .pcr_level = PCR_COMPAT_2_07,
> >          .max_threads = 8,
> >      },
> >  };
> > @@ -67,6 +75,35 @@ static const CompatInfo *compat_by_pvr(uint32_t pvr)
> >      return NULL;
> >  }
> >  
> > +bool ppc_check_compat(PowerPCCPU *cpu, uint32_t compat_pvr,
> > +                      uint32_t min_compat_pvr, uint32_t max_compat_pvr)
> > +{
> > +    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
> > +    const CompatInfo *compat = compat_by_pvr(compat_pvr);
> > +    const CompatInfo *min = compat_by_pvr(min_compat_pvr);
> > +    const CompatInfo *max = compat_by_pvr(max_compat_pvr);
> 
> 
> You keep giving very generic names (as "min" and "max") to local
> variables ;)

For local variables, brevity is a virtue.

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]