I think Peter's patch is good, I will resubmit my series patches based on this.
On 2019/2/14 4:18, Alex Bennée wrote: > > Peter Maydell <peter.mayd...@linaro.org> writes: > >> On Wed, 13 Feb 2019 at 16:29, Alex Bennée <alex.ben...@linaro.org> wrote: >>> >>> >>> Peter Maydell <peter.mayd...@linaro.org> writes: >>> >>>> At the moment the Arm implementations of kvm_arch_{get,put}_registers() >>>> don't support having QEMU change the values of system registers >>>> (aka coprocessor registers for AArch32). This is because although >>>> kvm_arch_get_registers() calls write_list_to_cpustate() to >>>> update the CPU state struct fields (so QEMU code can read the >>>> values in the usual way), kvm_arch_put_registers() does not >>>> call write_cpustate_to_list(), meaning that any changes to >>>> the CPU state struct fields will not be passed back to KVM. >>>> >>>> The rationale for this design is documented in a comment in the >>>> AArch32 kvm_arch_put_registers() -- writing the values in the >>>> cpregs list into the CPU state struct is "lossy" because the >>>> write of a register might not succeed, and so if we blindly >>>> copy the CPU state values back again we will incorrectly >>>> change register values for the guest. The assumption was that >>>> no QEMU code would need to write to the registers. >>>> >>>> However, when we implemented debug support for KVM guests, we >>>> broke that assumption: the code to handle "set the guest up >>>> to take a breakpoint exception" does so by updating various >>>> guest registers including ESR_EL1. >>>> >>>> Support this by making kvm_arch_put_registers() synchronize >>>> CPU state back into the list. We sync only those registers >>>> where the initial write succeeds, which should be sufficient. >> >>> [snip a long test setup that I didn't really understand] > > Sorry that was just making sure I'd documented the exact steps of the > manual test for posterity. > >> >> I'm confused -- I'm not sure if the things you're saying >> didn't work: >> (a) didn't work before this patch and still don't >> (b) used to work and are broken by this patch >> (c) used to not work and are fixed by this patch > > option (c) - this patch has made things better hence the t-b and r-b > tags. I might send you a follow up if we can also have single-step > working as well but that requires me to test a kernel patch (and remove > the error leg for guest single step in kvm64). > >> >> More generally: >> * this patch claims to fix the code path where QEMU sets >> up the guest to take a breakpoint exception (specifically >> making our change of ESR_EL1 actually take effect) -- so does >> it do that? Or is it impossible for that code path in QEMU >> to be used (if so, can we just remove it) ? > > No it's needed - I'm observing exceptions coming through the path and > then being delivered to the guest while the host is debugging - modulo > the you can't have active HW breakpoints in both guest and host at the > same time. > > Even here now we are sure the guest state is correctly reflected we > could make the debug code smarter and try it's best to preserve guest > debug regs while using it's own entirely in userspace. But I suspect > that is a bit niche. > >> >> thanks >> -- PMM > > > -- > Alex Bennée > > . >