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@xxxxxxx>
>>>> ---
>>>> 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... ;-)
Hope this helps,
M.
--
Jazz is not dead. It just smells funny...
