Date:   Mon, 22 Jun 2020 11:39:32 +0100
From:   Andrew Scull <ascull@google.com>
To:     Mark Rutland <mark.rutland@arm.com>
Cc:     Marc Zyngier <maz@kernel.org>, kvm@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org, kvmarm@lists.cs.columbia.edu,
        James Morse <james.morse@arm.com>,
        Julien Thierry <julien.thierry.kdev@gmail.com>,
        Suzuki K Poulose <suzuki.poulose@arm.com>,
        Dave Martin <Dave.Martin@arm.com>, kernel-team@android.com
Subject: Re: [PATCH v2 5/5] KVM: arm64: Simplify PtrAuth alternative patching
Message-ID: <20200622103932.GA178085@google.com>
References: <20200622080643.171651-1-maz@kernel.org>
 <20200622080643.171651-6-maz@kernel.org>
 <20200622091508.GB88608@C02TD0UTHF1T.local>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20200622091508.GB88608@C02TD0UTHF1T.local>
Sender: kvm-owner@vger.kernel.org
Precedence: bulk

On Mon, Jun 22, 2020 at 10:15:08AM +0100, Mark Rutland wrote:
> On Mon, Jun 22, 2020 at 09:06:43AM +0100, Marc Zyngier wrote:


> > --- a/arch/arm64/include/asm/kvm_ptrauth.h
> > +++ b/arch/arm64/include/asm/kvm_ptrauth.h
> > @@ -61,44 +61,36 @@
> >  
> >  /*
> >   * Both ptrauth_switch_to_guest and ptrauth_switch_to_host macros will
> > - * check for the presence of one of the cpufeature flag
> > - * ARM64_HAS_ADDRESS_AUTH_ARCH or ARM64_HAS_ADDRESS_AUTH_IMP_DEF and
> > + * check for the presence ARM64_HAS_ADDRESS_AUTH, which is defined as
> > + * (ARM64_HAS_ADDRESS_AUTH_ARCH || ARM64_HAS_ADDRESS_AUTH_IMP_DEF) and
> >   * then proceed ahead with the save/restore of Pointer Authentication
> > - * key registers.
> > + * key registers if enabled for the guest.
> >   */
> >  .macro ptrauth_switch_to_guest g_ctxt, reg1, reg2, reg3
> > -alternative_if ARM64_HAS_ADDRESS_AUTH_ARCH
> > +alternative_if_not ARM64_HAS_ADDRESS_AUTH
> >  	b	1000f
> >  alternative_else_nop_endif
> > -alternative_if_not ARM64_HAS_ADDRESS_AUTH_IMP_DEF
> > -	b	1001f
> > -alternative_else_nop_endif
> > -1000:
> >  	mrs	\reg1, hcr_el2
> >  	and	\reg1, \reg1, #(HCR_API | HCR_APK)
> > -	cbz	\reg1, 1001f
> > +	cbz	\reg1, 1000f
> >  	add	\reg1, \g_ctxt, #CPU_APIAKEYLO_EL1
> >  	ptrauth_restore_state	\reg1, \reg2, \reg3
> > -1001:
> > +1000:
> >  .endm
> 
> Since these are in macros, we could use \@ to generate a macro-specific
> lavel rather than a magic number, which would be less likely to conflict
> with the surrounding environment and would be more descriptive. We do
> that in a few places already, and here it could look something like:
> 
> | alternative_if_not ARM64_HAS_ADDRESS_AUTH
> | 	b	.L__skip_pauth_switch\@
> | alternative_else_nop_endif
> | 	
> | 	...
> | 
> | .L__skip_pauth_switch\@:
> 
> Per the gas documentation
> 
> | \@
> |
> |    as maintains a counter of how many macros it has executed in this
> |    pseudo-variable; you can copy that number to your output with ‘\@’,
> |    but only within a macro definition.

Is this relibale for this sort of application? The description just
sounds like a counter of macros rather than specifically a unique label
generator. It may work most of the time but also seems that it has the
potential to be more fragile given that it would change based on the
rest of the code in the file to potentially conflict with something it
didn't previously conflict with.