| Message ID | 20200430144831.59194-4-dbrazdil@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Split off nVHE hyp code | expand |
On Thu, 30 Apr 2020 15:48:19 +0100, David Brazdil <dbrazdil@google.com> wrote: > > __hyp_call_panic_nvhe contains inline assembly which did not declare > its dependency on the __hyp_panic_string symbol. > > Signed-off-by: David Brazdil <dbrazdil@google.com> > --- > arch/arm64/kvm/hyp/switch.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c > index 8a1e81a400e0..7a7c08029d81 100644 > --- a/arch/arm64/kvm/hyp/switch.c > +++ b/arch/arm64/kvm/hyp/switch.c > @@ -836,7 +836,7 @@ static void __hyp_text __hyp_call_panic_nvhe(u64 spsr, u64 elr, u64 par, > * making sure it is a kernel address and not a PC-relative > * reference. > */ > - asm volatile("ldr %0, =__hyp_panic_string" : "=r" (str_va)); > + asm volatile("ldr %0, =%1" : "=r" (str_va) : "S" (__hyp_panic_string)); > > __hyp_do_panic(str_va, > spsr, elr, > -- > 2.26.1 > > What breaks without this constraint? Is it a fix that should go in early? Otherwise looks good. Thanks, M.
Hi Marc, > > What breaks without this constraint? Is it a fix that should go in > early? Otherwise looks good. This only becomes an issue when __hyp_call_panic_nvhe() and __hyp_call_panic_vhe() are moved to separate files, so I don't think it's necessary to go in early. Currently the string variable (declared static) is seen by the C compiler as used by __hyp_call_panic_vhe(). But when split, the variable in the nVHE source file becomes unused, is dropped by the compiler and the inline assembly's reference is unresolved. We could then alias __hyp_text___hyp_panic_string back to the VHE copy, but this is the right way of addressing it. Thanks for the review, David
Hi David, On Thu, 07 May 2020 15:36:17 +0100, David Brazdil <dbrazdil@google.com> wrote: > > Hi Marc, > > > > > What breaks without this constraint? Is it a fix that should go in > > early? Otherwise looks good. > > This only becomes an issue when __hyp_call_panic_nvhe() and > __hyp_call_panic_vhe() are moved to separate files, so I don't think > it's necessary to go in early. > > Currently the string variable (declared static) is seen by the C > compiler as used by __hyp_call_panic_vhe(). But when split, the > variable in the nVHE source file becomes unused, is dropped by the > compiler and the inline assembly's reference is unresolved. We could > then alias __hyp_text___hyp_panic_string back to the VHE copy, but > this is the right way of addressing it. Thanks for the detailed explanation. I think some of it should make it in the commit message, pointing what breaks and when. M.
diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c index 8a1e81a400e0..7a7c08029d81 100644 --- a/arch/arm64/kvm/hyp/switch.c +++ b/arch/arm64/kvm/hyp/switch.c @@ -836,7 +836,7 @@ static void __hyp_text __hyp_call_panic_nvhe(u64 spsr, u64 elr, u64 par, * making sure it is a kernel address and not a PC-relative * reference. */ - asm volatile("ldr %0, =__hyp_panic_string" : "=r" (str_va)); + asm volatile("ldr %0, =%1" : "=r" (str_va) : "S" (__hyp_panic_string)); __hyp_do_panic(str_va, spsr, elr,
__hyp_call_panic_nvhe contains inline assembly which did not declare its dependency on the __hyp_panic_string symbol. Signed-off-by: David Brazdil <dbrazdil@google.com> --- arch/arm64/kvm/hyp/switch.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)