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]
Message-ID: <20200921134355.5lzma3qzyiexxepd@google.com>
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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ