Re: [PATCH RESEND v15 08/10] target-arm: kvm64: inject synchronous External Abort

From: gengdongjiu <gengdongjiu@huawei.com>
To: "Alex Bennée" <alex.bennee@linaro.org>,
	"Peter Maydell" <peter.maydell@linaro.org>
Cc: Eduardo Habkost <ehabkost@redhat.com>,
	kvm-devel <kvm@vger.kernel.org>,
	"Michael S. Tsirkin" <mst@redhat.com>,
	Marc Zyngier <marc.zyngier@arm.com>,
	Marcelo Tosatti <mtosatti@redhat.com>,
	QEMU Developers <qemu-devel@nongnu.org>,
	Shannon Zhao <shannon.zhaosl@gmail.com>,
	"zhengxiang \(A\)" <zhengxiang9@huawei.com>,
	qemu-arm <qemu-arm@nongnu.org>, James Morse <james.morse@arm.com>,
	Paolo Bonzini <pbonzini@redhat.com>,
	Igor Mammedov <imammedo@redhat.com>,
	Laszlo Ersek <lersek@redhat.com>,
	Richard Henderson <rth@twiddle.net>
Subject: Re: [PATCH RESEND v15 08/10] target-arm: kvm64: inject synchronous External Abort
Date: Mon, 26 Nov 2018 17:25:09 +0000	[thread overview]
Message-ID: <0184EA26B2509940AA629AE1405DD7F201F96F02@dggema523-mbx.china.huawei.com> (raw)

> >>
> >> Hi Peter,
> >>   Thanks for the review and comments.
> >>
> >> >
> >> > On 8 November 2018 at 10:29, Dongjiu Geng <gengdongjiu@huawei.com> 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.
> >
> >> 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].
> >
> > 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 ?
> <snip>
> 
> I did not - I was mostly focusing in the host-debugging-the-guest test case. I'll get a test rig up and check.

Thanks, please test it in the KVM mode, not in the TCG mode. Waiting for your test result.

> 
> --
> Alex Bennée