On Mon, Apr 15, 2024 at 5:35 PM Peter Maydell <peter.mayd...@linaro.org> wrote: > > On Sat, 13 Apr 2024 at 20:59, Dorjoy Chowdhury <dorjoychy...@gmail.com> wrote: > > > > Hi, > > Hope everyone is doing well. I was looking at "Bite Sized" tagged QEMU > > issues in gitlab to see if there is anything that makes sense for me > > as a first time contributor. I see this issue "QEMU gives wrong MPIDR > > value for Arm CPU types with MT=1" (gitlab URL: > > https://gitlab.com/qemu-project/qemu/-/issues/1608 ). > > > > From the bug ticket description, it is very clear that I will need to > > add a bool member variable in the "AarchCPU" struct which is in > > "target/arm/cpu.h" file. I believe the next logical change is to set > > this member variable to "true" in the corresponding arm cpu "initfn" > > functions (a55, a76, noeverse_n1) which are in "target/arm/cpu64.c" > > file. I have a few questions about the following steps as I am looking > > through the code. > > > > 1. I believe I will need to update the "arm_build_mp_affinity" > > function in "target/arm/cpu.c" file to now also take in a bool > > parameter that will indicate if the function should instead put the > > "core index" in the "aff1" bits instead of the existing logic of > > "aff0" bits and the cluster id in the "aff2" bits instead of the > > existing logic of "aff1" bits. But I see this function being invoked > > from 3 other files: "hw/arm/sbsa-ref.c", "hw/arm/virt.c", > > "hw/arm/npcm7xx.c". Should the function calls in these files always > > have that corresponding argument set to "false"? > > This bit of the codebase has got a bit more complicated since > I wrote up the bug report. I will look into this and get back > to you, but my suspicion is that these calls must return the > same value that the actual CPU MPIDR affinity values have, > because these values are going to end up in the DTB and ACPI > tables, and the OS will want them to match up with MPIDRs. >
Sounds great. Let me know. It sounds like then it could make sense to change the "arm_build_mp_affinity" function signature in file "target/arm/cpu.c" file to be like this: uint64_t arm_build_mp_affinity(bool has_smt, int idx, uint8_t clusterz) I think in all the files where it is invoked it should be possible to know the SMT status of the cpu using ARMCPU(qemu_get_cpu(cpu)) or similar. Let me know what you think. > > 2. As per the bug ticket description, I will also need to update the > > "mpidr_read_val" function in the "target/arm/helper.c" file to only > > set the MT bit (24th) to 1 if the member variable is true. I think > > there is nothing else to be done in this function apart from checking > > and then setting the MT bit. Is my assumption correct? > > Yes, that's right. > > > I think doing the above steps should fix the bug and probably we don't > > need anything else. It would be great if someone can help me answer > > the questions or any suggestion would be great if my assumptions are > > wrong. Thanks. > > The other thing we need to do is check the TRM (technical reference > manual) for the CPUs that were added since I filed that bug in > April 2023, to see if they need to have the flag set or not. The > ones we need to check are: > * cortex-a710 > * neoverse-n2 > * neoverse-v1 > Good point. I have now looked at the TRMs of the a710, neoverse-n2, neoverse-v2 and they are similar like the ones mentioned in the gitlab bug ticket i.e., MT bit should be 1, Aff0 should be 0, Aff1 should be core index, Aff2 should be cluster id. > thanks > -- PMM Let me know what you think. If everything sounds alright, I will try and post a patch. Thanks and regards, Dorjoy.
