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
Hi,
> Am 07.09.2020 um 16:22 schrieb Sasha Levin <[email protected]>:
>
> 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
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
> 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
Hi all,
On Mon, 25 Jan 2021 20:07:56 +0000,
Oliver Upton <[email protected]> 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.
On Mon, Jan 25, 2021 at 12:56 PM Marc Zyngier <[email protected]> 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