Message ID | 20210923151031.72408-1-mpe@ellerman.id.au (mailing list archive) |
---|---|
State | Accepted |
Headers | show |
Series | KVM: PPC: Book3S HV: Use GLOBAL_TOC for kvmppc_h_set_dabr/xdabr() | expand |
Context | Check | Description |
---|---|---|
snowpatch_ozlabs/github-powerpc_selftests | success | Successfully ran 8 jobs. |
snowpatch_ozlabs/github-powerpc_ppctests | success | Successfully ran 8 jobs. |
snowpatch_ozlabs/github-powerpc_sparse | success | Successfully ran 4 jobs. |
snowpatch_ozlabs/github-powerpc_clang | success | Successfully ran 7 jobs. |
snowpatch_ozlabs/github-powerpc_kernel_qemu | success | Successfully ran 24 jobs. |
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
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
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
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:
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(-)