On Fri, Nov 04, 2016 at 02:45:02PM +1100, Alexey Kardashevskiy wrote:
> On 31/10/16 19:39, David Gibson wrote:
> > 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 <da...@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
>
> In last 5+ years, I have never seen pointer compared anyhow but using "=="
> and "!=". A bit unusual.
Unusual, yes, but it has its uses from time to time.
>
>
> Reviewed-by: Alexey Kardashevskiy <a...@ozlabs.ru>
>
>
>
>
>
> >>> + */
> >>> { /* 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
signature.asc
Description: PGP signature
