Re: [PATCH v4 10/19] KVM: arm/arm64: Do not use kern_hyp_va() with kvm_vgic_global_state

[Christoffer Dall <christoffer.dall@linaro.org>]

On Fri, Feb 16, 2018 at 09:33:39AM +0000, Marc Zyngier wrote:
> On 16/02/18 09:05, Christoffer Dall wrote:
> > On Thu, Feb 15, 2018 at 01:22:56PM +0000, Marc Zyngier wrote:
> >> On 15/01/18 15:36, Christoffer Dall wrote:
> >>> On Thu, Jan 04, 2018 at 06:43:25PM +0000, Marc Zyngier wrote:
> >>>> kvm_vgic_global_state is part of the read-only section, and is
> >>>> usually accessed using a PC-relative address generation (adrp + add).
> >>>>
> >>>> It is thus useless to use kern_hyp_va() on it, and actively problematic
> >>>> if kern_hyp_va() becomes non-idempotent. On the other hand, there is
> >>>> no way that the compiler is going to guarantee that such access is
> >>>> always be PC relative.
> >>>
> >>> nit: is always be
> >>>
> >>>>
> >>>> So let's bite the bullet and provide our own accessor.
> >>>>
> >>>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> >>>> ---
> >>>>  arch/arm/include/asm/kvm_hyp.h   | 6 ++++++
> >>>>  arch/arm64/include/asm/kvm_hyp.h | 9 +++++++++
> >>>>  virt/kvm/arm/hyp/vgic-v2-sr.c    | 4 ++--
> >>>>  3 files changed, 17 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/arch/arm/include/asm/kvm_hyp.h b/arch/arm/include/asm/kvm_hyp.h
> >>>> index ab20ffa8b9e7..1d42d0aa2feb 100644
> >>>> --- a/arch/arm/include/asm/kvm_hyp.h
> >>>> +++ b/arch/arm/include/asm/kvm_hyp.h
> >>>> @@ -26,6 +26,12 @@
> >>>>  
> >>>>  #define __hyp_text __section(.hyp.text) notrace
> >>>>  
> >>>> +#define hyp_symbol_addr(s)						\
> >>>> +	({								\
> >>>> +		typeof(s) *addr = &(s);					\
> >>>> +		addr;							\
> >>>> +	})
> >>>> +
> >>>>  #define __ACCESS_VFP(CRn)			\
> >>>>  	"mrc", "mcr", __stringify(p10, 7, %0, CRn, cr0, 0), u32
> >>>>  
> >>>> diff --git a/arch/arm64/include/asm/kvm_hyp.h b/arch/arm64/include/asm/kvm_hyp.h
> >>>> index 08d3bb66c8b7..a2d98c539023 100644
> >>>> --- a/arch/arm64/include/asm/kvm_hyp.h
> >>>> +++ b/arch/arm64/include/asm/kvm_hyp.h
> >>>> @@ -25,6 +25,15 @@
> >>>>  
> >>>>  #define __hyp_text __section(.hyp.text) notrace
> >>>>  
> >>>> +#define hyp_symbol_addr(s)						\
> >>>> +	({								\
> >>>> +		typeof(s) *addr;					\
> >>>> +		asm volatile("adrp	%0, %1\n"			\
> >>>> +			     "add	%0, %0, :lo12:%1\n"		\
> >>>> +			     : "=r" (addr) : "S" (&s));			\
> >>>
> >>> Can't we use adr_l here?
> >>
> >> Unfortunately not. All the asm/assembler.h macros are unavailable to
> >> inline assembly. We could start introducing equivalent macros for that
> >> purpose, but that's starting to be outside of the scope of this series.
> >>
> > 
> > Absolutely.  Forget I asked.
> > 
> >>>
> >>>> +		addr;							\
> >>>> +	})
> >>>> +
> >>>
> >>> I don't fully appreciate the semantics of this macro going by its name
> >>> only.  My understanding is that if you want to resolve a symbol to an
> >>> address which is mapped in hyp, then use this.  Is this correct?
> >>
> >> The goal of this macro is to return a symbol's address based on a
> >> PC-relative computation, as opposed to a loading the VA from a constant
> >> pool or something similar. This works well for HYP, as an absolute VA is
> >> guaranteed to be wrong.
> >>
> >>>
> >>> If so, can we add a small comment (because I can't come up with a better
> >>> name).
> >>
> >> I'll add the above if that works for you.
> >>
> > 
> > Yes it does.  The only thing that remains a bit unclear is what the
> > difference between this and kern_hyp_va is, and when you'd choose to use
> > one over the other.  Perhaps we need a single place which documents our
> > primitives and tells us what to use when.  At least, I'm for sure not
> > going to be able to figure this out later on.
> 
> Let me try to explain that:
> 
> The two primitives work on different "objects". kern_hyp_va() works on
> an address. If what you have is a pointer, then kern_hyp_va is your
> friend. On the contrary, if what you have is a symbol instead of the
> address of that object (and thus not something we obtain by reading a
> variable), then hyp_symbol_addr is probably what you need.
> 
> Of course, a symbol can also be a variable, which makes things a bit
> harder. The asm constraint are such as compilation will break if you try
> to treat a local variable as a symbol (the 'S' constraint is defined as
> "An absolute symbolic address or a label reference", and the '&s' makes
> it pretty hard to fool).
> 
> I've tried to make it make it foolproof, but who knows... ;-)
> 
ok, so what exactly is this macro doing different then simply using the
symbol directly?  Only ensuring that the generated code is using
PC-relative addressing?  If so, should we be using this macro everywhere
in Hyp code where we dereference a symbol?

What is it about hyp code that makes us need this when it's not needed
for the kernel itself, and both support address space randomization?  Is
that because the main kernel is properly relocated after deciding on the
randomization, but Hyp is not?

It may be worth trying to put hyp_symbol_addr and kern_hyp_va in the
same header file and have something explaining when to use what and why
in that single place, but if that breaks the world, then never mind...

Thanks,
-Christoffer