[BUG]: KVM: arm64: Fix symbol dependency in __hyp_call_panic_nvhe

[BUG]: KVM: arm64: Fix symbol dependency in __hyp_call_panic_nvhe

[H. Nikolaus Schaller]

Hi,
it seems as if your patch

34f379956e9d7 ("KVM: arm64: Fix symbol dependency in __hyp_call_panic_nvhe")
[ Upstream commit b38b298aa4397e2dc74a89b4dd3eac9e59b64c96 ]

fails to compile in v5.8.7 for me (using an aarch64 gcc 4.9 cross-toolchain to try
to build a kernel for the PinePhone):

  CC      arch/arm64/kvm/hyp/switch.o - due to target missing
arch/arm64/kvm/hyp/switch.c: In function 'hyp_panic':
arch/arm64/kvm/hyp/switch.c:904:2: error: impossible constraint in 'asm'
  asm volatile("ldr %0, =%1" : "=r" (str_va) : "S" (__hyp_panic_string));
  ^

I can find the commit b38b298aa4397e2dc74a89b4dd3eac9e59b64c96 in upstream
but not the affected file. There is also "KVM: arm64: Split hyp/switch.c to VHE/nVHE"
which does a cleanup and rename and v5.9-rc4 compiles fine.

With a git revert 34f379956e9d7 on v5.8.7 I can compile without problems.

So something seems to be incomplete or premature with the backport.

BR and thanks,
Nikolaus Schaller

Re: [BUG]: KVM: arm64: Fix symbol dependency in __hyp_call_panic_nvhe

[Sasha Levin]

On Mon, Sep 07, 2020 at 03:29:40PM +0200, H. Nikolaus Schaller wrote:
>Hi,
>it seems as if your patch
>
>34f379956e9d7 ("KVM: arm64: Fix symbol dependency in __hyp_call_panic_nvhe")
>[ Upstream commit b38b298aa4397e2dc74a89b4dd3eac9e59b64c96 ]
>
>fails to compile in v5.8.7 for me (using an aarch64 gcc 4.9 cross-toolchain to try
>to build a kernel for the PinePhone):
>
>  CC      arch/arm64/kvm/hyp/switch.o - due to target missing
>arch/arm64/kvm/hyp/switch.c: In function 'hyp_panic':
>arch/arm64/kvm/hyp/switch.c:904:2: error: impossible constraint in 'asm'
>  asm volatile("ldr %0, =%1" : "=r" (str_va) : "S" (__hyp_panic_string));
>  ^

Does upstream build correctly for you?

>I can find the commit b38b298aa4397e2dc74a89b4dd3eac9e59b64c96 in upstream
>but not the affected file. There is also "KVM: arm64: Split hyp/switch.c to VHE/nVHE"
>which does a cleanup and rename and v5.9-rc4 compiles fine.

Right, it got moved around in upstream.

-- 
Thanks,
Sasha

Re: [BUG]: KVM: arm64: Fix symbol dependency in __hyp_call_panic_nvhe

[H. Nikolaus Schaller]

Hi,

> Am 07.09.2020 um 16:22 schrieb Sasha Levin <sashal@kernel.org>:
> 
> On Mon, Sep 07, 2020 at 03:29:40PM +0200, H. Nikolaus Schaller wrote:
>> Hi,
>> it seems as if your patch
>> 
>> 34f379956e9d7 ("KVM: arm64: Fix symbol dependency in __hyp_call_panic_nvhe")
>> [ Upstream commit b38b298aa4397e2dc74a89b4dd3eac9e59b64c96 ]
>> 
>> fails to compile in v5.8.7 for me (using an aarch64 gcc 4.9 cross-toolchain to try
>> to build a kernel for the PinePhone):
>> 
>> CC      arch/arm64/kvm/hyp/switch.o - due to target missing
>> arch/arm64/kvm/hyp/switch.c: In function 'hyp_panic':
>> arch/arm64/kvm/hyp/switch.c:904:2: error: impossible constraint in 'asm'
>> asm volatile("ldr %0, =%1" : "=r" (str_va) : "S" (__hyp_panic_string));
>> ^
> 
> Does upstream build correctly for you?

It is 100% upstream code in arch/arm64, just with a private config.
diff --stat arch/arm64 shows only 2 dts and 2 config files. It did
compile well with v5.8.5 and just broke after merge v5.8.7.

> 
>> I can find the commit b38b298aa4397e2dc74a89b4dd3eac9e59b64c96 in upstream
>> but not the affected file. There is also "KVM: arm64: Split hyp/switch.c to VHE/nVHE"
>> which does a cleanup and rename and v5.9-rc4 compiles fine.
> 
> Right, it got moved around in upstream.

Maybe this has fixed something...

BR and thanks,
Nikolaus

Re: [BUG]: KVM: arm64: Fix symbol dependency in __hyp_call_panic_nvhe

[David Brazdil]

Hi Nikolaus,

> > Right, it got moved around in upstream.
> 
> Maybe this has fixed something...
> 

Thanks for reporting this. I've managed to reproduce the problem with Linaro
GCC 4.9.4 and I can also confirm that the same toolchain builds v5.9-rc5 fine.

As Sasha pointed out, the patch was part of a larger series that ended up
moving the definition of __hyp_panic_string to a different source file. That
means in 5.8.7 switch.c sees it declared as 'static const char[]' but in
5.9 it is declared as 'extern const char[]'. When changed to 'extern', 5.8.7
compiles fine so this sounds to me like a bug in GCC that has since been fixed.

That means we have two options:
(a) define __hyp_panic_string in a different .c file in all pre-5.9 branches, or
(b) revert the backported patch.

The patch was needed in 5.9 and should stay there. It wasn't needed in earlier
versions because the symbol was being kept alive by another user. It did "fix"
the inline asm semantics, but the problem was never triggered in pre-5.9.

Sasha, with this and the GCC bug in mind, would you agree that (b) is the
better course of action?

-David

Re: [BUG]: KVM: arm64: Fix symbol dependency in __hyp_call_panic_nvhe

[Oliver Upton]

> That means we have two options:
> (a) define __hyp_panic_string in a different .c file in all pre-5.9 branches, or
> (b) revert the backported patch.
> 
> The patch was needed in 5.9 and should stay there. It wasn't needed in earlier
> versions because the symbol was being kept alive by another user. It did "fix"
> the inline asm semantics, but the problem was never triggered in pre-5.9.
> 
> Sasha, with this and the GCC bug in mind, would you agree that (b) is the
> better course of action?

Sasha,

Any chance we can get this patch reverted as David has suggested? It was
backported to 5.4 LTS in commit 653ae33b030b ("KVM: arm64: Fix symbol dependency in __hyp_call_panic_nvhe")
and is causing build issues with a 4.9.4 vintage of GCC.

Thanks!

--
Oliver

Re: [BUG]: KVM: arm64: Fix symbol dependency in __hyp_call_panic_nvhe

[Marc Zyngier]

Hi all,

On Mon, 25 Jan 2021 20:07:56 +0000,
Oliver Upton <oupton@google.com> wrote:
> 
> > That means we have two options:
> > (a) define __hyp_panic_string in a different .c file in all pre-5.9 branches, or
> > (b) revert the backported patch.
> > 
> > The patch was needed in 5.9 and should stay there. It wasn't needed in earlier
> > versions because the symbol was being kept alive by another user. It did "fix"
> > the inline asm semantics, but the problem was never triggered in pre-5.9.
> > 
> > Sasha, with this and the GCC bug in mind, would you agree that (b) is the
> > better course of action?
> 
> Sasha,
> 
> Any chance we can get this patch reverted as David has suggested? It
> was backported to 5.4 LTS in commit 653ae33b030b ("KVM: arm64: Fix
> symbol dependency in __hyp_call_panic_nvhe") and is causing build
> issues with a 4.9.4 vintage of GCC.

Absolutely. This issue has been lingering for some time now, and needs
addressing.

I'm definitely in favour of reverting this patch, although there are
two possible alternatives:

- Cherry-pick 9fd339a45be5 ("arm64: Work around broken GCC 4.9
  handling of "S" constraint"), which works around this particular GCC
  bug

- Cherry-pick dca5244d2f5b ("compiler.h: Raise minimum version of GCC
  to 5.1 for arm64"), which forbids GCC 4.9 as it has been
  demonstrated to mis-compile the kernel (and this patch is targeting
  stable anyway)

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [BUG]: KVM: arm64: Fix symbol dependency in __hyp_call_panic_nvhe

[Oliver Upton]

On Mon, Jan 25, 2021 at 12:56 PM Marc Zyngier <maz@kernel.org> wrote:
> - Cherry-pick 9fd339a45be5 ("arm64: Work around broken GCC 4.9
>   handling of "S" constraint"), which works around this particular GCC
>   bug
>
> - Cherry-pick dca5244d2f5b ("compiler.h: Raise minimum version of GCC
>   to 5.1 for arm64"), which forbids GCC 4.9 as it has been
>   demonstrated to mis-compile the kernel (and this patch is targeting
>   stable anyway)

If the latter is hitting stable then it sounds like high time to throw
out my broken GCC. Thanks Marc!

--
Oliver