Re: [PATCH 0/3] KVM: arm64: Ask the compiler to __always_inline functions used by KVM at HYP

From: Marc Zyngier <maz@kernel.org>
To: James Morse <james.morse@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>,
	Ard Biesheuvel <ard.biesheuvel@linaro.org>,
	linux-arm-kernel@lists.infradead.org,
	Sami Tolvanen <samitolvanen@google.com>,
	Will Deacon <will@kernel.org>,
	kvmarm@lists.cs.columbia.edu
Subject: Re: [PATCH 0/3] KVM: arm64: Ask the compiler to __always_inline functions used by KVM at HYP
Date: Fri, 21 Feb 2020 16:38:11 +0000	[thread overview]
Message-ID: <6479dd67fbe12c6517551dbd79dc7461@kernel.org> (raw)
In-Reply-To: <228ef9d9-1ded-05ee-22e5-0158a0e47d82@arm.com>

Hi James,

On 2020-02-21 14:57, James Morse wrote:
> Hi Marc,
> 
> On 21/02/2020 12:55, Marc Zyngier wrote:
>> On 2020-02-20 16:58, James Morse wrote:
>>> It turns out KVM relies on the inline hint being honoured by the 
>>> compiler
>>> in quite a few more places than expected. Something about the Shadow 
>>> Call
>>> Stack support[0] causes the compiler to avoid inline-ing and to place
>>> these functions outside the __hyp_text. This ruins KVM's day.
>>> 
>>> Add the simon-says __always_inline annotation to all the static
>>> inlines that KVM calls from HYP code.
>>> 
>>> This series based on v5.6-rc2.
>> 
>> Many thanks for going through all this.
>> 
>> I'm happy to take it if Catalin or Will ack the arm64 patches.
>> It case we decide to go the other way around:
>> 
>> Acked-by: Marc Zyngier <maz@kernel.org>
>> 
>> One thing I'd like to look into though is a compile-time check that
>> nothing in the hyp_text section has a reference to a non-hyp_text
>> symbol.
> 
> Heh, that hypothetical tool would choke on things like 
> arch/arm64/kvm/hyp/tlb.c:
> | static void __hyp_text __tlb_switch_to_guest_vhe(...)
> | {
> 
> [...]
> 
> |	local_irq_save(cxt->flags);
> 
> which calls trace_hardirqs_off() ... which is absolutely fine because
> this only happens on VHE.

Duh, indeed.

> To do it purely with the section information, you'd need to separate
> all the VHE code... (maybe as a debug option that only runs when VHE
> is turned off?)

We may have to to that anyway at some point. If the "KVM compartment"
thing becomes real, we may have to end-up compiling both separately
(and jettison the one we don't need at runtime).

>> We already have checks around non-init symbols pointing to init 
>> symbols,
>> and I was wondering if we could reuse this for fun and profit...
> 
> I think objtool is the tool-of-the-future that can do this. You need
> something that believes everything behind has_vhe() is unreachable...

I need to educate myself about objtool. Seems to be the miracle cure
for a lot of ailments! ;-)

Anyway, I've now queued the series for 5.6.

          M.
-- 
Jazz is not dead. It just smells funny...
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm