Re: [PATCH v2 04/16] arm64: Downgrade flush_icache_range to invalidate

From: Fuad Tabba <tabba@google.com>
To: Mark Rutland <mark.rutland@arm.com>
Cc: Ard Biesheuvel <ardb@kernel.org>,
	Linux ARM <linux-arm-kernel@lists.infradead.org>,
	 Will Deacon <will@kernel.org>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Marc Zyngier <maz@kernel.org>,  James Morse <james.morse@arm.com>,
	Alexandru Elisei <alexandru.elisei@arm.com>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	Robin Murphy <robin.murphy@arm.com>
Subject: Re: [PATCH v2 04/16] arm64: Downgrade flush_icache_range to invalidate
Date: Wed, 19 May 2021 17:29:02 +0100	[thread overview]
Message-ID: <CA+EHjTwTAKqO11dyzR8ZjY_4L5mWGco=L5gatYBCm=hAkzmPAg@mail.gmail.com> (raw)
In-Reply-To: <20210518160634.GO82842@C02TD0UTHF1T.local>

On Tue, May 18, 2021 at 5:06 PM Mark Rutland <mark.rutland@arm.com> wrote:
>
> On Tue, May 18, 2021 at 06:02:32PM +0200, Ard Biesheuvel wrote:
> > On Tue, 18 May 2021 at 17:53, Mark Rutland <mark.rutland@arm.com> wrote:
> > >
> > > On Mon, May 17, 2021 at 08:51:12AM +0100, Fuad Tabba wrote:
> > > > Since __flush_dcache_area is called right before,
> > > > invalidate_icache_range is sufficient in this case.
> > > >
> > > > Rewrite the comment to better explain the rationale behind the
> > > > cache maintenance operations used here.
> > > >
> > > > No functional change intended.
> > > > Possible performance impact due to invalidating only the icache
> > > > rather than invalidating and cleaning both caches.
> > > >
> > > > Reported-by: Catalin Marinas <catalin.marinas@arm.com>
> > > > Reported-by: Will Deacon <will@kernel.org>
> > > > Link: https://lore.kernel.org/linux-arch/20200511110014.lb9PEahJ4hVOYrbwIb_qUHXyNy9KQzNFdb_I3YlzY6A@z/
> > > > Signed-off-by: Fuad Tabba <tabba@google.com>
> > > > ---
> > > >  arch/arm64/kernel/machine_kexec.c | 10 +++++++---
> > > >  1 file changed, 7 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/arch/arm64/kernel/machine_kexec.c b/arch/arm64/kernel/machine_kexec.c
> > > > index 90a335c74442..ecd8915e02e1 100644
> > > > --- a/arch/arm64/kernel/machine_kexec.c
> > > > +++ b/arch/arm64/kernel/machine_kexec.c
> > > > @@ -68,10 +68,14 @@ int machine_kexec_post_load(struct kimage *kimage)
> > > >       kimage->arch.kern_reloc = __pa(reloc_code);
> > > >       kexec_image_info(kimage);
> > > >
> > > > -     /* Flush the reloc_code in preparation for its execution. */
> > > > +     /*
> > > > +      * For execution with the MMU off and I-cache on, reloc_code needs to be
> > > > +      * cleaned to the PoC and invalidated from the I-cache.
> > > > +      */
> > >
> > > Minor nit, but the I-cache is *always* on (SCTLR.I affects the
> > > attributes used for fetches into the I-caches), so it would be slightly
> > > better to drop the "and I-cache on" words.
> >
> > This may be true, but it may not be obvious to someone reading the
> > 'MMU off' part of the comment. Bottom line is that we will be running
> > in a mode where we may hit in the I-cache, so it needs to be
> > invalidated. If we miss in the I-cache, we should fetch from the PoC,
> > hence the D-cache clean.
>
> No disagreement there. I literally meant dropping the words "and I-cache
> on", leaving the whole comment as:
>
>         /*
>          * For execution with the MMU off, reloc_code needs to be
>          * cleaned to the PoC and invalidated from the I-cache.
>          */

Will fix this too.

Thanks,
/fuad

> Thanks,
> Mark.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel