Re: [Qemu-devel] [1/3] powerpc: scan_features() updates incorrect bits

2016-04-17 Thread Michael Ellerman
On Sat, 2016-04-16 at 00:27 +1000, Michael Ellerman wrote:
> On Fri, 2016-15-04 at 02:06:13 UTC, Unknown sender due to SPF wrote:
> > The real LE feature entry in the ibm_pa_feature struct has the
> > wrong number of elements. Instead of checking for byte 5, bit 0,
> > we check for byte 0, bit 0, and we also incorrectly update cpu user
> > feature bit 5.
> 
> Finally the user feature, we're setting 5, ie. 0x4 | 0x1, which is:
> 
> #define PPC_FEATURE_PPC_LE0x0001
> 
> And nothing else, 0x4 is free.

Bt, we should reserve 0x4 for the foreseeable future. Because it is
erroneously set on older kernels.

cheers




Re: [Qemu-devel] [1/3] powerpc: scan_features() updates incorrect bits

2016-04-17 Thread Michael Ellerman
On Fri, 2016-15-04 at 02:06:13 UTC, Unknown sender due to SPF wrote:
> The real LE feature entry in the ibm_pa_feature struct has the
> wrong number of elements. Instead of checking for byte 5, bit 0,
> we check for byte 0, bit 0, and we also incorrectly update cpu user
> feature bit 5.
> 
> diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
> index 7030b03..9a3a7c6 100644
> --- a/arch/powerpc/kernel/prom.c
> +++ b/arch/powerpc/kernel/prom.c
> @@ -158,7 +158,7 @@ static struct ibm_pa_feature {
>   {CPU_FTR_NOEXECUTE, 0, 0,   0, 6, 0},
>   {CPU_FTR_NODSISRALIGN, 0, 0,1, 1, 1},
>   {0, MMU_FTR_CI_LARGE_PAGE, 0,   1, 2, 0},
> - {CPU_FTR_REAL_LE, PPC_FEATURE_TRUE_LE, 5, 0, 0},
> + {CPU_FTR_REAL_LE, PPC_FEATURE_TRUE_LE, 0, 5, 0, 0},

This should be:

> + {CPU_FTR_REAL_LE, 0, PPC_FEATURE_TRUE_LE, 5, 0, 0},

Because the struct layout is:

static struct ibm_pa_feature {
unsigned long   cpu_features;   /* CPU_FTR_xxx bit */
unsigned long   mmu_features;   /* MMU_FTR_xxx bit */
unsigned intcpu_user_ftrs;  /* PPC_FEATURE_xxx bit */
unsigned intcpu_user_ftrs2; /* PPC_FEATURE2_xxx bit */
unsigned char   pabyte; /* byte number in ibm,pa-features */
unsigned char   pabit;  /* bit number (big-endian) */
unsigned char   invert; /* if 1, pa bit set => clear feature */
}


I'll fix it up locally.

cheers



Re: [Qemu-devel] [1/3] powerpc: scan_features() updates incorrect bits

2016-04-15 Thread Michael Ellerman
On Fri, 2016-15-04 at 02:06:13 UTC, Unknown sender due to SPF wrote:
> The real LE feature entry in the ibm_pa_feature struct has the
> wrong number of elements. Instead of checking for byte 5, bit 0,
> we check for byte 0, bit 0, and we also incorrectly update cpu user
> feature bit 5.
>
> Fixes: 44ae3ab3358e ("powerpc: Free up some CPU feature bits by moving out 
> MMU-related features")

So pulling that apart a bit.

> - {CPU_FTR_REAL_LE, PPC_FEATURE_TRUE_LE, 5, 0,   0},   (invert 0)
 ^^^  ^^
 cpu feat mmu feat user feat  byte bit

By checking byte 0 bit 0 (IBM numbering), means we're looking at the "Memory
Management Unit (MMU)" feature - ie. does the CPU have an MMU.

As you'd expect that bit is set on most platforms we care about. I see it set on
a P6 & P7 here running PowerVM, as well as by recent Qemu on P8.

Which means on all those systems we'll be enabling those bits.

As far as the cpu feature bits:

  #define CPU_FTR_REAL_LE   0x0040

On a guest running on P8 on older qemu which has no ibm,pa-features property I
see:

  cpu_features  = 0x17fc7aec18500249
  ^

So it's set, but not by this logic, because it's already in cpu_features for P8.

On Power6, which does have the ibm,pa-feature property with bit 0,0 set:

  cpu_features  = 0x090d1ae018500049
  ^

Also set, from the property, but it's also in the Power6 mask by default. So all
we've lost there is the ability to turn it off.


For the MMU feature, we're setting:

  #define PPC_FEATURE_TRUE_LE   0x0002

Which matches:

  #define MMU_FTR_TYPE_8xx  0x0002

And on the Power6 I do indeed see:

  mmu_features  = 0x7c03

Which says I have MMU_FTR_HPTE_TABLE and MMU_FTR_TYPE_8xx.

Luckily it looks like the only place that looks at MMU_FTR_TYPE_8xx is in Book3E
code, so will never be affected by this bug.


Finally the user feature, we're setting 5, ie. 0x4 | 0x1, which is:

#define PPC_FEATURE_PPC_LE  0x0001

And nothing else, 0x4 is free.

On the Power6, I can see it in /proc/pid/auxv:

0060  00 00 00 00 00 00 00 10  00 00 00 00 dc 00 74 47  |..tG|
 ^
 0x4 | 0x2 | 0x1

LD_SHOW_AUXV=1 doesn't show it, but that's just because my glibc is old.


Net result:
 - on P6 & P7 (& P8 with newer Qemu) we can't disable CPU_FTR_REAL_LE - but
   luckily we've never wanted to.
 - we're stuffing up mmu_features but it doesn't matter
 - we're advertising PPC_LE when we shouldn't be, but *probably* nothing cares.
 - we're advertising 0x4 in HWCAP which is undefined, but *almost certainly*
   nothing cares.

So that could have been a doozy but turns out not actually that bad. Phew :)

cheers


> --- a/arch/powerpc/kernel/prom.c
> +++ b/arch/powerpc/kernel/prom.c
> @@ -158,7 +158,7 @@ static struct ibm_pa_feature {
>   {CPU_FTR_NOEXECUTE, 0, 0,   0, 6, 0},
>   {CPU_FTR_NODSISRALIGN, 0, 0,1, 1, 1},
>   {0, MMU_FTR_CI_LARGE_PAGE, 0,   1, 2, 0},
> - {CPU_FTR_REAL_LE, PPC_FEATURE_TRUE_LE, 5, 0, 0},
> + {CPU_FTR_REAL_LE, PPC_FEATURE_TRUE_LE, 0, 5, 0, 0},
>   /*
>* If the kernel doesn't support TM (ie. 
> CONFIG_PPC_TRANSACTIONAL_MEM=n),
>* we don't want to turn on CPU_FTR_TM here, so we use CPU_FTR_TM_COMP