[PATCH v3 3/9] arm64: KVM: add trap handlers for AArch64 debug registers

[marc.zyngier@arm.com (Marc Zyngier)]

On Wed, Jul 09 2014 at  3:52:32 pm BST, Christoffer Dall <christoffer.dall@linaro.org> wrote:
> On Wed, Jul 09, 2014 at 12:09:29PM +0100, Marc Zyngier wrote:
>> On Wed, Jul 09 2014 at 10:38:13 am BST, Christoffer Dall
>> <christoffer.dall@linaro.org> wrote:
>> > On Fri, Jun 20, 2014 at 02:00:01PM +0100, Marc Zyngier wrote:
>> >> Add handlers for all the AArch64 debug registers that are accessible
>> >> from EL0 or EL1. The trapping code keeps track of the state of the
>> >> debug registers, allowing for the switch code to implement a lazy
>> >> switching strategy.
>> >>
>> >> Reviewed-by: Anup Patel <anup.patel@linaro.org>
>> >> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> >> ---
>> >>  arch/arm64/include/asm/kvm_asm.h  |  28 ++++++--
>> >>  arch/arm64/include/asm/kvm_host.h |   3 +
>> >>  arch/arm64/kvm/sys_regs.c         | 137 +++++++++++++++++++++++++++++++++++++-
>> >>  3 files changed, 159 insertions(+), 9 deletions(-)

[...]

>> >> +/*
>> >> + * We want to avoid world-switching all the DBG registers all the
>> >> + * time. For this, we use a DIRTY but, indicating the guest has
>> >
>> > a DIRTY but?  (at least there's only one t in there).
>>
>> The whole debug architecture makes me feel very dirty.
>>
>> >> + * modified the debug registers, and only restore the registers once,
>> >> + * disabling traps.
>> >
>> > I don't think I understand the "only restore the registers once" bit
>> > here.  I know I'm being incredibly stupid, but I forgot since the last
>> > review round how this actually works; when we return from the guest and
>> > the guest has somehow enabled certain DBG functionality, then we
>> > set the dirty
>> > flag, which means we should stop trapping and context switch all the
>> > registers on world-switches, but if we see when returning from the guest
>> > that the guest doesn't appear to be using the registers we enable
>> > trapping and stop world-switching, right?
>>
>> Almost. We always decide on the trapping when entering the guest:
>> - If the dirty bit is set (because we're coming back from trapping),
>>   disable the traps, restore the registers
>> - If debug is actively in use (DBG_MDSCR_KDE or DBG_MDSCR_MDE set),
>>   disable the traps, restore the registers
>
> this also sets the dirty bit then?

Indeed. I'll mention it.

>
>> - Otherwise, enable the traps
>>
>> When exiting the guest: If the dirty bit is set, save the registers and
>> clear the dirty bit.
>
> because the host should always be able to freely use the debug registers
> so we always have to restore the host registers on coming out of the VM,
> right?

Yes. The host may have its own debug state active, and we want to
preserve that.

>>
>> > Do we clearly define which state triggers the world-switching and why
>> > that's a good rationale? (sorry, the debug architecture is not my
>> > favorite part of the ARM ARM).
>>
>> I thing the above comment describes the state precisely. My rational is:
>> - If we've touched any debug register, it is likely that we're going to
>>   touch more of them. It then makes sense to disable the traps and start
>>   doing the save/restore dance
>> - If debug is active (DBG_MDSCR_KDE or DBG_MDSCR_MDE set), it is then
>>   mandatory to save/restore the registers, as the guest depends on them.
>>
>> Does this make the process clearer? If so, I can add it to the comment.
>>
>
> yes, the above comments help a lot.  thanks!!
>
> [if I don't see your response because you already left for vacation, I'm
> going to incorporate your comments in the patches to apply to
> kvmarm/next].

I'm not quite gone yet! ;-) Just enough time left to respin the branch
on top of what's already queued, push it out and post the series.

	M.
-- 
Without deviation from the norm, progress is not possible.