* [PATCH] KVM: PPC: Book3S HV: Use GLOBAL_TOC for kvmppc_h_set_dabr/xdabr()
@ 2021-09-23 15:10 Michael Ellerman
2021-09-30 6:12 ` Daniel Axtens
2021-11-17 11:23 ` Michael Ellerman
0 siblings, 2 replies; 4+ messages in thread
From: Michael Ellerman @ 2021-09-23 15:10 UTC (permalink / raw)
To: linuxppc-dev; +Cc: mikey, npiggin
kvmppc_h_set_dabr(), and kvmppc_h_set_xdabr() which jumps into
it, need to use _GLOBAL_TOC to setup the kernel TOC pointer, because
kvmppc_h_set_dabr() uses LOAD_REG_ADDR() to load dawr_force_enable.
When called from hcall_try_real_mode() we have the kernel TOC in r2,
established near the start of kvmppc_interrupt_hv(), so there is no
issue.
But they can also be called from kvmppc_pseries_do_hcall() which is
module code, so the access ends up happening with the kvm-hv module's
r2, which will not point at dawr_force_enable and could even cause a
fault.
With the current code layout and compilers we haven't observed a fault
in practice, the load hits somewhere in kvm-hv.ko and silently returns
some bogus value.
Note that we we expect p8/p9 guests to use the DAWR, but SLOF uses
h_set_dabr() to test if sc1 works correctly, see SLOF's
lib/libhvcall/brokensc1.c.
Fixes: c1fe190c0672 ("powerpc: Add force enable of DAWR on P9 option")
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
arch/powerpc/kvm/book3s_hv_rmhandlers.S | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/kvm/book3s_hv_rmhandlers.S b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
index 90484425a1e6..30a8a07cff18 100644
--- a/arch/powerpc/kvm/book3s_hv_rmhandlers.S
+++ b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
@@ -1999,7 +1999,7 @@ END_FTR_SECTION_IFSET(CPU_FTR_ARCH_300)
.globl hcall_real_table_end
hcall_real_table_end:
-_GLOBAL(kvmppc_h_set_xdabr)
+_GLOBAL_TOC(kvmppc_h_set_xdabr)
EXPORT_SYMBOL_GPL(kvmppc_h_set_xdabr)
andi. r0, r5, DABRX_USER | DABRX_KERNEL
beq 6f
@@ -2009,7 +2009,7 @@ EXPORT_SYMBOL_GPL(kvmppc_h_set_xdabr)
6: li r3, H_PARAMETER
blr
-_GLOBAL(kvmppc_h_set_dabr)
+_GLOBAL_TOC(kvmppc_h_set_dabr)
EXPORT_SYMBOL_GPL(kvmppc_h_set_dabr)
li r5, DABRX_USER | DABRX_KERNEL
3:
--
2.25.1
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] KVM: PPC: Book3S HV: Use GLOBAL_TOC for kvmppc_h_set_dabr/xdabr()
2021-09-23 15:10 [PATCH] KVM: PPC: Book3S HV: Use GLOBAL_TOC for kvmppc_h_set_dabr/xdabr() Michael Ellerman
@ 2021-09-30 6:12 ` Daniel Axtens
2021-09-30 6:59 ` Jordan Niethe
2021-11-17 11:23 ` Michael Ellerman
1 sibling, 1 reply; 4+ messages in thread
From: Daniel Axtens @ 2021-09-30 6:12 UTC (permalink / raw)
To: Michael Ellerman, linuxppc-dev; +Cc: mikey, npiggin
Hi Michael,
> kvmppc_h_set_dabr(), and kvmppc_h_set_xdabr() which jumps into
> it, need to use _GLOBAL_TOC to setup the kernel TOC pointer, because
> kvmppc_h_set_dabr() uses LOAD_REG_ADDR() to load dawr_force_enable.
This makes sense. LOAD_REG_ADDR() does ld reg,name@got(r2) and
_GLOBAL_TOC sets r2 based on r12 and .TOC. .
Looking at
e.g. https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1610846.html
it seems that we use GOT and TOC largely interchangeably... so assuming
I haven't completely misunderstood, the change this patch makes seems to
make sense to me. :)
> When called from hcall_try_real_mode() we have the kernel TOC in r2,
> established near the start of kvmppc_interrupt_hv(), so there is no
> issue.
>
> But they can also be called from kvmppc_pseries_do_hcall() which is
> module code, so the access ends up happening with the kvm-hv module's
> r2, which will not point at dawr_force_enable and could even cause a
> fault.
I checked and there isn't anywhere else the functions are called, so
this will now cover everything.
> With the current code layout and compilers we haven't observed a fault
> in practice, the load hits somewhere in kvm-hv.ko and silently returns
> some bogus value.
>
> Note that we we expect p8/p9 guests to use the DAWR, but SLOF uses
> h_set_dabr() to test if sc1 works correctly, see SLOF's
> lib/libhvcall/brokensc1.c.
I assume that something (the module loader?) patches the callsite to
restore r2 after the function call? I imagine something must otherwise
things would fall apart pretty quickly...
> Fixes: c1fe190c0672 ("powerpc: Add force enable of DAWR on P9 option")
That patch seems to only affect the DA_W_R not the DA_B_R - how does it
cause this bug?
All in all this looks good to me:
Reviewed-by: Daniel Axtens <dja@axtens.net>
Kind regards,
Daniel
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> ---
> arch/powerpc/kvm/book3s_hv_rmhandlers.S | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/powerpc/kvm/book3s_hv_rmhandlers.S b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
> index 90484425a1e6..30a8a07cff18 100644
> --- a/arch/powerpc/kvm/book3s_hv_rmhandlers.S
> +++ b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
> @@ -1999,7 +1999,7 @@ END_FTR_SECTION_IFSET(CPU_FTR_ARCH_300)
> .globl hcall_real_table_end
> hcall_real_table_end:
>
> -_GLOBAL(kvmppc_h_set_xdabr)
> +_GLOBAL_TOC(kvmppc_h_set_xdabr)
> EXPORT_SYMBOL_GPL(kvmppc_h_set_xdabr)
> andi. r0, r5, DABRX_USER | DABRX_KERNEL
> beq 6f
> @@ -2009,7 +2009,7 @@ EXPORT_SYMBOL_GPL(kvmppc_h_set_xdabr)
> 6: li r3, H_PARAMETER
> blr
>
> -_GLOBAL(kvmppc_h_set_dabr)
> +_GLOBAL_TOC(kvmppc_h_set_dabr)
> EXPORT_SYMBOL_GPL(kvmppc_h_set_dabr)
> li r5, DABRX_USER | DABRX_KERNEL
> 3:
> --
> 2.25.1
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] KVM: PPC: Book3S HV: Use GLOBAL_TOC for kvmppc_h_set_dabr/xdabr()
2021-09-30 6:12 ` Daniel Axtens
@ 2021-09-30 6:59 ` Jordan Niethe
0 siblings, 0 replies; 4+ messages in thread
From: Jordan Niethe @ 2021-09-30 6:59 UTC (permalink / raw)
To: Daniel Axtens; +Cc: Michael Neuling, linuxppc-dev, Nicholas Piggin
On Thu, Sep 30, 2021 at 4:13 PM Daniel Axtens <dja@axtens.net> wrote:
>
> Hi Michael,
>
> > kvmppc_h_set_dabr(), and kvmppc_h_set_xdabr() which jumps into
> > it, need to use _GLOBAL_TOC to setup the kernel TOC pointer, because
> > kvmppc_h_set_dabr() uses LOAD_REG_ADDR() to load dawr_force_enable.
>
> This makes sense. LOAD_REG_ADDR() does ld reg,name@got(r2) and
> _GLOBAL_TOC sets r2 based on r12 and .TOC. .
>
> Looking at
> e.g. https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1610846.html
> it seems that we use GOT and TOC largely interchangeably... so assuming
> I haven't completely misunderstood, the change this patch makes seems to
> make sense to me. :)
>
> > When called from hcall_try_real_mode() we have the kernel TOC in r2,
> > established near the start of kvmppc_interrupt_hv(), so there is no
> > issue.
> >
> > But they can also be called from kvmppc_pseries_do_hcall() which is
> > module code, so the access ends up happening with the kvm-hv module's
> > r2, which will not point at dawr_force_enable and could even cause a
> > fault.
>
> I checked and there isn't anywhere else the functions are called, so
> this will now cover everything.
>
> > With the current code layout and compilers we haven't observed a fault
> > in practice, the load hits somewhere in kvm-hv.ko and silently returns
> > some bogus value.
> >
> > Note that we we expect p8/p9 guests to use the DAWR, but SLOF uses
> > h_set_dabr() to test if sc1 works correctly, see SLOF's
> > lib/libhvcall/brokensc1.c.
>
> I assume that something (the module loader?) patches the callsite to
> restore r2 after the function call? I imagine something must otherwise
> things would fall apart pretty quickly...
>
> > Fixes: c1fe190c0672 ("powerpc: Add force enable of DAWR on P9 option")
>
> That patch seems to only affect the DA_W_R not the DA_B_R - how does it
> cause this bug?
Isn't it that patch which adds the LOAD_REG_ADDR(r11,
dawr_force_enable) to kvmppc_h_set_dabr() which is the problem?
>
> All in all this looks good to me:
> Reviewed-by: Daniel Axtens <dja@axtens.net>
>
> Kind regards,
> Daniel
>
> > Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> > ---
> > arch/powerpc/kvm/book3s_hv_rmhandlers.S | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/powerpc/kvm/book3s_hv_rmhandlers.S b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
> > index 90484425a1e6..30a8a07cff18 100644
> > --- a/arch/powerpc/kvm/book3s_hv_rmhandlers.S
> > +++ b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
> > @@ -1999,7 +1999,7 @@ END_FTR_SECTION_IFSET(CPU_FTR_ARCH_300)
> > .globl hcall_real_table_end
> > hcall_real_table_end:
> >
> > -_GLOBAL(kvmppc_h_set_xdabr)
> > +_GLOBAL_TOC(kvmppc_h_set_xdabr)
> > EXPORT_SYMBOL_GPL(kvmppc_h_set_xdabr)
> > andi. r0, r5, DABRX_USER | DABRX_KERNEL
> > beq 6f
> > @@ -2009,7 +2009,7 @@ EXPORT_SYMBOL_GPL(kvmppc_h_set_xdabr)
> > 6: li r3, H_PARAMETER
> > blr
> >
> > -_GLOBAL(kvmppc_h_set_dabr)
> > +_GLOBAL_TOC(kvmppc_h_set_dabr)
> > EXPORT_SYMBOL_GPL(kvmppc_h_set_dabr)
> > li r5, DABRX_USER | DABRX_KERNEL
> > 3:
> > --
> > 2.25.1
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] KVM: PPC: Book3S HV: Use GLOBAL_TOC for kvmppc_h_set_dabr/xdabr()
2021-09-23 15:10 [PATCH] KVM: PPC: Book3S HV: Use GLOBAL_TOC for kvmppc_h_set_dabr/xdabr() Michael Ellerman
2021-09-30 6:12 ` Daniel Axtens
@ 2021-11-17 11:23 ` Michael Ellerman
1 sibling, 0 replies; 4+ messages in thread
From: Michael Ellerman @ 2021-11-17 11:23 UTC (permalink / raw)
To: Michael Ellerman, linuxppc-dev; +Cc: mikey, npiggin
On Fri, 24 Sep 2021 01:10:31 +1000, Michael Ellerman wrote:
> kvmppc_h_set_dabr(), and kvmppc_h_set_xdabr() which jumps into
> it, need to use _GLOBAL_TOC to setup the kernel TOC pointer, because
> kvmppc_h_set_dabr() uses LOAD_REG_ADDR() to load dawr_force_enable.
>
> When called from hcall_try_real_mode() we have the kernel TOC in r2,
> established near the start of kvmppc_interrupt_hv(), so there is no
> issue.
>
> [...]
Applied to powerpc/fixes.
[1/1] KVM: PPC: Book3S HV: Use GLOBAL_TOC for kvmppc_h_set_dabr/xdabr()
https://git.kernel.org/powerpc/c/dae581864609d36fb58855fd59880b4941ce9d14
cheers
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-11-17 11:27 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-23 15:10 [PATCH] KVM: PPC: Book3S HV: Use GLOBAL_TOC for kvmppc_h_set_dabr/xdabr() Michael Ellerman
2021-09-30 6:12 ` Daniel Axtens
2021-09-30 6:59 ` Jordan Niethe
2021-11-17 11:23 ` Michael Ellerman
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).