From: Catalin Marinas <catalin.marinas@arm.com>
To: Laura Abbott <labbott@redhat.com>
Cc: Mark Rutland <mark.rutland@arm.com>,
Ard Biesheuvel <ard.biesheuvel@linaro.org>,
x86@kernel.org, Will Deacon <will.deacon@arm.com>,
linux-kernel@vger.kernel.org, linux-mm@kvack.org,
Ingo Molnar <mingo@redhat.com>,
Thomas Gleixner <tglx@linutronix.de>,
"H. Peter Anvin" <hpa@zytor.com>,
Andrew Morton <akpm@linux-foundation.org>,
Joonsoo Kim <iamjoonsoo.kim@lge.com>,
linux-arm-kernel@lists.infradead.org,
Marek Szyprowski <m.szyprowski@samsung.com>
Subject: Re: [PATCHv2 5/6] arm64: Use __pa_symbol for _end
Date: Wed, 16 Nov 2016 17:32:17 +0000 [thread overview]
Message-ID: <20161116173217.GB3224@e104818-lin.cambridge.arm.com> (raw)
In-Reply-To: <95d1f7bb-d451-3b0a-1a32-957a24023a49@redhat.com>
On Tue, Nov 15, 2016 at 04:09:07PM -0800, Laura Abbott wrote:
> On 11/15/2016 10:35 AM, Catalin Marinas wrote:
> > I'm fine with __pa_symbol use entirely from under arch/arm64. But if you
> > want to use __pa_symbol, I tried to change most (all?) places where
> > necessary, together with making virt_to_phys() only deal with the kernel
> > linear mapping. Not sure it looks cleaner, especially the
> > __va(__pa_symbol()) cases (we could replace the latter with another
> > macro and proper comment):
>
> I agree everything should be converted over, I was considering doing
> that in a separate patch but this covers everything nicely. Are you
> okay with me folding this in? (Few comments below)
Yes. I would also like Ard to review it since he introduced the current
__virt_to_phys() macro.
> > diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h
> > index eac3dbb7e313..e02f45e5ee1b 100644
> > --- a/arch/arm64/include/asm/memory.h
> > +++ b/arch/arm64/include/asm/memory.h
> > @@ -169,15 +169,22 @@ extern u64 kimage_voffset;
> > */
> > #define __virt_to_phys_nodebug(x) ({ \
> > phys_addr_t __x = (phys_addr_t)(x); \
> > - __x & BIT(VA_BITS - 1) ? (__x & ~PAGE_OFFSET) + PHYS_OFFSET : \
> > - (__x - kimage_voffset); })
> > + VM_BUG_ON(!(__x & BIT(VA_BITS - 1))); \
> > + ((__x & ~PAGE_OFFSET) + PHYS_OFFSET); \
> > +})
>
> I do think this is easier to understand vs the ternary operator.
> I'll add a comment detailing the use of __pa vs __pa_symbol somewhere
> as well.
Of course, a comment is welcome (I just did a quick hack to check that
it works).
> > --- a/arch/arm64/include/asm/mmu_context.h
> > +++ b/arch/arm64/include/asm/mmu_context.h
> > @@ -44,7 +44,7 @@ static inline void contextidr_thread_switch(struct task_struct *next)
> > */
> > static inline void cpu_set_reserved_ttbr0(void)
> > {
> > - unsigned long ttbr = virt_to_phys(empty_zero_page);
> > + unsigned long ttbr = __pa_symbol(empty_zero_page);
> >
> > write_sysreg(ttbr, ttbr0_el1);
> > isb();
> > @@ -113,7 +113,7 @@ static inline void cpu_install_idmap(void)
> > local_flush_tlb_all();
> > cpu_set_idmap_tcr_t0sz();
> >
> > - cpu_switch_mm(idmap_pg_dir, &init_mm);
> > + cpu_switch_mm(__va(__pa_symbol(idmap_pg_dir)), &init_mm);
>
> Yes, the __va(__pa_symbol(..)) idiom needs to be macroized and commented...
Indeed. At the same time we should also replace the LMADDR macro in
hibernate.c with whatever you come up with.
> > diff --git a/arch/arm64/kernel/hibernate.c b/arch/arm64/kernel/hibernate.c
> > index d55a7b09959b..81c03c74e5fe 100644
> > --- a/arch/arm64/kernel/hibernate.c
> > +++ b/arch/arm64/kernel/hibernate.c
> > @@ -51,7 +51,7 @@
> > extern int in_suspend;
> >
> > /* Find a symbols alias in the linear map */
> > -#define LMADDR(x) phys_to_virt(virt_to_phys(x))
> > +#define LMADDR(x) __va(__pa_symbol(x))
>
> ...Perhaps just borrowing this macro?
Yes but I don't particularly like the name, especially since it goes
into a .h file. Maybe __lm_sym_addr() or something else if you have a
better idea.
> > diff --git a/arch/arm64/mm/physaddr.c b/arch/arm64/mm/physaddr.c
> > index 874c78201a2b..98dae943e496 100644
> > --- a/arch/arm64/mm/physaddr.c
> > +++ b/arch/arm64/mm/physaddr.c
> > @@ -14,8 +14,8 @@ unsigned long __virt_to_phys(unsigned long x)
> > */
> > return (__x & ~PAGE_OFFSET) + PHYS_OFFSET;
> > } else {
> > - VIRTUAL_BUG_ON(x < kimage_vaddr || x >= (unsigned long)_end);
> > - return (__x - kimage_voffset);
> > + WARN_ON(1);
>
> Was the deletion of the BUG_ON here intentional? VIRTUAL_BUG_ON
> is the check enabled by CONFIG_DEBUG_VIRTUAL vs just CONFIG_DEBUG_VM.
> I intentionally kept CONFIG_DEBUG_VIRTUAL separate since the checks
> are expensive.
I wanted to always get a warning but fall back to __phys_addr_symbol()
so that I can track down other uses of __virt_to_phys() on kernel
symbols without killing the kernel. A better option would have been
VIRTUAL_WARN_ON (or *_ONCE) but we don't have it. VM_WARN_ON, as you
said, is independent of CONFIG_DEBUG_VIRTUAL.
We could as well kill the system with VIRTUAL_BUG_ON in this case but I
thought we should be more gentle until all the __virt_to_phys use-cases
are sorted out.
--
Catalin
next prev parent reply other threads:[~2016-11-16 17:32 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-11-02 21:00 [PATCHv2 0/6] CONFIG_DEBUG_VIRTUAL for arm64 Laura Abbott
2016-11-02 21:00 ` [PATCHv2 1/6] lib/Kconfig.debug: Add ARCH_HAS_DEBUG_VIRTUAL Laura Abbott
2016-11-02 21:00 ` [PATCHv2 2/6] mm/cma: Cleanup highmem check Laura Abbott
2016-11-02 21:00 ` [PATCHv2 3/6] arm64: Move some macros under #ifndef __ASSEMBLY__ Laura Abbott
2016-11-02 21:00 ` [PATCHv2 4/6] arm64: Add cast for virt_to_pfn Laura Abbott
2016-11-02 21:00 ` [PATCHv2 5/6] arm64: Use __pa_symbol for _end Laura Abbott
2016-11-02 22:52 ` Mark Rutland
2016-11-02 23:56 ` Laura Abbott
2016-11-03 15:51 ` Mark Rutland
2016-11-14 18:19 ` Catalin Marinas
2016-11-14 18:41 ` Laura Abbott
2016-11-15 18:35 ` Catalin Marinas
2016-11-16 0:09 ` Laura Abbott
2016-11-16 17:32 ` Catalin Marinas [this message]
2016-11-18 10:23 ` Ard Biesheuvel
2016-11-02 21:00 ` [PATCHv2 6/6] arm64: Add support for CONFIG_DEBUG_VIRTUAL Laura Abbott
2016-11-02 23:06 ` Mark Rutland
2016-11-03 0:05 ` Laura Abbott
2016-11-03 15:57 ` Mark Rutland
2016-11-02 23:07 ` [PATCHv2 0/6] CONFIG_DEBUG_VIRTUAL for arm64 Mark Rutland
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=20161116173217.GB3224@e104818-lin.cambridge.arm.com \
--to=catalin.marinas@arm.com \
--cc=akpm@linux-foundation.org \
--cc=ard.biesheuvel@linaro.org \
--cc=hpa@zytor.com \
--cc=iamjoonsoo.kim@lge.com \
--cc=labbott@redhat.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=m.szyprowski@samsung.com \
--cc=mark.rutland@arm.com \
--cc=mingo@redhat.com \
--cc=tglx@linutronix.de \
--cc=will.deacon@arm.com \
--cc=x86@kernel.org \
/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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).