In D22255#489934, @siddharthtuli_gmail.com wrote:

Hi Mmel,

In D22255#489923, @mmel wrote:

In D22255#489896, @siddharthtuli_gmail.com wrote:

Hi Mmel,
Please find my comments inline.

After more detailed analysis, I think that there are more problems.
Let me explain situation. Givens are:
• Actual locore.S mappings of kernel expects that physical memory is aligned to 1GB boundary. This implies that we don’t support boards with lower that 1 GB memory (I don’t see other real limitation caused by this fact).

In locore.S L1 block page table entry is built on 1GiB aligned physical load addr of kernel. However this is irrespective of using loader or U-Boot to boot the FreeBSD kernel.

• U-Boot booti command expect that kernel, fdt, and initrd blobs are loaded at ${kernel_addr_r}, ${fdt_addr_r} and ${initrd_addr_r} offsets.

Yes.
As part of this patch linux_load_initrd() will do build_l1_block_pagetable() to map initrd start address aligned to 1GiB PA=VA.
(By default, initrd_addr_r will be relocated to highmem by booti in U-Boot code (do_bootm_linux->boot_prep_linux->boot_reloc_ramdisk->boot_ramdisk_high)).
locore.S maps 1GiB algined kernel_addr_r PA=VA. I assumed that fdt_addr_r ( a couple of KiB’s in size) can also be within this 1GiB aligned addr. However, fdt_addr_r can be mapped similar to initrd start addr (in linux_load_initrd()).

Real value of these variables is driven by u-boot and we cannot expect nothing about this value. (order, gaps, etc..)

Sure.

It’s possible that kernel want to load kernel at begining of memory and initrd blob at end. We must support this, it's generic part of ‘distro_boot’ u-booot protocol/framework. And support of distro_boot is one of major goals for having booti supported, imo.

Sure. My understanding is that patch handles this. Physical load address of,
kernel is mapped into KVA starting at KERNBASE in locore.S
dtb blob is mapped into KVA starting at [ KERNBASE+(kernel size) ] in machdep_boot.c
initrd is mapped into KVA starting at [ KERNBASE+(kernel size) + (dtb size) ] in machdep_boot.c

This give me some conclusions:
Because of 1) optimal mapping granularity is 1GB, more fine grained granularity is nothing than waste of TLB entries and build_l2_block_pagetable() is useless. (at least I don’t see single advantage of using fine grained mappings, while disadvantage is clear - more TLB pressure ).

[1]
locore.S does maps kernel physical load addr into KVA starting at [ KERNBASE ] to [ KERNBASE + (kernel size)] using build_l2_block_pagetable. KVA is in L2 table which has 512, 2MiB entries. 1GiB L1 table entry is linked to this L2 table.
The following comments from create_pagetables: in locore.S,

• It relys on:
• We were loaded to an address that is on a 2MiB boundary
• All the memory must not cross a 1GiB boundaty
• x28 contains the physical address we were loaded from *

I am mapping dtb and initrd image into L2 table following the end of kernel. Therefore I created build_l2_block_pagetable() in machdep_boot.c
In fact if this not done dtb or initrd cannot be added into KVA. Due to above comments, I added a print in machdep_boot.c if kernel+dtb+initrd will exceed 1GiB.

This is not exact description. You allocates and maps new memory (by using lastaddr) for fdt and initrd blobs. Then you *copy* these blobs to new location. See lines 122 and 127 for fdt, and lines 270, 273 and 275 for initrd.

Basically, I am creating 2MiB KVA L2 table entries to move the fdt and initrd blobs from PA to KVA.

As you can see, you have mapped both source and destination address for initrd blob memmove(), but only destination address was mapped for fdt blob memmove() (point me to right place if I’m wrong, please).

Sorry I did not understand this clearly. inirtd size (in counts of 2MiB) is calculated using initrd-end - inird-start. For dtb the size is available in its header.

Problem is not about initrd but fdt blob
at line 127 we do
memmove((void *)lastaddr, dtb_ptr, dtb_size);
destination memory is mapped at line 122 by
build_l2_block_pagetable(lastaddr, dtb_size, abp
but where is mapped source memory (pointed by dtb_ptr in memmove())????

In D22255#489896, @siddharthtuli_gmail.com wrote:

Hi Mmel,
Please find my comments inline.

After more detailed analysis, I think that there are more problems.
Let me explain situation. Givens are:
• Actual locore.S mappings of kernel expects that physical memory is aligned to 1GB boundary. This implies that we don’t support boards with lower that 1 GB memory (I don’t see other real limitation caused by this fact).

In locore.S L1 block page table entry is built on 1GiB aligned physical load addr of kernel. However this is irrespective of using loader or U-Boot to boot the FreeBSD kernel.

• U-Boot booti command expect that kernel, fdt, and initrd blobs are loaded at ${kernel_addr_r}, ${fdt_addr_r} and ${initrd_addr_r} offsets.

Yes.
As part of this patch linux_load_initrd() will do build_l1_block_pagetable() to map initrd start address aligned to 1GiB PA=VA.
(By default, initrd_addr_r will be relocated to highmem by booti in U-Boot code (do_bootm_linux->boot_prep_linux->boot_reloc_ramdisk->boot_ramdisk_high)).
locore.S maps 1GiB algined kernel_addr_r PA=VA. I assumed that fdt_addr_r ( a couple of KiB’s in size) can also be within this 1GiB aligned addr. However, fdt_addr_r can be mapped similar to initrd start addr (in linux_load_initrd()).

Real value of these variables is driven by u-boot and we cannot expect nothing about this value. (order, gaps, etc..)

Sure.

It’s possible that kernel want to load kernel at begining of memory and initrd blob at end. We must support this, it's generic part of ‘distro_boot’ u-booot protocol/framework. And support of distro_boot is one of major goals for having booti supported, imo.

Sure. My understanding is that patch handles this. Physical load address of,
kernel is mapped into KVA starting at KERNBASE in locore.S
dtb blob is mapped into KVA starting at [ KERNBASE+(kernel size) ] in machdep_boot.c
initrd is mapped into KVA starting at [ KERNBASE+(kernel size) + (dtb size) ] in machdep_boot.c

This give me some conclusions:
Because of 1) optimal mapping granularity is 1GB, more fine grained granularity is nothing than waste of TLB entries and build_l2_block_pagetable() is useless. (at least I don’t see single advantage of using fine grained mappings, while disadvantage is clear - more TLB pressure ).

[1]
locore.S does maps kernel physical load addr into KVA starting at [ KERNBASE ] to [ KERNBASE + (kernel size)] using build_l2_block_pagetable. KVA is in L2 table which has 512, 2MiB entries. 1GiB L1 table entry is linked to this L2 table.
The following comments from create_pagetables: in locore.S,

• It relys on:
• We were loaded to an address that is on a 2MiB boundary
• All the memory must not cross a 1GiB boundaty
• x28 contains the physical address we were loaded from *

I am mapping dtb and initrd image into L2 table following the end of kernel. Therefore I created build_l2_block_pagetable() in machdep_boot.c
In fact if this not done dtb or initrd cannot be added into KVA. Due to above comments, I added a print in machdep_boot.c if kernel+dtb+initrd will exceed 1GiB.

This is not exact description. You allocates and maps new memory (by using lastaddr) for fdt and initrd blobs. Then you *copy* these blobs to new location. See lines 122 and 127 for fdt, and lines 270, 273 and 275 for initrd. As you can see, you have mapped both source and destination address for initrd blob memmove(), but only destination address was mapped for fdt blob memmove() (point me to right place if I’m wrong, please).

After more detailed analysis, I think that there are more problems.
Let me explain situation. Givens are:

1. Actual locore.S mappings of kernel expects that physical memory is aligned to 1GB boundary. This implies that we don’t support boards with lower that 1 GB memory (I don’t see other real limitation caused by this fact).
2. U-Boot booti command expect that kernel, fdt, and initrd blobs are loaded at ${kernel_addr_r}, ${fdt_addr_r} and ${initrd_addr_r} offsets. Real value of these variables is driven by u-boot and we cannot expect nothing about this value. (order, gaps, etc..) It’s possible that kernel want to load kernel at begining of memory and initrd blob at end. We must support this, it's generic part of ‘distro_boot’ u-booot protocol/framework. And support of distro_boot is one of major goals for having booti supported, imo.

This give me some conclusions:
Because of 1) optimal mapping granularity is 1GB, more fine grained granularity is nothing than waste of TLB entries and build_l2_block_pagetable() is useless. (at least I don’t see single advantage of using fine grained mappings, while disadvantage is clear - more TLB pressure ).
Because of 2) – all 3 booti blobs must be taken separately. I don’t see single reason for postponing mapping of FDT to C code. Original locoer.S works and build_l2_block_pagetable() is unusable. Using lastaddr for reservation of memory used by initrd is clearly nonsense. Initrd blob should be reserved by using (probably) arm_physmem_exclude_regions() and map it later , when pmap is fully bootstrapped.

please, use original locore.S from D13861 I'ts tested on many boards and it works :).

Moreover this doesn't boot on RockPro64, where original code from D13861 works without problems. jhibbits, please revert this, this code needs definitely more love. (Please, be sure that I'm first who wants booti support in kernel, but broken support is worse that no support)

I'm sorry for delayed review, but I am overloaded with my current work on Rockchip ports.
But in any case, if you want to reuse someone else's patch, it would be better to include the original author directly as the reviewer.

LGTM

LGTM

rest is OK for me.

The code itself looks OK for me. But I have problem with using its usage as default regulator init method, because:

• it cannot be used by regulators with its own init
• it expect that regulator is accessible early, at registration time. I'm not sure if this is possible in all cases (like I2C and interrupts ..)

Can you, please, give me your motivation for adding new resource types when it’s clear that these resources cannot be handled by resman/newbus without extreme API breakage?
For OFW/FDT world, major issues are:

• • given resource (request, allocated) cannot be described by single integer, and infrastructure must take it fully opaque (there is nothing like ‘resource start’, ‘resource end’ and interval enumerator)
• hierarchy for other resources(clk, regulator, reset, phy, ...) is not bus related and can have circular dependencies – so it cannot be represented by tree, but by graph. This result to fact that bus enumerator cannot prepare resource list for its child devices. This is something which break basic design expectation of newbus.
• circular resource dependencies (between types – frequent, within single resource type – no that frequent but exist mainly for clocks) also means, that we relay need staggered device_attach(), there is no way to do this by automatic ordering of devices attach, driven by resource availability.

Another current newbus or bus_space limitation is that we should have (general, globalized) memory attributes and be should pass it do all memory mapping function. On arm or arm64, PCI config space should be mapped with different attribute (strongly ordered) that pci (or other devices) mmio register space (posted writes are allowed). Actual behavior (all devices are mapped as strongly ordered) have measurable performance impact.
Don’t take me badly, I’m still newbus fan, I only don’t see reliable way how we can extend it without fatal/significant API breakage.

Yep, thanks.

That's simply perfect, many thanks.
I tested this on arm64, all work like charm. (But I'm able to do only limited test, my iflib based driver is still far far away from working state)

Imho, aggregated reading of ID registers is not a best solution.
Personally, I think that we should implement aggregated view by using HWCAPS (As we must do this in any case, upcoming COMPAT32 simply expects this).
But we should return actual/real values of ID registers. This allow us to write program which iterates over clusters and selects best cores for given workload.
I mainly talking about SHA or SVE extensions here.

bingo! I'm on r341345. And yes, r341347 fixed it.
Thanks and sorry for noise.

Ahh, right, I overlooked this, sorry. So I taking back WITHOUT_CAPSICUM case.

How this can works on kernels build without 'options CAPABILITIES'? Note that CAPABILITIES option is
not included in kernel option for small embedded boards (arm, mips...)?
We should change CAPSICUM to mandatory or implement fallback here (as we already do in rest of tree)

Only kernel was changed during bisecting, nothing else. And all kernels was cross-compiled by
same toolchain. Kernel before r339819 works, r339819 hangs, and fresh current with r339819
reverted also works. Also, during bisecting, any kernel after r339819 hangs.
So I thing that r339819 is culprit.

Unfortunately, this patch destabilized arm kernel.
The bad thing is that my Tegra does not panic, just hangs hard and I can’t even enter the JTAG debugger [1].
Moreover, this issue is very rare, I can do finish full buildkernel in most cases. The only known testcase is full
build of qt5-webengine (~16k of C++ files, +10 hours of compilation time).

Manu,
can you, please, subclass phy to USB specialized variant (say phyusb or usbphy)? You can use clk (clkdiv, clkmux ...) as template. We should implement lots of USB OTG related function here. VBUS voltage and over-current detection, ID pin status change interrupt, OTG state machine and I thing that should use proper layering from beginning.
If you too busy for this, I can implement (empty) subclass in next 2-3 days.

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 ?