Re: [Qemu-devel] [RFC] arm: Allow system registers for KVM guests to be changed by QEMU code

[gengdongjiu]

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 <address@hidden> writes:
> 
>> On Wed, 13 Feb 2019 at 16:29, Alex Bennée <address@hidden> wrote:
>>>
>>>
>>> Peter Maydell <address@hidden> 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
> 
> .
>