[PATCH] powerpc/pseries: Add POWER8NVL support to ibm, client-architecture-support call

[PATCH] powerpc/pseries: Add POWER8NVL support to ibm, client-architecture-support call

[Thomas Huth]

If we do not provide the PVR for POWER8NVL, a guest on this
system currently ends up in PowerISA 2.06 compatibility mode on
KVM, since QEMU does not provide a generic PowerISA 2.07 mode yet.
So some new instructions from POWER8 (like "mtvsrd") get disabled
for the guest, resulting in crashes when using code compiled
explicitly for POWER8 (e.g. with the "-mcpu=power8" option of GCC).

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 arch/powerpc/kernel/prom_init.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/powerpc/kernel/prom_init.c b/arch/powerpc/kernel/prom_init.c
index da51925..ccd2037 100644
--- a/arch/powerpc/kernel/prom_init.c
+++ b/arch/powerpc/kernel/prom_init.c
@@ -656,6 +656,7 @@ unsigned char ibm_architecture_vec[] = {
 	W(0xffff0000), W(0x003e0000),	/* POWER6 */
 	W(0xffff0000), W(0x003f0000),	/* POWER7 */
 	W(0xffff0000), W(0x004b0000),	/* POWER8E */
+	W(0xffff0000), W(0x004c0000),   /* POWER8NVL */
 	W(0xffff0000), W(0x004d0000),	/* POWER8 */
 	W(0xffffffff), W(0x0f000004),	/* all 2.07-compliant */
 	W(0xffffffff), W(0x0f000003),	/* all 2.06-compliant */
-- 
1.8.3.1

Re: [PATCH] powerpc/pseries: Add POWER8NVL support to ibm,client-architecture-support call

[Michael Ellerman]

On Tue, 2016-05-31 at 07:51 +0200, Thomas Huth wrote:

> If we do not provide the PVR for POWER8NVL, a guest on this
> system currently ends up in PowerISA 2.06 compatibility mode on
> KVM, since QEMU does not provide a generic PowerISA 2.07 mode yet.
> So some new instructions from POWER8 (like "mtvsrd") get disabled
> for the guest, resulting in crashes when using code compiled
> explicitly for POWER8 (e.g. with the "-mcpu=power8" option of GCC).
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>

So this should say:

  Fixes: ddee09c099c3 ("powerpc: Add PVR for POWER8NVL processor")

And therefore:

  Cc: stable@vger.kernel.org # v4.0+


Am I right?

cheers


> diff --git a/arch/powerpc/kernel/prom_init.c b/arch/powerpc/kernel/prom_init.c
> index da51925..ccd2037 100644
> --- a/arch/powerpc/kernel/prom_init.c
> +++ b/arch/powerpc/kernel/prom_init.c
> @@ -656,6 +656,7 @@ unsigned char ibm_architecture_vec[] = {
>  	W(0xffff0000), W(0x003e0000),	/* POWER6 */
>  	W(0xffff0000), W(0x003f0000),	/* POWER7 */
>  	W(0xffff0000), W(0x004b0000),	/* POWER8E */
> +	W(0xffff0000), W(0x004c0000),   /* POWER8NVL */
>  	W(0xffff0000), W(0x004d0000),	/* POWER8 */
>  	W(0xffffffff), W(0x0f000004),	/* all 2.07-compliant */
>  	W(0xffffffff), W(0x0f000003),	/* all 2.06-compliant */

Re: [PATCH] powerpc/pseries: Add POWER8NVL support to ibm,client-architecture-support call

[Thomas Huth]

On 31.05.2016 12:04, Michael Ellerman wrote:
> On Tue, 2016-05-31 at 07:51 +0200, Thomas Huth wrote:
> 
>> If we do not provide the PVR for POWER8NVL, a guest on this
>> system currently ends up in PowerISA 2.06 compatibility mode on
>> KVM, since QEMU does not provide a generic PowerISA 2.07 mode yet.
>> So some new instructions from POWER8 (like "mtvsrd") get disabled
>> for the guest, resulting in crashes when using code compiled
>> explicitly for POWER8 (e.g. with the "-mcpu=power8" option of GCC).
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
> 
> So this should say:
> 
>   Fixes: ddee09c099c3 ("powerpc: Add PVR for POWER8NVL processor")
> 
> And therefore:
> 
>   Cc: stable@vger.kernel.org # v4.0+
> 
> Am I right?

Right. (At least for virtualized systems ... for bare-metal systems,
that original patch was enough). So shall I resubmit my patch with these
two lines, or could you add them when you pick this patch up?

 Thomas

Re: [PATCH] powerpc/pseries: Add POWER8NVL support to ibm,client-architecture-support call

[Michael Ellerman]

On Tue, 2016-05-31 at 12:19 +0200, Thomas Huth wrote:
> On 31.05.2016 12:04, Michael Ellerman wrote:
> > On Tue, 2016-05-31 at 07:51 +0200, Thomas Huth wrote:
> > > If we do not provide the PVR for POWER8NVL, a guest on this
> > > system currently ends up in PowerISA 2.06 compatibility mode on
> > > KVM, since QEMU does not provide a generic PowerISA 2.07 mode yet.
> > > So some new instructions from POWER8 (like "mtvsrd") get disabled
> > > for the guest, resulting in crashes when using code compiled
> > > explicitly for POWER8 (e.g. with the "-mcpu=power8" option of GCC).
> > > 
> > > Signed-off-by: Thomas Huth <thuth@redhat.com>
> > 
> > So this should say:
> > 
> >   Fixes: ddee09c099c3 ("powerpc: Add PVR for POWER8NVL processor")
> > 
> > And therefore:
> > 
> >   Cc: stable@vger.kernel.org # v4.0+
> > 
> > Am I right?
> 
> Right. (At least for virtualized systems ... for bare-metal systems,
> that original patch was enough). So shall I resubmit my patch with these
> two lines, or could you add them when you pick this patch up?

Thanks, I'll add them here.

cheers

Re: powerpc/pseries: Add POWER8NVL support to ibm, client-architecture-support call

[Michael Ellerman]

On Tue, 2016-31-05 at 05:51:17 UTC, Thomas Huth wrote:
> If we do not provide the PVR for POWER8NVL, a guest on this
> system currently ends up in PowerISA 2.06 compatibility mode on
> KVM, since QEMU does not provide a generic PowerISA 2.07 mode yet.
> So some new instructions from POWER8 (like "mtvsrd") get disabled
> for the guest, resulting in crashes when using code compiled
> explicitly for POWER8 (e.g. with the "-mcpu=power8" option of GCC).
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>

Applied to powerpc fixes, thanks.

https://git.kernel.org/powerpc/c/7cc851039d643a2ee7df4d1817

cheers

Re: [PATCH] powerpc/pseries: Add POWER8NVL support to ibm,client-architecture-support call

[Balbir Singh]

On 31/05/16 20:32, Michael Ellerman wrote:
> On Tue, 2016-05-31 at 12:19 +0200, Thomas Huth wrote:
>> On 31.05.2016 12:04, Michael Ellerman wrote:
>>> On Tue, 2016-05-31 at 07:51 +0200, Thomas Huth wrote:
>>>> If we do not provide the PVR for POWER8NVL, a guest on this
>>>> system currently ends up in PowerISA 2.06 compatibility mode on
>>>> KVM, since QEMU does not provide a generic PowerISA 2.07 mode yet.
>>>> So some new instructions from POWER8 (like "mtvsrd") get disabled
>>>> for the guest, resulting in crashes when using code compiled
>>>> explicitly for POWER8 (e.g. with the "-mcpu=power8" option of GCC).
>>>>
>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>
>>> So this should say:
>>>
>>>   Fixes: ddee09c099c3 ("powerpc: Add PVR for POWER8NVL processor")
>>>
>>> And therefore:
>>>
>>>   Cc: stable@vger.kernel.org # v4.0+
>>>
>>> Am I right?
>>
>> Right. (At least for virtualized systems ... for bare-metal systems,
>> that original patch was enough). So shall I resubmit my patch with these
>> two lines, or could you add them when you pick this patch up?
> 
> Thanks, I'll add them here.

Don't we need to update IBM_ARCH_VEC_NRCORES_OFFSET as well?

Balbir Singh

Re: [PATCH] powerpc/pseries: Add POWER8NVL support to ibm,client-architecture-support call

[Michael Ellerman]

On Wed, 2016-06-08 at 11:14 +1000, Balbir Singh wrote:
> On 31/05/16 20:32, Michael Ellerman wrote:
> > On Tue, 2016-05-31 at 12:19 +0200, Thomas Huth wrote:
> > > On 31.05.2016 12:04, Michael Ellerman wrote:
> > > > On Tue, 2016-05-31 at 07:51 +0200, Thomas Huth wrote:
> > > > > If we do not provide the PVR for POWER8NVL, a guest on this
> > > > > system currently ends up in PowerISA 2.06 compatibility mode on
> > > > > KVM, since QEMU does not provide a generic PowerISA 2.07 mode yet.
> > > > > So some new instructions from POWER8 (like "mtvsrd") get disabled
> > > > > for the guest, resulting in crashes when using code compiled
> > > > > explicitly for POWER8 (e.g. with the "-mcpu=power8" option of GCC).
> > > > > 
> > > > > Signed-off-by: Thomas Huth <thuth@redhat.com>
> > > > 
> > > > So this should say:
> > > > 
> > > >   Fixes: ddee09c099c3 ("powerpc: Add PVR for POWER8NVL processor")
> > > > 
> > > > And therefore:
> > > > 
> > > >   Cc: stable@vger.kernel.org # v4.0+
> > > > 
> > > > Am I right?
> > > 
> > > Right. (At least for virtualized systems ... for bare-metal systems,
> > > that original patch was enough). So shall I resubmit my patch with these
> > > two lines, or could you add them when you pick this patch up?
> > 
> > Thanks, I'll add them here.
> 
> Don't we need to update IBM_ARCH_VEC_NRCORES_OFFSET as well?

Yep, patch sent this morning.

cheers

Re: [PATCH] powerpc/pseries: Add POWER8NVL support to ibm,client-architecture-support call

[Thomas Huth]

On 08.06.2016 03:14, Balbir Singh wrote:
> 
> On 31/05/16 20:32, Michael Ellerman wrote:
>> On Tue, 2016-05-31 at 12:19 +0200, Thomas Huth wrote:
>>> On 31.05.2016 12:04, Michael Ellerman wrote:
>>>> On Tue, 2016-05-31 at 07:51 +0200, Thomas Huth wrote:
>>>>> If we do not provide the PVR for POWER8NVL, a guest on this
>>>>> system currently ends up in PowerISA 2.06 compatibility mode on
>>>>> KVM, since QEMU does not provide a generic PowerISA 2.07 mode yet.
>>>>> So some new instructions from POWER8 (like "mtvsrd") get disabled
>>>>> for the guest, resulting in crashes when using code compiled
>>>>> explicitly for POWER8 (e.g. with the "-mcpu=power8" option of GCC).
>>>>>
>>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>>
>>>> So this should say:
>>>>
>>>>   Fixes: ddee09c099c3 ("powerpc: Add PVR for POWER8NVL processor")
>>>>
>>>> And therefore:
>>>>
>>>>   Cc: stable@vger.kernel.org # v4.0+
>>>>
>>>> Am I right?
>>>
>>> Right. (At least for virtualized systems ... for bare-metal systems,
>>> that original patch was enough). So shall I resubmit my patch with these
>>> two lines, or could you add them when you pick this patch up?
>>
>> Thanks, I'll add them here.
> 
> Don't we need to update IBM_ARCH_VEC_NRCORES_OFFSET as well?

D'oh! You're right, that needs to be changed, too! I'll send a fixup
patch once I've tested it...

By the way, there seems to be already a check for
ibm_architecture_vec[IBM_ARCH_VEC_NRCORES_OFFSET] != NR_CPUS in
prom_send_capabilities(), but it only prints out a warning which easily
gets lost in the kernel log ... I wonder whether we should rather stop
the boot there instead to catch this problem more easily?

 Thomas

Re: [PATCH] powerpc/pseries: Add POWER8NVL support to ibm,client-architecture-support call

[Thomas Huth]

On 08.06.2016 12:44, Michael Ellerman wrote:
> On Wed, 2016-06-08 at 11:14 +1000, Balbir Singh wrote:
>> On 31/05/16 20:32, Michael Ellerman wrote:
>>> On Tue, 2016-05-31 at 12:19 +0200, Thomas Huth wrote:
>>>> On 31.05.2016 12:04, Michael Ellerman wrote:
>>>>> On Tue, 2016-05-31 at 07:51 +0200, Thomas Huth wrote:
>>>>>> If we do not provide the PVR for POWER8NVL, a guest on this
>>>>>> system currently ends up in PowerISA 2.06 compatibility mode on
>>>>>> KVM, since QEMU does not provide a generic PowerISA 2.07 mode yet.
>>>>>> So some new instructions from POWER8 (like "mtvsrd") get disabled
>>>>>> for the guest, resulting in crashes when using code compiled
>>>>>> explicitly for POWER8 (e.g. with the "-mcpu=power8" option of GCC).
>>>>>>
>>>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>>>
>>>>> So this should say:
>>>>>
>>>>>   Fixes: ddee09c099c3 ("powerpc: Add PVR for POWER8NVL processor")
>>>>>
>>>>> And therefore:
>>>>>
>>>>>   Cc: stable@vger.kernel.org # v4.0+
>>>>>
>>>>> Am I right?
>>>>
>>>> Right. (At least for virtualized systems ... for bare-metal systems,
>>>> that original patch was enough). So shall I resubmit my patch with these
>>>> two lines, or could you add them when you pick this patch up?
>>>
>>> Thanks, I'll add them here.
>>
>> Don't we need to update IBM_ARCH_VEC_NRCORES_OFFSET as well?
> 
> Yep, patch sent this morning.

Ok, looks like BenH already posted a patch ... anyway, what do you think
about aborting the boot process here in case cores != NR_CPUS, rather
than just printing out a small warning which can easily get lost in the
kernel log?

 Thomas

Re: [PATCH] powerpc/pseries: Add POWER8NVL support to ibm,client-architecture-support call

[Michael Ellerman]

On Wed, 2016-06-08 at 13:17 +0200, Thomas Huth wrote:
> On 08.06.2016 12:44, Michael Ellerman wrote:
> > On Wed, 2016-06-08 at 11:14 +1000, Balbir Singh wrote:
> > > Don't we need to update IBM_ARCH_VEC_NRCORES_OFFSET as well?
> > 
> > Yep, patch sent this morning.
> 
> Ok, looks like BenH already posted a patch ...

And me before him :)

To be clear I'm not blaming you in any way for this, the existing code is
terrible and incredibly fragile.

> anyway, what do you think about aborting the boot process here in case cores
> != NR_CPUS, rather than just printing out a small warning which can easily get
> lost in the kernel log?

Yeah I agree it's easy to miss. And it's not part of dmesg (because it's from
prom_init()), so you *only* see it if you're actually staring at the console as
it boots (which is why my boot tests missed it).

I actually have plans to rewrite the whole thing to make it robust, so that
should avoid it ever being a problem again.

cheers