Hi Fuad, On Wed, Oct 07, 2026 at 11:29:11PM +0200, Fuad Tabba wrote: > Hi Gustavo, > > On Wed, 07 Oct 2026 17:56:30 +0100, Gustavo Romero <[email protected]> wrote: > [...] > > > With a 52-bit OA, walk a two-entry level 0 table through an IPA with > > > bit 50 set, which stage 2 maps back onto the table, so the walk > > > succeeds only if TTBR[4] is cleared from bit 4 of the base address. > > > With TCR.DS or VTCR.DS and a 48-bit OA, check that setting TTBR[5:2] > > > is a level 0 Address size fault. > > > > Maybe move this commit message to ttbr-baddr.c header and just replace it > > with > > something like "Add a test for checking if with a 52-bit OA a walk ...". > > Sure, I'll move it to the file header in v3. > > > I like better the test split done in v1. There there are two tests and the > > changes are clear, helping to understand which parts of the test are > > related to > > each fix. > > > > So the organization I think is better for this series is: > > > > Fix 0 > > Test 0 > > Fix 1 > > Test 1 > > Ack. > > [...] > > > > + /* > > > + * 52-bit OA: TTBR[4] is base address bit 50. Through stage 2, the > > > + * table at HIGH_IPA | s1_l0 is s1_l0, so the walk succeeds only if > > > > I struggled a bit to make sense of this part: "Through Stage 2, the table > > HIGH_IPA | s1_l0 is s1_l0 ...", not so much for the duplication of s1_l0 in > > the > > text but because Stage 2 uses the s2_l* tables. Any chance you could > > enhance a > > the explanation here? > > The level 0 table address is an IPA, and the s2_l2_high mapping is > what brings it back to s1_l0. How about: > > /* > * 52-bit OA: TTBR[4] is base address bit 50, so with it set the > * level 0 table is at IPA HIGH_IPA | s1_l0. Stage 2 maps that IPA > * onto s1_l0 (through s2_l2_high), so the walk succeeds only if > * TTBR[4] does not also offset the two-entry table by 0x10. > */
Yep, that's good. However, I missed one thing that I think is important to mention and that confused me after a while not looking at these TCG tests: RAM_BASE is precisely defined as 1 << 30 because the way we link the tests based on the memory layout defined in tests/tcg/aarch64/system/kernel.ld, right? That's the bit that also confused me, like, how Stage 2 entries: s2_l2[RAM_BASE >> 29] = RAM_BASE | S2_BLOCK; s2_l2_high[RAM_BASE >> 29] = RAM_BASE | S2_BLOCK are taking us back to s1_l0. How s1_l0 ends up related to RAM_BASE :) Hence, I think it's worth mentioning that relationship also in this text. How about something like: "RAM_BASE is chosen to match the memory layout defined in kernel.ld. As a result, s1_l0 resides at an offset from RAM_BASE after linking." Wdyt? > Thanks for the review. I'll respin once Richard and Peter have had a look. np! Sure ;) Cheers, Gustavo
