Re: xen/arm: Software Step ARMv8 - PC stuck on instruction

[Julien Grall <julien.grall@arm.com>]

On 03/08/17 11:46, James Morse wrote:
> Hi Florian,
>
> On 03/08/17 11:16, Florian Jakobsmeier wrote:
>> This is the current version:
>
> I'm not familiar with Xen, so forgive my annotations:
>
>> asmlinkage void leave_hypervisor_tail(void)
>>>  {
>>> +    /*This methode will be called after the 'guest_entry' macro in
>>> /arch/arm64/entry.S set guest registers
>>> +    Check single_step_enabled flag in domain struct here and set needed
>>> registers
>>> +    */
>>> +
>>> +    struct vcpu *v = current;
>>> +
>>> +    if ( unlikely(v->domain->arch.monitor.singlestep_enabled ) )
>>> +    {
>>> +        if(!(guest_cpu_user_regs()->cpsr & 0b1000))
>
> This tests SPSR.M[4], which is set for exits from AARCH32.
>
>
>>> +        {
>>> +            WRITE_SYSREG(READ_SYSREG(MDSCR_EL1) | 0x1, MDSCR_EL1);
>
> Sets MDSCR.EL1.SS to enable the state machine.
>
>
>>> +            WRITE_SYSREG(READ_SYSREG(MDCR_EL2)  | HDCR_TDE, MDCR_EL2);
>
> Sets TDE to trap debug exceptions to EL2 from lower exception levels. I'm
> surprised this isn't always set for Xen. Do you allow guests to use the debug
> features for their own purposes?

No. We already trap debug exceptions to EL2 by default and MDCR_EL2 
should already be configured correctly for that.

>
>
>>> +            guest_cpu_user_regs()->cpsr = guest_cpu_user_regs()->cpsr |
>>> 0x200000;
>
> Sets SPSR.SS to suppress the step exception in the guest until its executed an
> instruction.
>
>
>>> +            WRITE_SYSREG( READ_SYSREG(DAIF) & ~0x200, DAIF);
>
> Here I'm confused. This looks like you are clearing PSTATE.D from the DAIF
> register for EL2. This means debug exceptions are unmasked for exceptions from
> Xen at EL2.
>
> If you ERET with PSTATE.D clear the SPSR.SS bit will be discarded instead. I
> thin this is your bug. The rules in 'D2.12.4 Entering the active-not-pending
> state' require 'Debug exceptions are disabled from the current Exception level'.
>
> You've set MDSCR_EL1.SS, if you also have MDSCR_EL1.KDE set you will start
> single-stepping Xen once the register writes take effect, (which may be before
> or after this:)
>
>>> +            isb();
>
> What are you synchronising here? (Unless you want to single-step Xen I suspect
> you don't need this at all.)

Xen is been over cautious with isb at the moment :). I think this one is 
not necessary because you will affect EL1/EL0 context and it will be 
synchronized on the eret.

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel