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

[Peter Maydell]

On Mon, 19 Jun 2023 at 15:31, John Högberg <john.hogberg@ericsson.com> wrote:
>
> 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.

I think expanding it a little would help.

> > 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?

Similarly here tweaking the comment would be enough.

thanks
-- PMM