From: Pavel Tatashin <pasha.tatashin@soleen.com>
To: Mark Rutland <mark.rutland@arm.com>
Cc: James Morris <jmorris@namei.org>, Sasha Levin <sashal@kernel.org>,
LKML <linux-kernel@vger.kernel.org>,
Catalin Marinas <catalin.marinas@arm.com>,
Will Deacon <will@kernel.org>,
steve.capper@arm.com,
Linux ARM <linux-arm-kernel@lists.infradead.org>,
Marc Zyngier <marc.zyngier@arm.com>,
James Morse <james.morse@arm.com>,
Vladimir Murzin <vladimir.murzin@arm.com>,
Thomas Gleixner <tglx@linutronix.de>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
allison@lohutok.net, info@metux.net, alexios.zavras@intel.com,
Stefano Stabellini <sstabellini@kernel.org>,
boris.ostrovsky@oracle.com, jgross@suse.com,
Stefan Agner <stefan@agner.ch>,
Masahiro Yamada <yamada.masahiro@socionext.com>,
xen-devel@lists.xenproject.org,
Russell King - ARM Linux admin <linux@armlinux.org.uk>
Subject: Re: [PATCH v2 2/3] arm64: remove uaccess_ttbr0 asm macros from cache functions
Date: Wed, 27 Nov 2019 10:10:07 -0500 [thread overview]
Message-ID: <CA+CK2bBvgDe5zVur7EYJgYhoZesuQkZVeyRxPCBSySqsR=-YPQ@mail.gmail.com> (raw)
In-Reply-To: <20191127150137.GB51937@lakrids.cambridge.arm.com>
Hi Mark,
Thank you for reviewing this work.
> A commit message should provide rationale, rather than just a
> description of the patch. Something like:
>
> | We currently duplicate the logic to enable/disable uaccess via TTBR0,
> | with C functions and assembly macros. This is a maintenenace burden
> | and is liable to lead to subtle bugs, so let's get rid of the assembly
> | macros, and always use the C functions. This requires refactoring
> | some assembly functions to have a C wrapper.
Thank you for suggestion, I will fix my commit log.
>
> [...]
>
> > +static inline int invalidate_icache_range(unsigned long start,
> > + unsigned long end)
> > +{
> > + int rv;
> > +#if ARM64_HAS_CACHE_DIC
> > + rv = arch_invalidate_icache_range(start, end);
> > +#else
> > + uaccess_ttbr0_enable();
> > + rv = arch_invalidate_icache_range(start, end);
> > + uaccess_ttbr0_disable();
> > +#endif
> > + return rv;
> > +}
>
> This ifdeffery is not the same as an alternative_if, and even if it were
> the ARM64_HAS_CACHE_DIC behaviour is not the same as the existing
> assembly.
>
> This should be:
>
> static inline int invalidate_icache_range(unsigned long start,
> unsigned long end)
> {
> int ret;
>
> if (cpus_have_const_cap(ARM64_HAS_CACHE_DIC)) {
> isb();
> return 0;
> }
>
> uaccess_ttbr0_enable();
> ret = arch_invalidate_icache_range(start, end);
> uaccess_ttbr0_disable();
>
> return ret;
> }
I will fix it, thanks.
>
> The 'arch_' prefix should probably be 'asm_' (or have an '_asm' suffix),
> since this is entirely local to the arch code, and even then should only
> be called from the C wrappers.
Sure, I can change it to asm_*, I was using arch_* to be consistent
with __arch_copy_from_user() and friends.
Thank you,
Pasha
WARNING: multiple messages have this Message-ID (diff)
From: Pavel Tatashin <pasha.tatashin@soleen.com>
To: Mark Rutland <mark.rutland@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>,
Stefan Agner <stefan@agner.ch>,
Russell King - ARM Linux admin <linux@armlinux.org.uk>,
Masahiro Yamada <yamada.masahiro@socionext.com>,
Will Deacon <will@kernel.org>,
boris.ostrovsky@oracle.com, Sasha Levin <sashal@kernel.org>,
Stefano Stabellini <sstabellini@kernel.org>,
James Morris <jmorris@namei.org>,
Linux ARM <linux-arm-kernel@lists.infradead.org>,
xen-devel@lists.xenproject.org,
Vladimir Murzin <vladimir.murzin@arm.com>,
Marc Zyngier <marc.zyngier@arm.com>,
alexios.zavras@intel.com, Thomas Gleixner <tglx@linutronix.de>,
allison@lohutok.net, jgross@suse.com, steve.capper@arm.com,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
LKML <linux-kernel@vger.kernel.org>,
James Morse <james.morse@arm.com>,
info@metux.net
Subject: Re: [PATCH v2 2/3] arm64: remove uaccess_ttbr0 asm macros from cache functions
Date: Wed, 27 Nov 2019 10:10:07 -0500 [thread overview]
Message-ID: <CA+CK2bBvgDe5zVur7EYJgYhoZesuQkZVeyRxPCBSySqsR=-YPQ@mail.gmail.com> (raw)
In-Reply-To: <20191127150137.GB51937@lakrids.cambridge.arm.com>
Hi Mark,
Thank you for reviewing this work.
> A commit message should provide rationale, rather than just a
> description of the patch. Something like:
>
> | We currently duplicate the logic to enable/disable uaccess via TTBR0,
> | with C functions and assembly macros. This is a maintenenace burden
> | and is liable to lead to subtle bugs, so let's get rid of the assembly
> | macros, and always use the C functions. This requires refactoring
> | some assembly functions to have a C wrapper.
Thank you for suggestion, I will fix my commit log.
>
> [...]
>
> > +static inline int invalidate_icache_range(unsigned long start,
> > + unsigned long end)
> > +{
> > + int rv;
> > +#if ARM64_HAS_CACHE_DIC
> > + rv = arch_invalidate_icache_range(start, end);
> > +#else
> > + uaccess_ttbr0_enable();
> > + rv = arch_invalidate_icache_range(start, end);
> > + uaccess_ttbr0_disable();
> > +#endif
> > + return rv;
> > +}
>
> This ifdeffery is not the same as an alternative_if, and even if it were
> the ARM64_HAS_CACHE_DIC behaviour is not the same as the existing
> assembly.
>
> This should be:
>
> static inline int invalidate_icache_range(unsigned long start,
> unsigned long end)
> {
> int ret;
>
> if (cpus_have_const_cap(ARM64_HAS_CACHE_DIC)) {
> isb();
> return 0;
> }
>
> uaccess_ttbr0_enable();
> ret = arch_invalidate_icache_range(start, end);
> uaccess_ttbr0_disable();
>
> return ret;
> }
I will fix it, thanks.
>
> The 'arch_' prefix should probably be 'asm_' (or have an '_asm' suffix),
> since this is entirely local to the arch code, and even then should only
> be called from the C wrappers.
Sure, I can change it to asm_*, I was using arch_* to be consistent
with __arch_copy_from_user() and friends.
Thank you,
Pasha
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
WARNING: multiple messages have this Message-ID (diff)
From: Pavel Tatashin <pasha.tatashin@soleen.com>
To: Mark Rutland <mark.rutland@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>,
Stefan Agner <stefan@agner.ch>,
Russell King - ARM Linux admin <linux@armlinux.org.uk>,
Masahiro Yamada <yamada.masahiro@socionext.com>,
Will Deacon <will@kernel.org>,
boris.ostrovsky@oracle.com, Sasha Levin <sashal@kernel.org>,
Stefano Stabellini <sstabellini@kernel.org>,
James Morris <jmorris@namei.org>,
Linux ARM <linux-arm-kernel@lists.infradead.org>,
xen-devel@lists.xenproject.org,
Vladimir Murzin <vladimir.murzin@arm.com>,
Marc Zyngier <marc.zyngier@arm.com>,
alexios.zavras@intel.com, Thomas Gleixner <tglx@linutronix.de>,
allison@lohutok.net, jgross@suse.com, steve.capper@arm.com,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
LKML <linux-kernel@vger.kernel.org>,
James Morse <james.morse@arm.com>,
info@metux.net
Subject: Re: [Xen-devel] [PATCH v2 2/3] arm64: remove uaccess_ttbr0 asm macros from cache functions
Date: Wed, 27 Nov 2019 10:10:07 -0500 [thread overview]
Message-ID: <CA+CK2bBvgDe5zVur7EYJgYhoZesuQkZVeyRxPCBSySqsR=-YPQ@mail.gmail.com> (raw)
In-Reply-To: <20191127150137.GB51937@lakrids.cambridge.arm.com>
Hi Mark,
Thank you for reviewing this work.
> A commit message should provide rationale, rather than just a
> description of the patch. Something like:
>
> | We currently duplicate the logic to enable/disable uaccess via TTBR0,
> | with C functions and assembly macros. This is a maintenenace burden
> | and is liable to lead to subtle bugs, so let's get rid of the assembly
> | macros, and always use the C functions. This requires refactoring
> | some assembly functions to have a C wrapper.
Thank you for suggestion, I will fix my commit log.
>
> [...]
>
> > +static inline int invalidate_icache_range(unsigned long start,
> > + unsigned long end)
> > +{
> > + int rv;
> > +#if ARM64_HAS_CACHE_DIC
> > + rv = arch_invalidate_icache_range(start, end);
> > +#else
> > + uaccess_ttbr0_enable();
> > + rv = arch_invalidate_icache_range(start, end);
> > + uaccess_ttbr0_disable();
> > +#endif
> > + return rv;
> > +}
>
> This ifdeffery is not the same as an alternative_if, and even if it were
> the ARM64_HAS_CACHE_DIC behaviour is not the same as the existing
> assembly.
>
> This should be:
>
> static inline int invalidate_icache_range(unsigned long start,
> unsigned long end)
> {
> int ret;
>
> if (cpus_have_const_cap(ARM64_HAS_CACHE_DIC)) {
> isb();
> return 0;
> }
>
> uaccess_ttbr0_enable();
> ret = arch_invalidate_icache_range(start, end);
> uaccess_ttbr0_disable();
>
> return ret;
> }
I will fix it, thanks.
>
> The 'arch_' prefix should probably be 'asm_' (or have an '_asm' suffix),
> since this is entirely local to the arch code, and even then should only
> be called from the C wrappers.
Sure, I can change it to asm_*, I was using arch_* to be consistent
with __arch_copy_from_user() and friends.
Thank you,
Pasha
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
next prev parent reply other threads:[~2019-11-27 15:10 UTC|newest]
Thread overview: 45+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-11-22 2:24 [PATCH v2 0/3] Use C inlines for uaccess Pavel Tatashin
2019-11-22 2:24 ` [Xen-devel] " Pavel Tatashin
2019-11-22 2:24 ` Pavel Tatashin
2019-11-22 2:24 ` [PATCH v2 1/3] arm/arm64/xen: use C inlines for privcmd_call Pavel Tatashin
2019-11-22 2:24 ` [Xen-devel] " Pavel Tatashin
2019-11-22 2:24 ` Pavel Tatashin
2019-11-22 19:59 ` Stefano Stabellini
2019-11-22 19:59 ` [Xen-devel] " Stefano Stabellini
2019-11-22 19:59 ` Stefano Stabellini
2019-11-22 2:24 ` [PATCH v2 2/3] arm64: remove uaccess_ttbr0 asm macros from cache functions Pavel Tatashin
2019-11-22 2:24 ` [Xen-devel] " Pavel Tatashin
2019-11-22 2:24 ` Pavel Tatashin
2019-11-27 15:01 ` Mark Rutland
2019-11-27 15:01 ` [Xen-devel] " Mark Rutland
2019-11-27 15:01 ` Mark Rutland
2019-11-27 15:10 ` Pavel Tatashin [this message]
2019-11-27 15:10 ` [Xen-devel] " Pavel Tatashin
2019-11-27 15:10 ` Pavel Tatashin
2019-11-27 15:14 ` Mark Rutland
2019-11-27 15:14 ` [Xen-devel] " Mark Rutland
2019-11-27 15:14 ` Mark Rutland
2019-11-22 2:24 ` [PATCH v2 3/3] arm64: remove the rest of asm-uaccess.h Pavel Tatashin
2019-11-22 2:24 ` [Xen-devel] " Pavel Tatashin
2019-11-22 2:24 ` Pavel Tatashin
2019-11-27 15:11 ` Mark Rutland
2019-11-27 15:11 ` [Xen-devel] " Mark Rutland
2019-11-27 15:11 ` Mark Rutland
2019-11-27 15:31 ` Pavel Tatashin
2019-11-27 15:31 ` [Xen-devel] " Pavel Tatashin
2019-11-27 15:31 ` Pavel Tatashin
2019-11-27 16:03 ` Mark Rutland
2019-11-27 16:03 ` [Xen-devel] " Mark Rutland
2019-11-27 16:03 ` Mark Rutland
2019-11-27 16:09 ` Pavel Tatashin
2019-11-27 16:09 ` [Xen-devel] " Pavel Tatashin
2019-11-27 16:09 ` Pavel Tatashin
2019-11-27 17:01 ` Mark Rutland
2019-11-27 17:01 ` [Xen-devel] " Mark Rutland
2019-11-27 17:01 ` Mark Rutland
2019-11-27 17:13 ` Pavel Tatashin
2019-11-27 17:13 ` [Xen-devel] " Pavel Tatashin
2019-11-27 17:13 ` Pavel Tatashin
2019-11-26 13:50 ` [PATCH v2 0/3] Use C inlines for uaccess Pavel Tatashin
2019-11-26 13:50 ` [Xen-devel] " Pavel Tatashin
2019-11-26 13:50 ` Pavel Tatashin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CA+CK2bBvgDe5zVur7EYJgYhoZesuQkZVeyRxPCBSySqsR=-YPQ@mail.gmail.com' \
--to=pasha.tatashin@soleen.com \
--cc=alexios.zavras@intel.com \
--cc=allison@lohutok.net \
--cc=boris.ostrovsky@oracle.com \
--cc=catalin.marinas@arm.com \
--cc=gregkh@linuxfoundation.org \
--cc=info@metux.net \
--cc=james.morse@arm.com \
--cc=jgross@suse.com \
--cc=jmorris@namei.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux@armlinux.org.uk \
--cc=marc.zyngier@arm.com \
--cc=mark.rutland@arm.com \
--cc=sashal@kernel.org \
--cc=sstabellini@kernel.org \
--cc=stefan@agner.ch \
--cc=steve.capper@arm.com \
--cc=tglx@linutronix.de \
--cc=vladimir.murzin@arm.com \
--cc=will@kernel.org \
--cc=xen-devel@lists.xenproject.org \
--cc=yamada.masahiro@socionext.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.