linux-kernel - Re: [PATCH v3 04/11] kvm: arm64: Remove __hyp_this_cpu

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 21 Sep 2020 14:43:55 +0100
From:   David Brazdil <dbrazdil@...gle.com>
To:     Will Deacon <will@...nel.org>
Cc:     kvmarm@...ts.cs.columbia.edu,
        Catalin Marinas <catalin.marinas@....com>,
        Marc Zyngier <maz@...nel.org>,
        James Morse <james.morse@....com>,
        Julien Thierry <julien.thierry.kdev@...il.com>,
        Suzuki K Poulose <suzuki.poulose@....com>,
        Dennis Zhou <dennis@...nel.org>, Tejun Heo <tj@...nel.org>,
        Christoph Lameter <cl@...ux.com>,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        kernel-team@...roid.com, Andrew Scull <ascull@...gle.com>
Subject: Re: [PATCH v3 04/11] kvm: arm64: Remove __hyp_this_cpu_read

Hi Will,

> > +static inline unsigned long __hyp_my_cpu_offset(void)
> > +{
> > +	unsigned long off;
> > +
> > +	/*
> > +	 * We want to allow caching the value, so avoid using volatile and
> > +	 * instead use a fake stack read to hazard against barrier().
> > +	 */
> 
> I don't think we need to copy/paste the comment...
> 
> > +	asm("mrs %0, tpidr_el2" : "=r" (off) :
> > +		"Q" (*(const unsigned long *)current_stack_pointer));
> 
> ... especially given that we're not preemptible at EL2 with nVHE, maybe
> we don't need to play this trick at all because we're always going to be
> on the same CPU. So we could actually just do:
> 
> 	return read_sysreg(tpidr_el2);
> 
> which is much better, and the comment should say something to that effect.

I must be misinterpreting the comment. I understood that it enables the compiler
optimizing multiple reads of TPIDR by avoiding 'asm volatile' (signaling that
the value does not change between reads). So what exactly does it do?

read_sysreg expands to 'asm volatile' but I have no problem with priotizing
readability over a micro-optimization.

> > +#if defined(__KVM_NVHE_HYPERVISOR__) || defined(__KVM_VHE_HYPERVISOR__)
> > +#define __my_cpu_offset __hyp_my_cpu_offset()
> 
> Why would VHE code need to use this? Especially in light of my preemption
> comments above, shouldn't it now be using __kern_my_cpu_offset()?

During v2 review Andrew Scull pointed out we can avoid alternatives on VHE code
by using __hyp_my_cpu_offset for it as well. Obviously if __hyp_my_cpu_offset
becomes nVHE-specific, we can always move VHE back to __kern. This was just
about saving a few cycles during boot.

David