-
Notifications
You must be signed in to change notification settings - Fork 6.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
xtensa: add qemu_xtensa_dc233c board for MMU testing #61864
Conversation
The following west manifest projects have been modified in this Pull Request:
Note: This message is automatically posted and updated by the Manifest GitHub Action. |
6e13867
to
112307b
Compare
112307b
to
6c4bc4b
Compare
@jcmvbkbc FYI |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks amazing; I'm going to give this a shot ASAP with the call0 work (which I think I have fleshed out now -- all the pieces are there and make sense and I just need to fix all the goofs, which is going to be a ton easier on qemu than it has been on the Mediatek board, can I just say?)
Some notes and nitpicks if you have time, but my vote would be to merge this fast and fix it later as needed.
@@ -0,0 +1,279 @@ | |||
/* | |||
* THIS FILE WAS AUTOMATICALLY GENERATED. DO NOT EDIT. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a patch in the mt8195 PR that will automatically invoke the script at build time, might be worth trying that if you have time.
|
||
__asm__ volatile("rsr.vaddrstatus %0" : "=r"(vaddrstatus)); | ||
__asm__ volatile("rsr.vaddr0 %0" : "=r"(vaddr0)); | ||
__asm__ volatile("rsr.vaddr1 %0" : "=r"(vaddr1)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool. I bumped into this handler in my transcription and couldn't figure out why this was happening here vs. just in the assembly entry hooks. Nice to see it isn't needed for something I didn't understand. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These actually are neat because you can see which address is at fault, especially fault on data as EXCVADDR points to the code but not the data address.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EXCVADDR points to the code but not the data address.
This is not true. EXCVADDR points to the faulting data address in most of the exceptions, epc1 points to the faulting instruction.
vaddrstatus/vaddr* were introduced in the very recent MMU version that can work with cores having more than one load/store unit, MMUv3 (the one in dc233c) doesn't have it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Weird... maybe there is something wrong with the xt-ocd and GDB I have been using on the hardware. Will need to double check.
* i.e. vecbase, so that we need to reserve those | ||
* space or else k_mem_map() would be mapping those, | ||
* resulting in faults. | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mentioned this in the other PR, but I really think this particular code belongs in the SOC layer, as that's where memory map responsibility is and we can be a TON more prescriptive (e.g. "reserve the 4k vector region area at the start of hpsram" or whatever instead of "linker scripts usually...").
_WindowVectors_text_start = ABSOLUTE(.); | ||
KEEP (*(.WindowVectors.text)) | ||
_WindowVectors_text_end = ABSOLUTE(.); | ||
} >sram0_0_seg :sram0_0_phdr |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Likewise there's a patch in the mt8195 series which will auto-generate all this vector nonsense for you from the offsets in the core-isa.h file, putting it in a file you can just include that emits to just one memory region. Saves a ton of boilerplate.
.debug.xt.callgraph 0 : | ||
{ | ||
KEEP (*(.debug.xt.callgraph .debug.xt.callgraph.* .gnu.linkonce.xt.callgraph.*)) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having just recopied this same code too, we really should have an "xt-toolchain-debug.ld" include or something rather than continuing to cut and past stuff that quite frankly I don't have any understanding of at all.
* | ||
* Linker script for the Xtensa platform. | ||
*/ | ||
#include <xtensa-dc233c.ld> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Irrelevant nitpick, but I don't see the value to putting the linker script in a separate file included from the official linker script file? If it was shared with other SOCs or had some configurability, sure, but this seems like just creating a new file for no reason?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am just copying from sample_controller
here. My plan is to see if we can use dc233c
for qemu_xtensa
, so we can get rid of sample_controller
. At that point, I will need to clean all these up and refactor. That's going to take some time to unify some of the common linker script stuff for all Xtensa platforms.
xtensa_itlb_entry_write_sync( | ||
Z_XTENSA_PTE(vecbase, Z_XTENSA_KERNEL_RING, | ||
Z_XTENSA_MMU_X | Z_XTENSA_MMU_CACHED_WT), | ||
Z_XTENSA_TLB_ENTRY((uint32_t)vecbase, 4)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recognize there's some SOC-specific code paths with doing TLB setup, but this really seems like an arch layer thing to me. Maybe we could generate pin settings for vectors based on a simpler API where the SOC just labels its vector region for the arch layer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue here is that all 4KB instruction TLBs are auto-refills and cannot be pinned. With default config, way 4 TLB covers 1MB, way 5 128MB, and way 6 512MB. We cannot simply pin a whole 1MB region in instr TLB in the common arch layer because that will make the whole region executable, and is a security risk (e.g. RODATA being executable). So this needs to be done on a platform or SoC basis as the linker script decides where other stuff locates, as it is highly dependent on the memory layout.
Adds a CPU binding for the Xtensa LX3 core. Signed-off-by: Daniel Leung <[email protected]>
Turns out not all MMU enabled Xtensa cores have vaddrstatus, vaddr0 and vaddr1. And there does not seem to be a way to determine whether they are available. So remove them from the exception printout for now. Signed-off-by: Daniel Leung <[email protected]>
MMU needs to be initialized before going in to C, so z_xtensa_mmu_init() is called in crt1.S before call to z_cstart(). Note that this is the default case and crt1.S can be disabled if board and SoC desire to do so. Signed-off-by: Daniel Leung <[email protected]>
This adds code to deal with TLB misses as these comes as level 1 interrupts. Signed-off-by: Daniel Leung <[email protected]>
This removes the identity map of the first 512MB in TLB way 6. Or else it would interfere with mapped entries resulting in double mapped TLB exception. Signed-off-by: Daniel Leung <[email protected]>
During MMU initialization, we clear TLB way 6 to remove all identity mapping. Depending on CPU configuration, there are certain number of entries per way. So use the number from core-isa.h to clear enough entries instead of hard-coded number 8. Specifying an entry number outside of permitted range may result in CPU reacting in weird way so better to avoid that. Signed-off-by: Daniel Leung <[email protected]>
Simply using __data_start and __data_end is not enough as it leaves out kobject regions which is supposed to be near .data section. So use _image_ram_start and _image_ram_end instead to enclose data, bss and various kobject regions (among others). Signed-off-by: Daniel Leung <[email protected]>
If CONFIG_XTENSA_RPO_CACHE is not enabled, it can be assumed that memory is not double mapped in hardware for cached and uncached access. So we can specify those regions to have cache via TLB. Signed-off-by: Daniel Leung <[email protected]>
rsync after writing MISC0..3 or EXCSAVE1..7 registers is needed before reading them. Signed-off-by: Flavio Ceolin <[email protected]>
Instead of only handling data TLB misses for VECBASE, change it to handle all data TLB misses in the double exception handler. It is because we may encounter data TLB misses when trying to preload page table entries inside user exception handler. Signed-off-by: Daniel Leung <[email protected]> Signed-off-by: Anas Nashif <[email protected]> Signed-off-by: Flavio Ceolin <[email protected]>
This adds arch_reserved_pages_update() which is called in k_mem_manage_init() to reserve some physical pages so they are not re-mapped. This is due to Zephyr's linker scripts for Xtensa which often puts something before z_mapped_start (aka .text, for example, vecbase). That space needs to be reserved or else k_mem_map() would be mapping those that could result in faults. Signed-off-by: Daniel Leung <[email protected]>
...and move it to xtensa_mmu_priv.h. This would allow the SoC layer to use the RING number if needed. Signed-off-by: Daniel Leung <[email protected]>
...VECBASE during MMU initialization. This is to make sure that we can use the TLB miss handling in the exception vector after we have moved back the VECBASE during MMU initialization. Or else we would be forever stuck in ITLB miss because the exception vectors are not in TLB and we cannot populate the TLB because those vectors are not in TLB. Signed-off-by: Daniel Leung <[email protected]>
This adds a function arch_xtensa_mmu_post_init() which can be implemented on the SoC layer to perform additional MMU initialization steps if necessary. Signed-off-by: Daniel Leung <[email protected]>
The Xtensa HAL has been updated with the following changes: * Removed intel_s1000 overlay files as it is no longer needed. * Fix uninitialized variable warning in xthal_v2p() * Add config and overlay files for dc233c core so we can use QEMU with this core to test MMU code. Signed-off-by: Daniel Leung <[email protected]>
This adds SoC and board configs to support the dc233c core that is available on QEMU. This core has more features than sample_controller, such as MMU support. Signed-off-by: Daniel Leung <[email protected]>
merging as this is blocking testing of new toolchains in the new zephyr sdk. |
This adds a new board
qemu_xtensa_dc233c
so that we can test MMU on Xtensa using QEMU, and various other changes needed for this to work.Needs: