Hello Andrew, First, thanks for the comments.
On 22 July 2015 at 14:38, Andrew Jones <drjo...@redhat.com> wrote: > I took a quick look at this and see issues with the test code. First, > you're spinning on a stack variable with this, > > /* Wait for our turn */ > while(next_cpu != cpu); > > next_cpu needs to be global, and incremented atomically. I haven't gotten > around to adding atomic_add/inc yet, but it would easy, and I'm happy to > do it, even yet this week. I tend to agree with what you are saying, but isn't a static local declaration forcing all CPUs to see the same variable (and not their instance)? Otherwise the test would hang indefinitely with multiple CPUs in this check. About atomicity I thought of using a lock, but then I realized that there is no way to have a race condition. Let me explain in more detail: Variable 'next_cpu', as being static will be initialized to 0, making all CPUs, except CPU0, to spin. Since only CPU0 will update next_cpu (from 0 to 1), while all others are spinning, incrementing it atomically shouldn't be mandatory. Of course I might have missed something very obvious, so please correct me if I am wrong. > And, as for the MMU, I see from the comment in your test code that you're > hitting an exception when trying to modify code. This is because the code > is mapped readonly in order to use it from usermode. I suggest you modify > the page tables (see below for how) to map the code writeable. Do this > before kicking your secondary cpus, so they'll come up ready. Indeed it is as you say. When I was experimenting with the MMU, by removing PTE_RDONLY in lib/arm/mmu.c:113 I was able to run the test without the need to disable it. At the time, I was uncertain how to do it properly without this hack (which was outside the test case), so instead I opted to disable the MMU. Your suggested way, to set it inside the test case, makes sense, so I will probably do it like that. > There are other issues you'll need to fix as well though in the test code; > count should be initialized, result should be volatile, others? I suggest > you make sure it works for one vcpu first. Again, 'count' is declared as static, which initializes it to 0. I played around with 'result' being volatile or not, but I didn't see any difference. I guess it is better to play it on the safe side, since this one is sensitive for the test. > Just thought of another issue with the unit test. There's no isb() > following the code modification. Good catch, I played around with dsb and isb, as well as local_flush_tlb_all() at the time, but I wasn't sure about if I am paranoid or really needed. Since you also mention this I will test more the use of an isb after modifying the opcode. > I look forward to seeing your fixed up kvm-unit-test test posted. Please > CC me on it. Thanks again for your input, I will try to update by early next week. Best regards.
