Re: [PATCH] KVM: PPC: Book3S HV: Use GLOBAL_TOC for kvmppc_h_set_dabr/xdabr()

[Daniel Axtens <dja@axtens.net>]

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