On Thu, Aug 23, 2012 at 07:46:30AM +0100, Arnd Bergmann wrote:
On Thursday 16 August 2012, Will Deacon wrote:
quoted
On Wed, Aug 15, 2012 at 03:34:04PM +0100, Arnd Bergmann wrote:
quoted
On Tuesday 14 August 2012, Catalin Marinas wrote:
quoted
+asmlinkage int compat_sys_personality(compat_ulong_t personality)
+{
+ int ret;
+
+ if (personality(current->personality) == PER_LINUX32 &&
+ personality == PER_LINUX)
+ personality = PER_LINUX32;
+ ret = sys_personality(personality);
+ if (ret == PER_LINUX32)
+ ret = PER_LINUX;
+ return ret;
+}
Where did you get this from?
You should not need compat_sys_personality, just call the native function.
Hmm, but in that case an aarch32 application doing a personality(PER_LINUX)
syscall will start seeing the wrong uname.
Coming back at this topic, I noticed another issue. Jiri Kosina
has recently posted patches to fix this function in the other architectures
in order to mask out the other personality bits, which is a correct fix,
but the above function is odd for other reasons.
* On MIPS, it is used only for compat tasks, like you have it above.
* On PA-RISC, it is used for native 32 bit tasks and for compat 32 bit tasks,
but not for native 64 bit ones.
* On IA64, it was used for compat tasks (support for which has since
been removed from the kernel), plus all 32 bit tasks would start with
PER_LINUX32.
* On PowerPC, Sparc and s390, it is used for native 64 bit tasks and for
compat 32 bit tasks, but not for native 32 bit ones.
* On Tile, it was never used.
* On x86_64, it used to be defined (copied from ia64) but not used
throughout the git history.
The semantics of the function are also interesting: The intention seems
to be that to a compat task, PER_LINUX32 would appear as PER_LINUX.
The effect is that any process can set PER_LINUX32 but it can never
be unset except by a 64 bit MIPS or PA-RISC task.
IMHO, it makes sense to keep the compat_sys_personality() as implemented
above. You may want to start a chroot ARMv7 environment using "linux32"
but don't want some 32-bit app calling personality(PER_LINUX) (as that's
the default personality on an ARMv7 system) and unknowingly changing the
personality that you wanted to enforce via "linux32".
I agree with not setting the personality based on the ELF type, but
that's different from the compat_sys_personality().
Since x86_64 does not implement this behavior at all, I suspect that
there are now lots of things depending on not having it, while all
the other architectures might also have some (even predating the
x86_64 port) use cases that depend on depend on not being able to
observe PER_LINUX32 in 32 bit compat tasks.
I think we should try to agree on how this is all supposed to work
and use common code, either put the ppc/sparc/s390 version into
sys_personality, or remove all of them and just do what x86 and tile
do, using the regular sys_personality for all tasks.
Late topic for the KS :).
I don't think we can move this behaviour to sys_personality. We may want
to add a generic compat_sys_personality() if we agree on the above
use-case.
--
Catalin
On Fri, Aug 17, 2012 at 3:35 PM, Catalin Marinas
[off-list ref] wrote:
quoted
On Fri, Aug 17, 2012 at 10:41:10AM +0100, Santosh Shilimkar wrote:
quoted
So you expect all the secondary CPUs to be in wakeup state and probably
looping in WFE for a signal from kernel to boot. There is one issue
with this requirement though. For large CPU system, you need to reset
all the CPUs and hit this waiting loop. This will lead to large inrush
current need at bootup which may be not be supported. To avoid this
issue, secondary CPUs are kept in OFF state and then they are woken
up from kernel one by one whenever they need to be brought into the
system. This requirement should be considered.
I agree, this part will be extended. That's one method that we currently
support and suitable to the model.
The better method is the SMC standardisation that Charles Garcia-Tobin
has written (to be made available soon) and was presented at the last
Linaro Connect in HK. Given that the CPU power is usually controlled by
the secure side, we'll ask for an SMC to be issued for waking up
secondary CPUs, so it's up to the secure firmware to write the correct
hardware registers.
Thanks for the information. SMC standardization would indeed help
to overcome some of these. Will wait for that information before
next set of questions.
Yes please. If the SMC is not standardized for most calls at least,
we'll end up with a horrible mess of SoC specific calls like we
currently have. Related to that, the virtualization calls should be
also standardized so we don't end up with multiple different hypervisors
with different calls.
What good is the run-time BUG() here? Nothing should be calling these
when CONFIG_COMPAT is disabled, so I think you should just remove
the #ifdef around the declarations, and the entire #else case.
They are called from handle_signal(), so that's to avoid #ifdef inside
functions. I can drop the BUG() (but keep the empty function) and
change the checks to is_compat_task() so that the compiler optimises the
condition out when !COMPAT.
--
Catalin
What good is the run-time BUG() here? Nothing should be calling these
when CONFIG_COMPAT is disabled, so I think you should just remove
the #ifdef around the declarations, and the entire #else case.
They are called from handle_signal(), so that's to avoid #ifdef inside
functions. I can drop the BUG() (but keep the empty function) and
change the checks to is_compat_task() so that the compiler optimises the
condition out when !COMPAT.
Sounds good. Note that you can turn a lot of #ifdef into
if(IS_ENABLED(CONFIG_FOO)) as well, even if there is no other runtime
check for them.
Arnd
+asmlinkage int compat_sys_personality(compat_ulong_t personality)
+{
+ int ret;
+
+ if (personality(current->personality) == PER_LINUX32 &&
+ personality == PER_LINUX)
+ personality = PER_LINUX32;
+ ret = sys_personality(personality);
+ if (ret == PER_LINUX32)
+ ret = PER_LINUX;
+ return ret;
+}
Where did you get this from?
You should not need compat_sys_personality, just call the native function.
Hmm, but in that case an aarch32 application doing a personality(PER_LINUX)
syscall will start seeing the wrong uname.
Coming back at this topic, I noticed another issue. Jiri Kosina
has recently posted patches to fix this function in the other architectures
Yeah, there were quite a few broken ones, some of them since the beginning
of time.
in order to mask out the other personality bits, which is a correct fix,
but the above function is odd for other reasons.
* On MIPS, it is used only for compat tasks, like you have it above.
* On PA-RISC, it is used for native 32 bit tasks and for compat 32 bit tasks,
but not for native 64 bit ones.
* On IA64, it was used for compat tasks (support for which has since
been removed from the kernel), plus all 32 bit tasks would start with
PER_LINUX32.
* On PowerPC, Sparc and s390, it is used for native 64 bit tasks and for
compat 32 bit tasks, but not for native 32 bit ones.
* On Tile, it was never used.
* On x86_64, it used to be defined (copied from ia64) but not used
throughout the git history.
The semantics of the function are also interesting: The intention seems
to be that to a compat task, PER_LINUX32 would appear as PER_LINUX.
The effect is that any process can set PER_LINUX32 but it can never
be unset except by a 64 bit MIPS or PA-RISC task.
Since x86_64 does not implement this behavior at all, I suspect that
there are now lots of things depending on not having it, while all
the other architectures might also have some (even predating the
x86_64 port) use cases that depend on depend on not being able to
observe PER_LINUX32 in 32 bit compat tasks.
I think we should try to agree on how this is all supposed to work
and use common code, either put the ppc/sparc/s390 version into
sys_personality, or remove all of them and just do what x86 and tile
do, using the regular sys_personality for all tasks.
How about rather introducing common compat_sys_personality() and switching
the archs that are using it to it? Unifying the behavior (PER_LINUX /
PER_LINUX32 masquerading) should be painless.
Thanks,
--
Jiri Kosina
SUSE Labs
Hi Arnd,
On Wed, Aug 22, 2012 at 06:13:10PM +0100, Catalin Marinas wrote:
On Wed, Aug 22, 2012 at 01:27:14PM +0100, Arnd Bergmann wrote:
quoted
On Wednesday 22 August 2012, Catalin Marinas wrote:
quoted
But what's more important - moving this wrapper to glibc causes issues
with the page size. We support both 4KB and 64KB pages on 64-bit systems
(the latter without compat support). The kernel is in a better position
to do the shift by a compile-time constant. Glibc would need to enquire
the actual page size to do the shift before calling sys_mmap_pgoff. If
we assume in glibc that the shift is always 12, we need another wrapper
in the kernel anyway for 64KB page configuration. So passing the offset
in bytes worked best for us.
Right, the kernel interface should really be independent of the page
size, as sys_mmap2 normally is, and sys_mmap2 is not provided here.
sys_mmap2 is indeed independent of the page size on most architectures
assuming that the last argument represents the offset in units of 4096.
The cris and ia64 seem to differ (one being 8K, the other variable).
sys_mmap is also independent of the page size.
But using sys_mmap2 for a 64-bit architecture, especially when the page
size is not always 4K, does not bring any advantages. We end up doing a
shift by 12 in glibc and another shift by (PAGE_SHIFT - 12) in the
kernel wrapper. Unless I missed your point, I don't see the reason for
using sys_mmap2 on a 64-bit architecture, apart from it being newer (and
compat support should not have any relevance, we have different syscall
tables anyway).
I forgot about this at the KS and we haven't got to a clear conclusion.
Do we (1) stick with the sys_mmap() for 64-bit systems and avoid offset
conversion in both glibc and kernel or (2) use sys_mmap2() with a 12
shift in glibc and (PAGE_SHIFT - 12) in the kernel wrapper?
I personally prefer (1) as it doesn't require a kernel wrapper and we
avoid the double shifting. A reason for (2) would be if we ever need
file offsets greater than 16EB.
--
Catalin
On Monday 03 September 2012, Catalin Marinas wrote:
I forgot about this at the KS and we haven't got to a clear conclusion.
Do we (1) stick with the sys_mmap() for 64-bit systems and avoid offset
conversion in both glibc and kernel or (2) use sys_mmap2() with a 12
shift in glibc and (PAGE_SHIFT - 12) in the kernel wrapper?
I personally prefer (1) as it doesn't require a kernel wrapper and we
avoid the double shifting.
Yes, I think it's ok this way.
A reason for (2) would be if we ever need file offsets greater than 16EB.
Let's not worry about this for now, all the other architectures will
have the same problem when we get there.
Arnd
On Wed, Aug 15, 2012 at 03:49:54PM +0100, Arnd Bergmann wrote:
On Tuesday 14 August 2012, Catalin Marinas wrote:
quoted
+/*
+ * Single-value transfer routines. They automatically use the right
+ * size if we just have the right pointer type. Note that the functions
+ * which read from user space (*get_*) need to take care not to leak
+ * kernel data even if the calling code is buggy and fails to check
+ * the return value. This means zeroing out the destination variable
+ * or buffer on error. Normally this is done out of line by the
+ * fixup code, but there are a few places where it intrudes on the
+ * main code path. When we only write to user space, there is no
+ * problem.
+ */
+extern long __get_user_1(void *);
+extern long __get_user_2(void *);
+extern long __get_user_4(void *);
+extern long __get_user_8(void *);
+
+#define __get_user_x(__r2,__p,__e,__s,__i...) \
+ asm volatile( \
+ __asmeq("%0", "x0") __asmeq("%1", "x2") \
+ "bl __get_user_" #__s \
+ : "=&r" (__e), "=r" (__r2) \
+ : "0" (__p) \
+ : __i, "cc")
+
+#define get_user(x,p) \
+ ({ \
+ register const typeof(*(p)) __user *__p asm("x0") = (p);\
+ register unsigned long __r2 asm("x2"); \
+ register long __e asm("x0"); \
+ switch (sizeof(*(__p))) { \
+ case 1: \
+ __get_user_x(__r2, __p, __e, 1, "x30"); \
+ break; \
+ case 2: \
+ __get_user_x(__r2, __p, __e, 2, "x3", "x30"); \
+ break; \
+ case 4: \
+ __get_user_x(__r2, __p, __e, 4, "x30"); \
+ break; \
+ case 8: \
+ __get_user_x(__r2, __p, __e, 8, "x30"); \
+ break; \
+ default: __e = __get_user_bad(); break; \
+ } \
+ x = (typeof(*(p))) __r2; \
+ __e; \
+ })
It's fairly unusual to have out of line get_user/put_user functions.
What is the reason for this, other than copying from ARM?
I changed these to inline asm. The only reason which I don't think
matters much in this case is a few KB increase in Image size.
--
Catalin
From: Russell King - ARM Linux <hidden> Date: 2012-09-05 19:13:30
On Wed, Aug 15, 2012 at 02:49:54PM +0000, Arnd Bergmann wrote:
It's fairly unusual to have out of line get_user/put_user functions.
What is the reason for this, other than copying from ARM?
Actually, we never used to out of line on ARM, and then I experimented,
and found there was a net benefit - not only in code size but also
there appeared to be a performance benefit by out of lining them.
From: Chris Metcalf <hidden> Date: 2012-09-05 19:56:31
On 8/21/2012 4:17 PM, Arnd Bergmann wrote:
On Tuesday 21 August 2012, Catalin Marinas wrote:
quoted
On Thu, Aug 16, 2012 at 01:37:53PM +0100, Arnd Bergmann wrote:
quoted
No, the uname output is meant to tell you about the system, not the
instruction set that you are using (you already know that in compiled
code).
OK, so we assumed that compat tasks should get a uname as close as
possible to a 32-bit system, i.e. armv8l, for full compatibility. This
would allow us to run something like 32-bit Debian on an AArch64 kernel
without worrying about any scripts failing.
You can still do that, just boot with init="/sbin/setarch armv7 /sbin/init".
quoted
But I can see on x86 that it always reports x86_64 even if the task is
x86_32.
Not just x86, the same behavior is used on powerpc, s390, mips, sparc and
parisc. Not sure about tile though.
tile also reports "tilegx" regardless of whether the task is 64-bit or
32-bit compat.
--
Chris Metcalf, Tilera Corp.
http://www.tilera.com
On Wed, Sep 05, 2012 at 08:13:12PM +0100, Russell King - ARM Linux wrote:
On Wed, Aug 15, 2012 at 02:49:54PM +0000, Arnd Bergmann wrote:
quoted
It's fairly unusual to have out of line get_user/put_user functions.
What is the reason for this, other than copying from ARM?
Actually, we never used to out of line on ARM, and then I experimented,
and found there was a net benefit - not only in code size but also
there appeared to be a performance benefit by out of lining them.
There are indeed a few KB gain in code size but that's probably coming
from the exception table since otherwise you just replace a bl with
ldrt. It depends on what the compiler does as well, the arm code has
some carefully chosen registers when calling the __get_user_x function.
If you do the access_ok inline and the __get_user_x separately, the size
increase is even greater (at least in the arm64 case it can get to over
20KB). I think x86 does the access_ok check out of line.
For now I changed the arm64 code to inline as most of the other
architectures but I'll revisit this when I get some hardware. Another
thing to be optimised is the size of the exception table entry. I use
two .quad declarations for the fault address and branch but I guess they
could be made relative to PAGE_OFFSET and only use .long (or some
prel31).
--
Catalin
From: Russell King - ARM Linux <hidden> Date: 2012-09-05 21:05:46
On Wed, Sep 05, 2012 at 10:01:37PM +0100, Catalin Marinas wrote:
There are indeed a few KB gain in code size but that's probably coming
from the exception table since otherwise you just replace a bl with
ldrt. It depends on what the compiler does as well, the arm code has
some carefully chosen registers when calling the __get_user_x function.
It's more than that - it's not just the ldr but also a zeroing of a
temporary register to hold the error code should the instruction fault.
So it's not only the exception tables but also an increase in the
main path - and that's where you benefit from having it out of line and
thereby a hotter i-cache.
If you do the access_ok inline and the __get_user_x separately, the size
increase is even greater (at least in the arm64 case it can get to over
20KB). I think x86 does the access_ok check out of line.
Please talk to Will about get_user() and put_user(). Afterwards you
will definitely want to keep them out of line on 64-bit ARM.
On Wed, Sep 05, 2012 at 10:05:34PM +0100, Russell King - ARM Linux wrote:
On Wed, Sep 05, 2012 at 10:01:37PM +0100, Catalin Marinas wrote:
quoted
There are indeed a few KB gain in code size but that's probably coming
from the exception table since otherwise you just replace a bl with
ldrt. It depends on what the compiler does as well, the arm code has
some carefully chosen registers when calling the __get_user_x function.
It's more than that - it's not just the ldr but also a zeroing of a
temporary register to hold the error code should the instruction fault.
So it's not only the exception tables but also an increase in the
main path - and that's where you benefit from having it out of line and
thereby a hotter i-cache.
On 32-bit we have __get_user() inline and get_user() out of line. What
was the history behind this?
quoted
If you do the access_ok inline and the __get_user_x separately, the size
increase is even greater (at least in the arm64 case it can get to over
20KB). I think x86 does the access_ok check out of line.
Please talk to Will about get_user() and put_user(). Afterwards you
will definitely want to keep them out of line on 64-bit ARM.
As I said, I already made the change to always inline get_user/put_user
with some penalty in the Image size but it makes the code cleaner. I'm
not entirely convinced of the performance gain/loss especially on ARMv8
cores with physically tagged caches. There is room for optimisation when
I get real silicon.
--
Catalin
(revisiting unanswered emails :))
On Wed, Aug 15, 2012 at 01:33:55AM +0100, Olof Johansson wrote:
On Tue, Aug 14, 2012 at 06:52:14PM +0100, Catalin Marinas wrote:
quoted
+/*
+ * I/O port access primitives.
+ */
+#define IO_SPACE_LIMIT 0xffff
+
+/*
+ * We currently don't have any platform with PCI support, so just leave this
+ * defined to 0 until needed.
+ */
+#define PCI_IOBASE ((void __iomem *)0)
You could just leave out the PCI / I/O code alltogether instead.
I would leave this in as some of the first platforms to appear will have
PCIe. At some point we'll add a fixed address where the PCI_IOBASE is
mapped.
--
Catalin
On Friday 14 September 2012, Catalin Marinas wrote:
(revisiting unanswered emails :))
On Wed, Aug 15, 2012 at 01:33:55AM +0100, Olof Johansson wrote:
quoted
On Tue, Aug 14, 2012 at 06:52:14PM +0100, Catalin Marinas wrote:
quoted
+/*
+ * I/O port access primitives.
+ */
+#define IO_SPACE_LIMIT 0xffff
+
+/*
+ * We currently don't have any platform with PCI support, so just leave this
+ * defined to 0 until needed.
+ */
+#define PCI_IOBASE ((void __iomem *)0)
You could just leave out the PCI / I/O code alltogether instead.
I would leave this in as some of the first platforms to appear will have
PCIe. At some point we'll add a fixed address where the PCI_IOBASE is
mapped.
I guess the cleanest way would be to reserve a virtual memory region right away
and document it in the file where you describe the memory layout. Then you can
fill the value in here.
Arnd
Does it really make sense to export these to modules?
Changed them to _GPL now but we may still want them exported to modules.
Who knows, we may just have the SoC code in a loadable module (from
initramfs).
I find the powerpc way of having a machine descriptor structure with these
(and other) function pointers in it a bit cleaner, since it gives you
one place to plug it all in. I'd recommend that you consider doing that
here as well, for these three and potentially other cases in the future.
(See arch/powerpc/include/asm/machdep.h, struct machdep_calls).
At some point we may add them but currently I want the SoC code to be
minimal.
--
Catalin
On Wed, Aug 15, 2012 at 01:10:43AM +0100, Olof Johansson wrote:
On Tue, Aug 14, 2012 at 06:52:09PM +0100, Catalin Marinas wrote:
quoted
+#ifndef __ASM_CPUTYPE_H
+#define __ASM_CPUTYPE_H
+
+#define ID_MIDR_EL1 "midr_el1"
+#define ID_CTR_EL0 "ctr_el0"
+
+#define ID_AA64PFR0_EL1 "id_aa64pfr0_el1"
+#define ID_AA64DFR0_EL1 "id_aa64dfr0_el1"
+#define ID_AA64AFR0_EL1 "id_aa64afr0_el1"
+#define ID_AA64ISAR0_EL1 "id_aa64isar0_el1"
+#define ID_AA64MMFR0_EL1 "id_aa64mmfr0_el1"
+
+#define read_cpuid(reg) ({ \
+ u64 __val; \
+ asm("mrs %0, " reg : "=r" (__val)); \
+ __val; \
+})
+
+/*
+ * The CPU ID never changes at run time, so we might as well tell the
+ * compiler that it's constant. Use this function to read the CPU ID
+ * rather than directly reading processor_id or read_cpuid() directly.
+ */
+static inline u32 __attribute_const__ read_cpuid_id(void)
+{
+ return read_cpuid(ID_MIDR_EL1);
+}
+
+static inline u32 __attribute_const__ read_cpuid_cachetype(void)
+{
+ return read_cpuid(ID_CTR_EL0);
+}
Is this perhaps a carry-over from arch/arm? Abstracting out read_cpuid()
doesn't seem to buy anything here, just opencode the one-line assembly
in each.
read_cpuid() is called from several other files under arch/arm64 and
also used in expressions, so it's a good abstraction.
I know this is a carry-over from arch/arm, but how about moving this
to more of a C construct similar to arch/powerpc/kernel/cputable.c
instead? It's considerably easier to read that way, and it's convenient
to have the definitions all in one place, making it easier to share some
of the functions, etc.
Done in version 3. It's easier to read :).
--
Catalin
On Fri, Sep 14, 2012 at 06:31:59PM +0100, Arnd Bergmann wrote:
On Friday 14 September 2012, Catalin Marinas wrote:
quoted
(revisiting unanswered emails :))
On Wed, Aug 15, 2012 at 01:33:55AM +0100, Olof Johansson wrote:
quoted
On Tue, Aug 14, 2012 at 06:52:14PM +0100, Catalin Marinas wrote:
quoted
+/*
+ * I/O port access primitives.
+ */
+#define IO_SPACE_LIMIT 0xffff
+
+/*
+ * We currently don't have any platform with PCI support, so just leave this
+ * defined to 0 until needed.
+ */
+#define PCI_IOBASE ((void __iomem *)0)
You could just leave out the PCI / I/O code alltogether instead.
I would leave this in as some of the first platforms to appear will have
PCIe. At some point we'll add a fixed address where the PCI_IOBASE is
mapped.
I guess the cleanest way would be to reserve a virtual memory region right away
and document it in the file where you describe the memory layout. Then you can
fill the value in here.
Yes, easy to do. Any access will fault until we add the PCI support.
--
Catalin
From: Olof Johansson <hidden> Date: 2012-09-16 00:28:25
On Fri, Sep 14, 2012 at 06:39:46PM +0100, Catalin Marinas wrote:
On Fri, Sep 14, 2012 at 06:31:59PM +0100, Arnd Bergmann wrote:
quoted
On Friday 14 September 2012, Catalin Marinas wrote:
quoted
(revisiting unanswered emails :))
On Wed, Aug 15, 2012 at 01:33:55AM +0100, Olof Johansson wrote:
quoted
On Tue, Aug 14, 2012 at 06:52:14PM +0100, Catalin Marinas wrote:
quoted
+/*
+ * I/O port access primitives.
+ */
+#define IO_SPACE_LIMIT 0xffff
+
+/*
+ * We currently don't have any platform with PCI support, so just leave this
+ * defined to 0 until needed.
+ */
+#define PCI_IOBASE ((void __iomem *)0)
You could just leave out the PCI / I/O code alltogether instead.
I would leave this in as some of the first platforms to appear will have
PCIe. At some point we'll add a fixed address where the PCI_IOBASE is
mapped.
I guess the cleanest way would be to reserve a virtual memory region right away
and document it in the file where you describe the memory layout. Then you can
fill the value in here.
Yes, easy to do. Any access will fault until we add the PCI support.
Does it really make sense to export these to modules?
Changed them to _GPL now but we may still want them exported to modules.
Who knows, we may just have the SoC code in a loadable module (from
initramfs).
Ok, keeping the option open for that makes some sense.
quoted
I find the powerpc way of having a machine descriptor structure with these
(and other) function pointers in it a bit cleaner, since it gives you
one place to plug it all in. I'd recommend that you consider doing that
here as well, for these three and potentially other cases in the future.
(See arch/powerpc/include/asm/machdep.h, struct machdep_calls).
At some point we may add them but currently I want the SoC code to be
minimal.
It's not about the size of the code, it's about having one place where
the SoC divergence is exposed instead of adding hooks here and there. But
agreed, it could be moved to over time.
-Olof