Firstly, I want to say that this patch is OK. It only slightly changed memory layout for TLS segment (because it uses malloc_aligned()) which exposed unrelated bug in ARM TLS relocation.

In D16555#353199, @alc wrote:

Which linker is being used?

Can you please email me the output of "procstat -av"?

Output mailed directly.
And about linker - it's hard to say :)

Tested on Jetson TK1 (cortex-A15) without single problem.
From preliminary collected statistic it looks like pmap_enter(..., psind == 1) is used ~60 times per buildworld.
Unfortunately, the same statistic also told me that is pmap_enter_object() doesn't not create single 1MB mappings, even for shared libraries.
I'm pretty sure that this worked before - but this regression is not related to this patch.

The commit message is a bit misleading. This patch ensures DMA coherency only for control structures but audio data buffers are not covered by required bus_dmamap_sync() operations. (We simply don't have right sync operation for syncing in progress DMA buffers - something like make buffer range snapshot before CPU read and make buffer range snapshot after CPU write).
Can you, please, elaborate this fact in commit message.

Applied, booted new kernel and make buildworld passed without problem.
LGTM.
Thanks for your ARM effort.

Rebased to current, renamed UBOOT_BOOT_API to LINUX_BOOT_API

Tested also on RPi-B (armv6) without problem.
Please, also remove rng related lines from sys/dts/arm/rpi[2].dts stubs.

+100 for overlays.
But I'm not sure if it's right time to publish base .dtb files in this way. We cannot use it for now (because interrupts for uart and spi are routed throw "bcm2835-aux") and it's unclear if binding headers (dts/include/dt-bindings/*) are same (or compatible) as linux mainstream ones.

• print actual mitigation variant if bootversode
• slightly restructure code to make additional vendors or CPUs addition easier

Addressed all objections.

In D13861#290576, @andrew wrote:

You didn't describe any reason it wouldn't work. Rather than the kernel as the first argument to booti, it would be the second, after either passing it through mkimage, or loading it as a raw file with a size.

mkimage, imho, is not supported(or not preferred - it have bundled load address inside) on arm64, but this is not important yet.

I agree but, unfortunately, yes. The context_id is used for selecting right stack. See line 270 in new file.

Boot CPU have always cpuid 0 so yes, it boots (i hope). I have disordered cpus nodes in my DTS and all looks OK.

Yep, I known. But my actual situation with Jetson TX1 board is more complicated.

• the TX1 firmware can only load U-Boot for eMMC.
• shipped U-Boot doesn't support EFI
• on OS start, U-Boot modifies DTB (it excludes memory used by secure monitor and PSCI,, it modifies the pinmux table so that it matches the current setting), it stored trained settings for DDR4 controller for various memory frequencies...)

imho, we should put platform specialized drivers to dev subtree only if given driver can be reused on multiple platforms. I personally vote for not moving it.

Perfect, thanks.

Everything else looks OK for me.

Nice, this looks OK for me
But,it seems that there are some related problems:

• the PCB for proc0/thread0 is allocated on top of stack, from dirty memory, but not all fields are not initialized - this is probably a root cause why this bug is not visible on all boards.
• we leaks initial low memory mappings in kernel_pmap. This can results to double mappings with different attributes which is undefined behavior. Imho, we should clear all mappings in low memory region.

yes, but the fix looks incorrect for me.
I think that we can have same problem with secondary cores(idle threads). Imho, cpu_startup() is more appropriate place for this and we should derive TTBR0 value from kernel_pmap[1]. See arm version of cpu_startup() and call to pmap_set_pcb_pagedir(kernel_pmap, pcb); .

No objection from arm or intrng side.

I can confirm same issue on Cortex-A72. Loading zero to TTBR0_EL1 causes immediate exception (in sched_switch+0x3a0).

Thanks !!!

Kib,
thanks. I will incorporate all your comments into final commit.

Sorry for delay. We don't support arm for FreeBSD 10 and arm packages are built only for FreeBSD11 and higher.
See http://pkg.freebsd.org/

Generally speaking, I think that implementing more and more knowledge about TLS ABI into kernel is direct way to hell and it simply leads to hard to solve compatibility issues.
The ideal kernel should take TP pointer as opaque value, without any attempt to interpret it.

with full respect, I don’t think that this is right way. Moreover, I think that you papering over real problem there.
With this patch, what’s happen if someone requests any higher alignment (that actual 16) for TLS data? Or, can patched kernel run the old init (pre r324938)?

I can confirm that R cad be built on native ARMv7 system with this.

Seems that exp-run passed without problems. Can you, please, commit this ?

• malloc_aligned() modified to rtld version
• added alignment fix also for Variant I
• check malloc() return values
• use custom version of assert()

In D12743#264414, @kib wrote:

Does it make sense to include machine/elf.h from auxv.h ? Why not make it a user duty ?

Imho yes, AT_ values are defined in machine/elf.h so user should include it in (almost) all cases.

Would be nice to provide some kind of man page.

I will ask Ian for man page, this kind of job is significantly out of my skill (even in my native language). My bad I known.

Split out libc changes

Drop getauxval(), make _elf_aux_info() public.

My exp-run for selected ports just ended and It found unexpected problem.
Some ports detect getauxval() presence and if is present then expect that all (linux specific) AT_ flags are defined and implemented (e.g. security/p11-kit ).

Can be this MFCed to stable? In other words, do we support in place upgrade from 10 (soft FP ABI) to 11 (hard FP ABI)?
From my point of view, this is OK for FBSD 12.

Oups, my bad, sorry.
We cannot use standard way for tunable sysctl initialization here. SYSINIT machinery is executed far after cpuinfo_init() so boot CPU doesn't get right quirks.
Anyway, I submitted fix for this in r320054.
So again, sorry for troubles...

I'm fine with rest.

In D11203#231938, @mw_semihalf.com wrote:

In D11203#231935, @meloun-miracle-cz wrote:

I do not think this patch was accepted in mainline Linux -> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-August/363953.html

But this one is -> https://github.com/torvalds/linux/commit/98ea2dba65932ffc456b6d7b11b8a0624e2f7b95
Please note that commit log uses "the outer cache sync operation is useless: and not a " the entire outer cache operations are useless".
Also note that this behavior is driven by "arm,io-coherent" property.

Event better. We were thinking how to replace the callbacks in the pl310 in a nice way. Are you ok with using "arm,io-coherent" property in pl310 in a similar way?

I'm fine with both solutions, with preference to "arm,io-coherent". Using standardized, documented way is always better :)

I do not think this patch was accepted in mainline Linux -> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-August/363953.html

When a PL310 cache is used in a system that provides hardware
coherency, the entire outer cache operations are useless, and can be
skipped.

This is significantly incorrect/misleading/false sentence. All these outer cache operations are necessary for standard mapping operations, irrespective of coherency status.
Please, reword this part in commit.

Too late but still...
What exactly is "cpu_class" and why we need at all? I understand why it's useful for ARMv4, but it's near useless for ARMv7 (there is too much variants)

In D10682#231591, @zbb wrote:

In D10682#231585, @meloun-miracle-cz wrote:

BTW. As for the unsolvable problem with the memory attributes change from DEVICE to SO we could use the same method as you used for ACTLR modification, means we could create another sysctl quirk and so on...
In platform code for Armada38X we could then check whether memory attributes are set as expected and if not we could fire an assertion. What do you think?

Imho, this problem is already solved by pmap_remap_vm_attr() (r318021) - or I didn't notice something important?

OK. I though that using this function so late was discussed to be not allowed. But if it is OK to use in platform_late_init than this concludes this issue. So I understand that will get a green light after putting pmap_remap_vm_attr() to platform_late_init() ?

In D10682#231259, @zbb wrote:

@meloun-miracle-cz if I understand correctly, in your commit you simply create a environment variable to modify ACTLR from the loader.conf or etc.
We can use that solution directly for our purposes without any further modifications, platform_late_init() complications and etc. Until ARMADA38X will be compatible with the GENERIC kernel we could potentially add environment configuration file to be compiled-into the kernel. The latter would need testing whether it works with the loader.conf file as well or not (I expect problems with that).
Regardless of the settings for Armada38X we can drop this revision completely and start using environment variables. Agree?

Yep, exactly, I agree.

Hmm, allow me a short summary:

• This patch simply cannot work. On boot core, the platform_cpu_init() is called before platform is recognized. On secondary cores, platform_cpu_init() is called before given core enters to coherency domain, so each single memory write (outside of stack) causes disaster..
• I don't see any way how we can do any platform specific operation before reinit_mmu() is called.
• The goal can be reached by different, much more consistent way.

In D10765#225464, @dim wrote:

It would be nice to have some tests, even if they are rudimentary... :)

The one should really save all edited files before updating review :(

• fix style(9) for copyright blocks
• pair RETURNV() with ENTERV()

Oups, the 'lib/msun/src/math.h.orig' will not be included in the final commit, of course.

Nothing strong, but I would probably prefer a new method for late init on secondary cores, instead of reusing of platform_late_init() - something like platform_mp_late_init.
Code sharing between BP / AP will be very rare, i think.

I would prefer to move this code to platform_late_init().
The only drawback is that you must use cp15_actlr_set() + cp15_actlr_get() for boot CPU...

I think that this patch is not longer needed?
Both issues (device memory class remap, ACTRL modification) can be processed in platform_late_init().
I'm right?

Otherwise, I'm fine with this.

Committed as r318021. Sorry for huge delay...

Thanks for clarification, I see it now.

Thanks.

In D8616#212404, @andrew wrote:

In D8616#210278, @meloun-miracle-cz wrote:

In D8616#210126, @andrew wrote:

In D8616#210013, @skra wrote:

(1) The staff around PIC and xref was implemented with concrete idea. You are trying to change it. So, it's not cleanup, it's an implementation of new idea. And I don't see why this change would be better.

It might be ok for FDT, however we need to also consider ACPI where zero is a valid xref.

(2) You are forcing devices which do not know (or have) their XREFs to generate some instead of using one and only XREF_UNKNOWN for these cases, which is zero.

But zero may be a valid xref. I would prefer we create a new value for XREF_UNKNOWN separate from XREF_INVALID, e.g. -2. This will be used by devices that don't naturally have an xref, e.g. a PCI GPIO controller, but would allow the error checking in the patch to be kept. I didn't do this in this patch as I'm not aware of any drivers in the tree that would use this so am unable to test it.

In short:

• in current code takes 0 (or NULL) xref as unknown xref not as error indicator.
• we cannot expect that all "information sources" *FDT/ACPI/HINTS" can share same value for unknown xref natively

I don't see why, both ACPI and FDT will use positive values close or equal to zero.

• natural value for unknown xref is 0 for FDT , NULL for HINTS

-1 is a natural value for unknown, it's too large for an FDT xref to work, and memory around the top of the virtual address space is unusable by HINTS. It also has the property where

• if you want change value of unknown xref correctly, then you must also remap FDT no such property to INTRNG unknown xref for all FDT users

As best as I can tell the OFW code uses -1 for unknown values. This patch will help find bugs where these values are unchecked.

• the "#define XREF_INVALID (-1)" is nonsense, it's hidden "#define XREF_INVALID ACPI_INVALID_HANDLE" as these values are coupled

The exact value doesn't matter, e.g. -2 would also work, however by using -1 we will catch bugs in drivers that fail to check the return value of ofw_bus_get_node before passing them to OF_xref_from_node.

(3) XREF_INVALID used the way you are suggesting is a nonsense. I would see XREF_INVALID only as an error code.

We already use errno values in this code so the error code would be EINVAL.

Where we pass any error code to xref parameter? Ore where we return error code from function returning xref?

They shouldn't be passing in error values as if it was a valid xref.

(4) This part of INTRNG was implemented strictly according to Michal, so I hope that you do not commit this before Michal agrees with it.

As far as I know I've answered all questions, and haven't seen any further feedback in a month.

Sorry for delay, I'm overloaded these days (or weeks)
And no, I don't think that you answered my questions:

• newly added kasserts limits already working cases. But why?

Because drivers passing the invalid value in are broken, they should only get this from failing to check return values.

• you want to change already working pic_lookup() behavior. But why?

Because it won't work with ACPI. As such I tried to make the minimal changes needed to support this.

Can you be, please, little more verbose? I'm unable to detect where exactly the problem is.

In D10218#211611, @andrew wrote:

In D10218#211540, @meloun-miracle-cz wrote:

Please tell me, do you really think that hellish "#ifdef SOC_xxx" style used in Marvell subdirectory is the right one?
For me, using "#ifdef SOC_xxx" is unacceptable in common code, and is deprecated in vendor subdirs.

The SOC_XXX checks are correct when it's to enable code that will only be run on a single SoC, e.g. the platform code. Unfortunately the Marvell code uses it to change definitions and I would like to move away from that style.

Yep, I agree. Thanks for clarification.

Armada38x have Marvell specific (or modified) PL310 ? If not then all errata fixes (but two at line 274 and 295) are checked online. Why do you want to disable them? Do you have measured impact caused by these tests?

Please tell me, do you really think that hellish "#ifdef SOC_xxx" style used in Marvell subdirectory is the right one?
For me, using "#ifdef SOC_xxx" is unacceptable in common code, and is deprecated in vendor subdirs.

Alternatively, we can use global variable for unknown xref value , initialized at early system startup.

In D8616#210126, @andrew wrote:

In D8616#210013, @skra wrote:

(1) The staff around PIC and xref was implemented with concrete idea. You are trying to change it. So, it's not cleanup, it's an implementation of new idea. And I don't see why this change would be better.

It might be ok for FDT, however we need to also consider ACPI where zero is a valid xref.

(2) You are forcing devices which do not know (or have) their XREFs to generate some instead of using one and only XREF_UNKNOWN for these cases, which is zero.

But zero may be a valid xref. I would prefer we create a new value for XREF_UNKNOWN separate from XREF_INVALID, e.g. -2. This will be used by devices that don't naturally have an xref, e.g. a PCI GPIO controller, but would allow the error checking in the patch to be kept. I didn't do this in this patch as I'm not aware of any drivers in the tree that would use this so am unable to test it.

In short:

• in current code takes 0 (or NULL) xref as unknown xref not as error indicator.
• we cannot expect that all "information sources" *FDT/ACPI/HINTS" can share same value for unknown xref natively
• natural value for unknown xref is 0 for FDT , NULL for HINTS
• if you want change value of unknown xref correctly, then you must also remap FDT no such property to INTRNG unknown xref for all FDT users
• the "#define XREF_INVALID (-1)" is nonsense, it's hidden "#define XREF_INVALID ACPI_INVALID_HANDLE" as these values are coupled