[PATCH] KVM: arm64: stop propagating DAIF flags between kernel and VHE's world switch

[james.morse@arm.com (James Morse)]

Hi Christoffer,

On 30/08/17 09:33, Christoffer Dall wrote:
> On Tue, Aug 29, 2017 at 06:10:57PM +0100, James Morse wrote:
>> On 24/08/17 16:23, Christoffer Dall wrote:
>>> On Thu, Aug 10, 2017 at 12:30:21PM +0100, James Morse wrote:
>>>> debug exceptions remain disabled due to the guest exit exception,
>>>> (as does SError: today this is the only time SError is unmasked in the
>>>> kernel). The flags stay in this state until we return to userspace.
>>>>
>>>> We have a __vhe_hyp_call() function that does the isb that we implicitly
>>>> have on non-vhe systems, add the DAIF save/restore here, instead of in
>>>> __sysreg_{save,restore}_host_state() which would require an extra isb()
>>>> between the hosts VBAR_EL1 being restored and DAIF being restored.
>>
>>> This also means that anyone else calling kvm_call_hyp(), which we are
>>> beginning to do more often, would also do this save/restore which
>>> shouldn't really be necessary, doesn't it?
>>
>> I think it is, its done implicitly via SPSR_EL2 by the HVC/ERET on non-VHE.
>> Does the HYP code on the other end of kvm_call_hyp() expect to be called with
>> DAIF masked? What about the other way, if the HYP code modifies the DAIF flags
>> should that spread back into the kernel?
> 
> Well, for VHE I don't think this is any different than any other
> function.  There really is no concept of 'hyp code' --- or we should aim
> for there not to be --- with VHE, so if some code expects interrupts
> disabled or other changes to the DAIF flags, that code should really do
> that for VHE.

Aha, this is where I see the world differently.
/me adjusts world-view,

I will scrap this patch and re-do it along these lines.


> The rationale being that in the long run we want to keep "jumping to
> hyp" the oddball legacy case, where everything else is just the
> kernel/hypervisor functionality.

This makes sense.


[ ... trims your excellent argument ... ]


> Have we actually looked at the places where we call kvm_call_hyp() and
> do they require a different setting of the DAIF flags from other kernel
> code running at EL2 with VHE ?

I can go through them, but I think its just world-switch as it modifies
debug-registers, vbar_el1 and enters/exits the guest.


> I understand that the behavior is currently different, but what I'm
> asking is, instead of having to save and restore anything to the stack,
> can you simply do:
> 
>      __kvm_vcpu_run(struct kvm_vcpu *vcpu)
>      {
>          if (has_vhe())
>              asm("msr     daifset, #0xf");
> 
> 	...
>         exit_code = __guest_enter(vcpu, host_ctxt);
> 	...
> 
> 	if (has_vhe())
>              asm("msr     daifclr, #0xd");
>      }

Sure, this can be done as late as setting vbar_el1 for VHE, at which point the
reason for masking these is clear. Before this point the hosts vectors will
happily handle debug/fiq/serror.

Your right KVM can 'just know' the values to set so the noise around


> (not sure about the FIQ flag - does the kernel usually 

This is some of the stuff we need to clear up. Today we never expect SError or
FIQ and should panic() if we get one. But these flags are largely ignored so
when the CPU masks them on exception we leave them like that.
After the SError rework process-context should have everything unmasked, today
its just debug+irqs unmasked.


Sorry if this email exchange has been frustrating, I didn't have your view of
where all this should end up.


Thanks,

James