Hi Peter, On 2018/11/24 2:45, Peter Maydell wrote: > On Wed, 21 Nov 2018 at 14:34, gengdongjiu <gengdongjiu@xxxxxxxxxx> wrote: >> >> Hi Peter, >> Thanks for the review and comments. >> >>> >>> On 8 November 2018 at 10:29, Dongjiu Geng <gengdongjiu@xxxxxxxxxx> wrote: >>>> +bool write_part_cpustate_to_list(ARMCPU *cpu, ptrdiff_t fieldoffset) >>> >>> What is this about? Nothing else in QEMU needs to mess with the cpustate synchronization. My first assumption is that you should not >>> need to do so either. >> >> We should change the guest CP15 ESR_EL1's value, the only method is to change the cpu->cpreg_values[] in QEMU, then QEMU call write_list_to_kvmstate() >> to set the cpu->cpreg_values[] to KVM which include the specified ESR_EL1 value, KVM do world switch, and then set the specified ESR_EL1's value to guest kernel. > > Ah, I see. This is a bug in our current handling of the register > state, where we implicitly assume that nothing in QEMU will ever > want to change any system register values. This assumption is > now false -- kvm_arm_handle_debug() broke it -- so we need to > fix the code that does kvm_arch_put_registers(). There is a comment > in the kvm32.c version of that function about this. (The kvm64.c > version has the same assumption but doesn't comment on it.) > > We should (ideally) fix this bug in the code that does register > syncing, without requiring places in QEMU that update system > registers to have to manually indicate which registers they have > changed. I'll have a think about how best to do this. Ok, it is great that you will think about it, waiting for your wonderful solution > >> About the detailed explanation, as shown in [2]. >> >> kvm_arm_handle_debug() does not need to do this because QEMU does not need to change CP15 registers, such as ESR_EL1. > > kvm_arm_handle_debug does change ESR_EL1: it is injecting an exception > and so should set the exception register. This happens when it > calls the do_interrupt() hook, because arm_cpu_do_interrupt_aarch64() > writes to env->cp15.esr_el[new_el]. Yes, I see it, but the env->cp15.esr_el[new_el] shouldn't be successfully set to KVM when call kvm_arch_put_registers() > > I'm not entirely sure why this is working today, in fact. > Alex, did you test whether our debug-exception-injection > reports the correct ESR_EL1 to the guest ? Alex? > >>>> +/* Inject synchronous external abort */ static void >>>> +kvm_inject_arm_sea(CPUState *c) { >>>> + ARMCPU *cpu = ARM_CPU(c); >>>> + CPUARMState *env = &cpu->env; >>>> + CPUClass *cc = CPU_GET_CLASS(c); >>>> + uint32_t esr; >>>> + int ret; >>>> + >>>> + /* This exception is synchronous data abort */ >>>> + c->exception_index = EXCP_DATA_ABORT; >>>> + /* Inject the exception to guest EL1 */ >>>> + env->exception.target_el = 1; >>> >>> These comments don't tell us anything that the code does not. >> >> Thanks, do you mean I need to remove it or add more detailed comments to it? > > As a rule of thumb, comments should provide information to > the reader which they wouldn't get if they only had the code. > Comments often answer the "why do we do this" question, or > provide an overall summary of what the code is going to do, > or refer to an external source (a datasheet, an algorithm) > that is necessary to understand the code. It's better to > avoid comments that say "what the code is doing" at a line-by-line > level, because the code itself already answers the "what" > question at that level of detail. sure, got it, thanks for the explanation and guidelines. > > thanks > -- PMM > > . >