Re: [PATCH qemu v2 2/2] tests/tcg/aarch64: Add testcases for IC IVAU and dual-mapped code

[John Högberg]

Thanks for the review! :)

> All new source files must start with a license-and-copyright
comment.
> ... snip ...
> Generally in QEMU we prefer to typedef the function type,
not the pointer-to-function type.
> ... snip ...
> You should probably mark all these asm statements as 'volatile'
> to ensure that the compiler doesn't decide it can helpfully
> reorder or remove them.

Got it, I'll fix those.

> Your asm code may be position-independent, but you're copying the
> entire function here including the preamble and postamble that the
> compiler emits for it, and you don't do anything in the makefile
> to ensure that it's going to be position-independent.
> ... snip ...
> Why use an explicit register variable for this rather than
> having the __asm__ return its result via an output ?

Good point, I'll rewrite this part to avoid these issues.

> This is a QEMU test case, though, and QEMU doesn't care about
> cache lines because we don't model the cache. So why does it
> matter ?

We're trying to catch code modification through the use of instructions
that invalidate cache lines. I wanted the test to cover invalidation of
the code that does the invalidation itself too as "what happens if we
invalidate the current TB and return" came up in the discussion on the
bug tracker, but I can certainly cut this or expand on it in a comment
if you wish.

> There's no guarantee on actual hardware that omitting this
> flush will cause the test to fail. The hardware implementation
> is permitted to drop stuff from the cache any time it feels
> like it, and it might choose to do that right at this point.
> So any attempt to test "fails if we don't flush the icache"
> would be a flaky test. It would also fail on a hardware
> implementation where the icache and dcache are transparently
> coherent (which is a permitted implementation; the CTR_EL0
> DIC and IDC bits exist to tell software it can happily
> skip the cache ops).
>
> System mode QEMU works because we model an implementation
> with no caches (which is also permitted architecturally).

True, do you want me to change the comment to this effect or cut the
comment altogether?

> I don't understand the purpose of all these functions.
> I was expecting the test to be something like
>  * write initial code
>  * execute it
>  * modify it
>  * execute again and check we got the changed version
>
> I don't see why we need four "compare" functions to do that.

Sure, the self-modification test alone should suffice.

Thanks again :)

Regards,
John Högberg