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