Re: [PATCH] powerpc/pci: Fix IO space breakage after of_pci_range_to_resource() change
(hope my email makes it everywhere, using a webmail client at the moment as I'm at plumbersconf Michael Ellerman hat am 16. Oktober 2014 um 05:22 geschrieben: > > > Commit 0b0b0893d49b "of/pci: Fix the conversion of IO ranges into IO > resources" changed the behaviour of of_pci_range_to_resource(). I just looked at this after benh mentioned the problem on IRC, here's a log dump :26 AMargh 9:27 AMthe whole ARM OF PCI rework seems to completely break PIO on powerpc 9:30 AM → willy joined (^willy@62.156.150.204) 9:35 AMand reverting it would mean reverting all of ARM new PCI stuff 9:35 AMcrap 9:35 AMthat business with IO space allocation taking over our code without understanding what it does 9:35 AMyuck 9:41 AM → markf , olaf , sarnold , gospo , cmarinas , Mahesh1 , joern , clark_ and benhjoined ⇐ gcl and clark quit ↔ willy , jbarnes , jbrandeb_ and Mahesh popped in ↔ sameo , jbrandeb , steved and jj nipped out • srikar → srikar_away , raghu → raghu_away Thursday, October 16th, 2014 12:06 AM → fweisbec , Mahesh , kamalesh , heiko , olaf , riel and willy joined ⇐ shaggy , sammj , sameo , clark_ , lenb and jj quit ↔ jbrandeb , cdub , Mahesh1 and gcl popped in ↔ jbrandeb_ , benh, joern and BenC nipped out • mpe|away → mpe|away , raghu_away → raghu , srikar_away → srikar 10:16 AMbenh: is it the of_pci_range_to_resource change? 10:17 AMthe new pci_ioremap_iospace logic should not get used on powerpc at all, so I didn't expect any breakage 10:18 AMI wasn't too happy with all the details of Liviu's series, bit in the end it seemed reasonable enough 10:20 AMhe really wanted to use the pci_address_to_pio code from powerpc and in the end I stopped complaining 10:21 AMthe new code can do a few things that simpler versions could not, e.g. handling multiple host bridges getting registered when they have the same I/O space window 10:23 AM → jj joined (^j...@static-50-53-60-87.bvtn.or.frontiernet.net) 10:33 AMbenh: I can see how it breaks your pci_process_bridge_OF_ranges, we had the same problem in some of the ARM platforms and Liviu fixed those but apparently didn't realize he had to change the ppc implementation (and get your ack) too 10:35 AMthe good news is that it should in fact simplify your code to fix it, but the fact that this bug got into the kernel in the first place is extremely annoying 10:39 AMbenh: the fixup that is done in your pcibios_reserve_legacy_regions is now already performed in of_pci_range_to_resource 10:39 AMwe had duplicated the same thing in each pci host driver (and they all got it wrong), so the intent was to move it into a common place 10:40 AMbut of course it's a bug to do it twice 10:43 AMpci_register_io_range is trying to do a more generalized version of how you assign hose->io_base_virt, you should probably override that to keep the current behavior 10:45 AMpcibios_map_phb_io_space I mean, for ppc64 10:52 AM → cmarinas joined (~cmari...@fw-tnat.cambridge.arm.com) 10:53 AMbenh: for 3.18, the best approach is likely to #ifdef <%23ifdef> PCI_IOBASE the changes in of_pci_range_to_resource 10:54 AMI suspect you are fine with effectively reverting Liviu's changes that way, and you can decide whether or not you want to later make the powerpc code use the common logic 11:01 AM → cdub and sarnold joined ⇐ cmarinas quit 11:28 AMarnd_: can you shoot the above in an email CCed to mpe ? 11:28 AMarnd_: he did a band aid that works 11:28 AMarnd_: and see the comment I made today about using his stuff if I can specify where I want the IO ranges 11:28 AMarnd_: I want to keep the way I do the layout on ppc64 > Previously it simply populated the resource based on the arguments. Now > it calls pci_register_io_range() and pci_address_to_pio(). These both > have two implementations depending on whether PCI_IOBASE is defined, > which it is not for powerpc. > > Further complicating matters, both routines are weak, and powerpc > implements it's own version of one - pci_address_to_pio(). However > powerpc's implementation depends on other initialisations which are done > later in boot. Right, sorry for missing this during the last review of the broken patches. > The end result is incorrectly initialised IO space. Often we can get > away with that, because we don't make much use of IO space. However > virtio requires it, so we see eg: > > pci_bus :00: root bus resource [io 0x] (bus address > [0x-0x]) > PCI: Cannot allocate resource region 0 of device :00:01.0, will remap > virtio-pci :00:01.0: can't enable device: BAR 0 [io size 0x0020] not > assigned > > The simplest fix for now is to just stop using of_pci_range_to_resource(), > and open-code the original imp`lementation, that's all we want it to do. The same bug is likely to
Re: [PATCH/RFC 0/4] Probe deferral for IOMMU DT integration
Laura Abbott hat am 6. Februar 2015 um 01:31 geschrieben: > > The requirement for this is based on a previous patch to add clock > support to the ARM SMMU driver[2]. Once we have clock support, it's > possible that the driver itself may need to be defered which breaks > a bunch of assumptions about how SMMU probing is supposed to work. Hi Laura, I was hoping that we would not need this, and instead treat the iommu in the same way as timers and SMP initialization, both of which need to be run early at boot time but may rely on clock controllers to be initialized first. Is there a specific requirement that makes this impossible here, or is your intention to solve the problem more nicely by allowing deferred probing over forcing the input clocks of the iommu to be early? Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [musl] Re: [PATCHv3 00/24] ILP32 support in ARM64
> Szabolcs Nagy hat am 11. Februar 2015 um 20:05 geschrieben:
> * Catalin Marinas [2015-02-11 17:39:19 +]:
> > (adding Marcus)
> >
> > On Tue, Feb 10, 2015 at 06:13:02PM +, Rich Felker wrote:
> > > I don't know if this has been discussed on libc-alpha yet or not, but
> > > I think we need to open a discussion of how it relates to open glibc
> > > bug #16437, which presently applies only to x32 (ILP32 ABI on x86_64):
> > >
> > > https://sourceware.org/bugzilla/show_bug.cgi?id=16437
> ...
> > So w.r.t. C11, the exported kernel timespec looks fine. But I think the
> > x32 kernel support (and the current ILP32 patches) assume a native
> > struct timespec with tv_nsec as 64-bit.
> >
> > If we are to be C11 conformant, glibc on x32 has a bug as it defines
> > timespec incorrectly. This hid a bug in the kernel handling the
> > corresponding x32 syscalls. What's the best fix for x32 I can't really
> > tell (we need people to agree on where the bugs are).
> >
> > At least for AArch64 ILP32 we are still free to change the user/kernel
> > ABI, so we could add wrappers for the affected syscalls to fix this up.
> >
>
> yes, afaik on x32 the 64bit kernel expects 64bit layout,
> arm64 can fix this
We have to fix it on all 32-bit architectures when we move to 64-bit time_t.
I think ideally you'd want a user space definition like
typedef long long time_t;
struct timespec {
time_t tv_sec;
long long tv_nsec;
};
which is the only way to avoid passing uninitialized tv_nsec into the kernel
from arbitrary user space doing ioctl. This is of course against POSIX and
C99. Changing POSIX to allow it is probably easier than the C standard,
but we have a couple of years before we need to make this the default.
In the kernel headers, the current plan is to provide interfaces taking
structures
typedef long long __kernel_time64_t;
struct __kernel_timespec64_t {
__kernel_time64_t tv_sec;
long long tv_nsec;
};
at least for ioctls, to avoid the ambiguity with libc headers specifying
something else.
A libc could use other definitions with added padding for what it exports
to user space though, or even make this depend on compile-time flags to
determine whether to make tv_nsec as long long, or to insert padding
in the right place based on endianess.
> > > While most of the other type changes proposed (I'm looking at
> > > https://lkml.org/lkml/2014/9/3/719) are permissible and simply
> > > ugly/undesirable,
> >
> > They may be ugly but definitely not undesirable ;).
> >
>
> several types have the same c level definition across all archs..
> except x32 because of
>
> typedef long long __kernel_long_t;
>
> this should not cause posix/c conformance issues (as you noted
> timespec is ok in the uapi header only the kernel side behaviour
> is wrong)
The kernel behavior is right for the syscalls except ioctl, the uapi
header is wrong (not according to C99, but according to common sense)
and needs to be changed in order to work with big-endian. For x32,
I wonder if we can just #define timespec as __kernel_timespec64
once we introduce that.
> > > Working around the discrepencies in userspace IS possible, but ugly.
> > > We do it in musl libc for x32 right now -- see:
> > >
> > > http://git.musl-libc.org/cgit/musl/tree/arch/x32/syscall_arch.h?id=v1.1.6
> > > http://git.musl-libc.org/cgit/musl/tree/arch/x32/src/syscall_cp_fixup.c?id=v1.1.6
> >
> > For AArch64 ILP32 I would rather see the fix-ups in kernel wrappers.
> >
> > Are you aware of other cases like this?
> >
>
> i know at least one android kernel issue: there is an ioctl for the
> alarm device that takes timespec argument
>
> (i think it's not in the mainline kernel and i guess android does
> not care about x32 so it was not an issue so far, but this is something
> that should not be fixed on the libc side)
ioctl is a known problem with x32 and the ARM ILP32 support. This is
not limited to timespec but to any driver that uses an ioctl with
a data structure that includes a __kernel_long_t or __kernel_ulong_t
argument. There are a couple of drivers using these, and we either
need to change the structures to use 'long' instead, or fix the driver
to be aware of the difference between old-style 32-bit compat and
x32-style compat ioctl handling.
The types affected by this are
include/uapi/asm-generic/posix_types.h:typedef __kernel_ulong_t __kernel_ino_t;
include/uapi/asm-generic/posix_types.h:typedef __kernel_ulong_t __kernel_size_t;
include/uapi/asm-generic/posix_types.h:typedef __kernel_long_t
__kernel_suseconds_t;
include/uapi/asm-generic/posix_types.h:typedef __kernel_long_t
__kernel_ssize_t;
include/uapi/asm-generic/posix_types.h:typedef __kernel_long_t
__kernel_ptrdiff_t;
include/uapi/asm-generic/posix_types.h:typedef __kernel_long_t __kernel_off_t;
include/uapi/asm-generic/posix_types.h:typedef __kernel_long_t __kernel_time_t;
include/uapi/asm-generic/posix_types.h:typedef __kernel_long_t
__kernel_clock_t;
and anything derived
Re: [musl] Re: [PATCHv3 00/24] ILP32 support in ARM64
Sorry about the HTML mail, I'm currently travelling without access to my regular
mail client.
> "a...@arndb.de" hat am 11. Februar 2015 um 22:02 geschrieben:
>
> Rich Felker hat am 11. Februar 2015 um 21:12 geschrieben:
> > On Wed, Feb 11, 2015 at 08:50:06PM +0100, a...@arndb.de wrote:
> > > > > At least for AArch64 ILP32 we are still free to change the
> > > > > user/kernel
> > > > > ABI, so we could add wrappers for the affected syscalls to fix this
> > > > > up.
> > > > yes, afaik on x32 the 64bit kernel expects 64bit layout,
> > > > arm64 can fix this
> > >
> > > We have to fix it on all 32-bit architectures when we move to 64-bit
> > > time_t.
> > >
> > > I think ideally you'd want a user space definition like
> > >
> > > typedef long long time_t;
> > > struct timespec {
> > > time_t tv_sec;
> > > long long tv_nsec;
> > > };
> > >
> > > which is the only way to avoid passing uninitialized tv_nsec into the
> > > kernel
> > > from arbitrary user space doing ioctl. This is of course against POSIX
> > > and
> > > C99. Changing POSIX to allow it is probably easier than the C standard,
> > > but we have a couple of years before we need to make this the default.
> >
> > I don't see why you want it to be long long. There is no harm in
> > passing uninitialized padding to the kernel; the kernel just needs to
> > do the right thing and ignore it (or avoid reading it to begin with).
>
> This would however mean having three different implementations
> in the kernel rather than just two: Every driver that can pass a timespec
> with this model needs to handle the native 64-bit case (64/64), the legacy
> 32-bit case (32/32) and the y2038-safe case (64/32). Most code can
> already handle the first two, and none today handles the third. If you
> want to make the handling explicitly incompatible with native 64-bit
> mode, you get a lot of untested code in obscure places that are never
> tested properly, while using the normal behavior in the kernel at least
> gives us the same bugs that we already have on native 64-bit systems.
>
> In some cases, there may also be a measurable performance penalty
> in interpreting a user space data structure manually over copying
> it (including the timespec values) in one chunk.
>
> An alternative would be to change the native 64-bit case to ignore the upper
> half of tv_nsec and always just copy the low bits. This should work
> fine almost all of the time, but I fear that there might be corner cases
> where existing 64-bit user space depends on passing large or negative
> tv_nsec values into the kernel.
>
> > The other direction, passing uninitialized data from the kernel to
> > userspace, would be dangerous. But it doesn't happen as long as the
> > userspace padding is positioned (in an endian-dependent manner) where
> > the high bits of the kernel type would lie. It could happen if you
> > used a separate conversion wrapper that ony wrote 32 bits, but if you
> > wanted to take that approach you'd just need the wrapper to also write
> > the padding field manually.
>
> Going from kernel to user space should not be an issue as long as we
> always just write two 64-bit words, and this will zero-fill the upper half.
>
> > > In the kernel headers, the current plan is to provide interfaces taking
> > > structures
> > >
> > > typedef long long __kernel_time64_t;
> > > struct __kernel_timespec64_t {
> > > __kernel_time64_t tv_sec;
> > > long long tv_nsec;
> > > };
> > >
> > > at least for ioctls, to avoid the ambiguity with libc headers specifying
> > > something else.
> >
> > This seems hideous from an application standpoint. Application
> > programmers don't want to know, and shouldn't need to know, these
> > silly implementation details that make no sense except as historical
> > baggage. They should just be able to use "struct timespec" everywhere
> > and have it work.
> The kernel does not even know how timespec is defined by libc, and we have
> to at least be able to handle the common cases of timespec being 32/32
> and 64/64 (or 64/32 plus explicit padding). For system calls, we can rely
> on libc calling the syscalls that match the definition (or convert the
> structure as necessary), while for ioctl the command number is chosen
> by the application and has to match the structure
Re: [PATCHv3 00/24] ILP32 support in ARM64
> Catalin Marinas hat am 12. Februar 2015 um 19:17
> geschrieben:
> On Wed, Feb 11, 2015 at 02:21:18PM -0500, Rich Felker wrote:
> > On Wed, Feb 11, 2015 at 05:39:19PM +, Catalin Marinas wrote:
> > > On Tue, Feb 10, 2015 at 06:13:02PM +, Rich Felker wrote:
> > > > I don't know if this has been discussed on libc-alpha yet or not, but
> > > > I think we need to open a discussion of how it relates to open glibc
> > > > bug #16437, which presently applies only to x32 (ILP32 ABI on x86_64):
> > > >
> > > > https://sourceware.org/bugzilla/show_bug.cgi?id=16437
> > >
> > > I'm trying to understand the problem first. Quoting from the bug above
> > > (which I guess is quoted form C11):
> > >
> > > "The range and precision of times representable in clock_t and time_t
> > > are implementation-defined. The timespec structure shall contain at
> > > least the following members, in any order.
> > >
> > > time_t tv_sec; // whole seconds -- >= 0
> > > long tv_nsec; // nanoseconds -- [0, 9]"
> > >
> > > So changing time_t to 64-bit is fine on x32. The timespec struct
> > > exported by the kernel always uses a long for tv_nsec. However, glibc uses
> > > __syscall_slong_t which ends up as 64-bit for x32 (I guess it mirrors
> > > the __kernel_long_t definition).
> > >
> > > So w.r.t. C11, the exported kernel timespec looks fine. But I think the
> > > x32 kernel support (and the current ILP32 patches) assume a native
> > > struct timespec with tv_nsec as 64-bit.
> >
> > The exported kernel timespec is not fine if long is defined as a
> > 32-bit type, which it is for x32 and the proposed aarch64-ILP32 ABIs.
>
> The exported kernel headers comply with POSIX as they use long for
> tv_nsec. The exported headers can be used in user space and with an
> ILP32 ABI, long is 32-bit. The problem is the syscall handler which uses
> the same structure in kernel where long is 64-bit. But this doesn't
> change the fact that the exported header was still correct from a user
> perspective.
This is not ILP32 specific really, we need to add the same set of syscalls
for all 32-bit systems, in addition to the existing ones that take
a 32-bit time_t.
> The solution (for new ports) could be similar to the other such
> solutions in the compat layer. A kernel internal structure which is
> binary-compatible with the ILP32 user one (as exported by the kernel):
>
> struct ilp32_timespec_kernel_internal_only {
> __kernel_time_t tv_sec; /* seconds */
> int tv_nsec; /* nanoseconds */
> };
>
> and a syscall wrapper which converts between ilp32_timespec and timespec
> (take compat_sys_clock_settime as an example).
We then have to to this on all architectures, and not call it ilp32_timespec,
but call it something else.
I would much prefer to only have two versions of each syscall that takes a
timespec rather than three versions, or having a version that behaves
differently based on the type of program calling it. On native 32-bit
systems, we should have the native syscall taking the 16-byte structure
(using long long __kernel_time64_t) along with the compatibility syscall
with a 8-byte structure for existing applications.
On 64-bit systems, the same syscall source can be used for the normal 16-byte
structure on native 64-bit tasks, ilp32 tasks (x32, aarch64-32), and future
compat32 (i386, aarch32, ...) tasks, while the syscall for the 8-byte structure
deals with legacy compat32 tasks that do not yet use __kernel_time64_t.
> If the user structure has some padding (and as I've read in this thread
> it is allowed), it could be even easier for the kernel. The padding
> could be 32-bit before or after tv_nsec, depending on endianness.
The problem as pointed out before is that if you do this, 32-bit tasks
need to have the padding word zeroed at some stage for data passed into
the kernel, while 64-bit tasks need to return an error if the upper half
of the tv_nsec word is nonzero, at least for interfaces that are documented
to do this.
This can be done either in the kernel or in the libc. In the kernel, it
comes down to a function like
int get_user_timespec64(struct timespec64 *ts, struct __kernel_timespec64 __user
*uts, bool task_32bit)
{
struct __kernel_timespec64 input;
if (copy_from_user(&input, uts, sizeof(input))
return -EFAULT;
ts->tv_sec = input.tv_sec;
if (task_32bit)
ts->tv_nsec = (int)input.tv_nsec;
else
ts->tv_nsec = input.tv_nsec;
return 0;
}
with data types of
struct timespec64 {
time64_t tv_sec;
long tv_nsec;
};
struct __kernel_timespec64 {
__kernel_time64_t tv_nsec;
#if (__BYTE_ORDER == __BIG_ENDIAN) && (__BITS_PER_LONG == 32)
u32 __pad;
#endif
long tv_nsec;
#if (__BYTE_ORDER == __LITTLE_ENDIAN) && (__BITS_PER_LONG == 32)
u32 __pad;
#endif
};
The data structure definition is a little bit fragile, as it depends on
user space not using the __BIT_ENDIAN symbol in a conflicting way. So
far we h
Re: [PATCH 03/11] ARM: BCM: put back ARCH_MULTI_V7 dependency for mobile
> Florian Fainelli hat am 12. Februar 2015 um 21:02 > geschrieben: > 2015-02-12 11:42 GMT-08:00 Arnd Bergmann : > > A recent cleanup rearranged the Kconfig file for mach-bcm and > > accidentally dropped the dependency on ARCH_MULTI_V7, which > > makes it possible to now build the two mobile SoC platforms > > on an ARMv6-only kernel, resulting in a log of Kconfig > > warnings like > > > > warning: ARCH_BCM_MOBILE selects ARM_ERRATA_775420 which has unmet direct > > dependencies (CPU_V7) > > > > and which of course cannot work on any machine. > > > > This puts back the dependencies as before. > > Since both of these Kconfig options also select ARCH_BCM_MOBILE, you > could put the select there instead? No, that would only work with 'select', but we need 'depends on' here. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
