Re: [PATCH 3/5] arm64: KVM: vgic-v2: Add the GICV emulation infrastructure

[Christoffer Dall]

On Thu, Sep 01, 2016 at 03:28:36PM +0100, Marc Zyngier wrote:
> On 01/09/16 13:46, Christoffer Dall wrote:
> > On Fri, Aug 19, 2016 at 01:38:13PM +0100, Marc Zyngier wrote:
> >> In order to efficiently perform the GICV access on behalf of the
> >> guest, we need to be able to do avoid going back all the way to
> > 
> > s/do//
> > 
> >> the host kernel.
> >>
> >> For this, we introduce a new hook in the world switch code,
> >> conveniently placed just after populating the fault info.
> >> At that point, we only have saved/restored the GP registers,
> >> and we can quickly perform all the required checks (data abort,
> >> translation fault, valid faulting syndrome, not an external
> >> abort, not a PTW).
> >>
> >> Coming back from the emulation code, we need to skip the emulated
> >> instruction. This involves an additional bit of save/restore in
> >> order to be able to access the guest's PC (and possibly CPSR if
> >> this is a 32bit guest).
> >>
> >> At this stage, no emulation code is provided.
> >>
> >> Signed-off-by: Marc Zyngier 
> >> ---
> >>  arch/arm64/include/asm/kvm_hyp.h |  1 +
> >>  arch/arm64/kvm/hyp/switch.c  | 32 
> >>  include/kvm/arm_vgic.h   |  3 +++
> >>  virt/kvm/arm/hyp/vgic-v2-sr.c|  7 +++
> >>  virt/kvm/arm/vgic/vgic-v2.c  |  2 ++
> >>  5 files changed, 45 insertions(+)
> >>
> >> diff --git a/arch/arm64/include/asm/kvm_hyp.h 
> >> b/arch/arm64/include/asm/kvm_hyp.h
> >> index cff5105..88ec3ac 100644
> >> --- a/arch/arm64/include/asm/kvm_hyp.h
> >> +++ b/arch/arm64/include/asm/kvm_hyp.h
> >> @@ -123,6 +123,7 @@ typeof(orig) * __hyp_text fname(void)  
> >> \
> >>  
> >>  void __vgic_v2_save_state(struct kvm_vcpu *vcpu);
> >>  void __vgic_v2_restore_state(struct kvm_vcpu *vcpu);
> >> +bool __vgic_v2_perform_cpuif_access(struct kvm_vcpu *vcpu);
> >>  
> >>  void __vgic_v3_save_state(struct kvm_vcpu *vcpu);
> >>  void __vgic_v3_restore_state(struct kvm_vcpu *vcpu);
> >> diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
> >> index ae7855f..0be1594 100644
> >> --- a/arch/arm64/kvm/hyp/switch.c
> >> +++ b/arch/arm64/kvm/hyp/switch.c
> >> @@ -17,6 +17,7 @@
> >>  
> >>  #include 
> >>  #include 
> >> +#include 
> >>  #include 
> >>  
> >>  static bool __hyp_text __fpsimd_enabled_nvhe(void)
> >> @@ -232,6 +233,21 @@ static bool __hyp_text __populate_fault_info(struct 
> >> kvm_vcpu *vcpu)
> >>return true;
> >>  }
> >>  
> >> +static void __hyp_text __skip_instr(struct kvm_vcpu *vcpu)
> >> +{
> >> +  vcpu->arch.ctxt.gp_regs.regs.pc = read_sysreg_el2(elr);
> >> +
> >> +  if (vcpu_mode_is_32bit(vcpu)) {
> >> +  vcpu->arch.ctxt.gp_regs.regs.pstate = read_sysreg_el2(spsr);
> >> +  kvm_skip_aarch32_instr(vcpu, kvm_vcpu_trap_il_is32bit(vcpu));
> >> +  write_sysreg_el2(vcpu->arch.ctxt.gp_regs.regs.pstate, spsr);
> >> +  } else {
> >> +  *vcpu_pc(vcpu) += 4;
> >> +  }
> >> +
> >> +  write_sysreg_el2(vcpu->arch.ctxt.gp_regs.regs.pc, elr);
> >> +}
> >> +
> >>  static int __hyp_text __guest_run(struct kvm_vcpu *vcpu)
> >>  {
> >>struct kvm_cpu_context *host_ctxt;
> >> @@ -270,6 +286,22 @@ again:
> >>if (exit_code == ARM_EXCEPTION_TRAP && !__populate_fault_info(vcpu))
> >>goto again;
> >>  
> >> +  if (static_branch_unlikely(_v2_cpuif_trap) &&
> >> +  exit_code == ARM_EXCEPTION_TRAP) {
> > 
> > do you get the static branch benefit when the test contains an &&
> > clause?  (I haven't looked at the generated assembly, no)
> 
> You do, otherwise the C semantics would be broken. This is strictly
> equivalent to:
> 
>   if (static_branch_unlikely()) {
>   if (exit_code == ...) {
>   [...]
>   }
>   }
> 
> > Also, if you flip this static branch for code both mapped in EL1 and
> > EL2, would you potentially need some form of additional icache
> > maintenance here?
> > 
> > Or are you relying on the static branch being set at boot time and hold
> > forever true/false?
> 
> I asked myself this exact question when I did this, and became convinced
> that this was OK for two reasons:
> - we do it only once
> - when we do it, we haven't executed that code yet, so it cannot be in
> the cache yet
> 
> > 
> >> +  bool valid;
> >> +
> >> +  valid = kvm_vcpu_trap_get_class(vcpu) == ESR_ELx_EC_DABT_LOW &&
> >> +  kvm_vcpu_trap_get_fault_type(vcpu) == FSC_FAULT &&
> >> +  kvm_vcpu_dabt_isvalid(vcpu) &&
> >> +  !kvm_vcpu_dabt_isextabt(vcpu) &&
> >> +  !kvm_vcpu_dabt_iss1tw(vcpu);
> >> +
> >> +  if (valid &&  __vgic_v2_perform_cpuif_access(vcpu)) {
> > 
> > extra whitespace
> > 
> >> +  __skip_instr(vcpu);
> > 
> > does this interact in any amusing way with single-step guest debugging?
> 
> Ouch. Good point. Actually, nothing that uses kvm_skip_instr() works for
> singlestep/watchpoint 

Re: [PATCH 3/5] arm64: KVM: vgic-v2: Add the GICV emulation infrastructure

[Peter Maydell]

On 1 September 2016 at 15:28, Marc Zyngier  wrote:
> On 01/09/16 13:46, Christoffer Dall wrote:
>> On Fri, Aug 19, 2016 at 01:38:13PM +0100, Marc Zyngier wrote:
>>> +__skip_instr(vcpu);
>>
>> does this interact in any amusing way with single-step guest debugging?
>
> Ouch. Good point. Actually, nothing that uses kvm_skip_instr() works for
> singlestep/watchpoint either (trapped sysreg, WFx, spurious traps,
> MMIO). I guess that's something we need to fix overall.

I vaguely remember having conversations about this way way back.
All the "advance one insn" stuff (including how the singlestep
state machine gets advanced) is probably on the wrong side of
"return to userspace".

thanks
-- PMM
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 3/5] arm64: KVM: vgic-v2: Add the GICV emulation infrastructure

[Marc Zyngier]

On 01/09/16 13:46, Christoffer Dall wrote:
> On Fri, Aug 19, 2016 at 01:38:13PM +0100, Marc Zyngier wrote:
>> In order to efficiently perform the GICV access on behalf of the
>> guest, we need to be able to do avoid going back all the way to
> 
> s/do//
> 
>> the host kernel.
>>
>> For this, we introduce a new hook in the world switch code,
>> conveniently placed just after populating the fault info.
>> At that point, we only have saved/restored the GP registers,
>> and we can quickly perform all the required checks (data abort,
>> translation fault, valid faulting syndrome, not an external
>> abort, not a PTW).
>>
>> Coming back from the emulation code, we need to skip the emulated
>> instruction. This involves an additional bit of save/restore in
>> order to be able to access the guest's PC (and possibly CPSR if
>> this is a 32bit guest).
>>
>> At this stage, no emulation code is provided.
>>
>> Signed-off-by: Marc Zyngier 
>> ---
>>  arch/arm64/include/asm/kvm_hyp.h |  1 +
>>  arch/arm64/kvm/hyp/switch.c  | 32 
>>  include/kvm/arm_vgic.h   |  3 +++
>>  virt/kvm/arm/hyp/vgic-v2-sr.c|  7 +++
>>  virt/kvm/arm/vgic/vgic-v2.c  |  2 ++
>>  5 files changed, 45 insertions(+)
>>
>> diff --git a/arch/arm64/include/asm/kvm_hyp.h 
>> b/arch/arm64/include/asm/kvm_hyp.h
>> index cff5105..88ec3ac 100644
>> --- a/arch/arm64/include/asm/kvm_hyp.h
>> +++ b/arch/arm64/include/asm/kvm_hyp.h
>> @@ -123,6 +123,7 @@ typeof(orig) * __hyp_text fname(void)
>> \
>>  
>>  void __vgic_v2_save_state(struct kvm_vcpu *vcpu);
>>  void __vgic_v2_restore_state(struct kvm_vcpu *vcpu);
>> +bool __vgic_v2_perform_cpuif_access(struct kvm_vcpu *vcpu);
>>  
>>  void __vgic_v3_save_state(struct kvm_vcpu *vcpu);
>>  void __vgic_v3_restore_state(struct kvm_vcpu *vcpu);
>> diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
>> index ae7855f..0be1594 100644
>> --- a/arch/arm64/kvm/hyp/switch.c
>> +++ b/arch/arm64/kvm/hyp/switch.c
>> @@ -17,6 +17,7 @@
>>  
>>  #include 
>>  #include 
>> +#include 
>>  #include 
>>  
>>  static bool __hyp_text __fpsimd_enabled_nvhe(void)
>> @@ -232,6 +233,21 @@ static bool __hyp_text __populate_fault_info(struct 
>> kvm_vcpu *vcpu)
>>  return true;
>>  }
>>  
>> +static void __hyp_text __skip_instr(struct kvm_vcpu *vcpu)
>> +{
>> +vcpu->arch.ctxt.gp_regs.regs.pc = read_sysreg_el2(elr);
>> +
>> +if (vcpu_mode_is_32bit(vcpu)) {
>> +vcpu->arch.ctxt.gp_regs.regs.pstate = read_sysreg_el2(spsr);
>> +kvm_skip_aarch32_instr(vcpu, kvm_vcpu_trap_il_is32bit(vcpu));
>> +write_sysreg_el2(vcpu->arch.ctxt.gp_regs.regs.pstate, spsr);
>> +} else {
>> +*vcpu_pc(vcpu) += 4;
>> +}
>> +
>> +write_sysreg_el2(vcpu->arch.ctxt.gp_regs.regs.pc, elr);
>> +}
>> +
>>  static int __hyp_text __guest_run(struct kvm_vcpu *vcpu)
>>  {
>>  struct kvm_cpu_context *host_ctxt;
>> @@ -270,6 +286,22 @@ again:
>>  if (exit_code == ARM_EXCEPTION_TRAP && !__populate_fault_info(vcpu))
>>  goto again;
>>  
>> +if (static_branch_unlikely(_v2_cpuif_trap) &&
>> +exit_code == ARM_EXCEPTION_TRAP) {
> 
> do you get the static branch benefit when the test contains an &&
> clause?  (I haven't looked at the generated assembly, no)

You do, otherwise the C semantics would be broken. This is strictly
equivalent to:

if (static_branch_unlikely()) {
if (exit_code == ...) {
[...]
}
}

> Also, if you flip this static branch for code both mapped in EL1 and
> EL2, would you potentially need some form of additional icache
> maintenance here?
> 
> Or are you relying on the static branch being set at boot time and hold
> forever true/false?

I asked myself this exact question when I did this, and became convinced
that this was OK for two reasons:
- we do it only once
- when we do it, we haven't executed that code yet, so it cannot be in
the cache yet

> 
>> +bool valid;
>> +
>> +valid = kvm_vcpu_trap_get_class(vcpu) == ESR_ELx_EC_DABT_LOW &&
>> +kvm_vcpu_trap_get_fault_type(vcpu) == FSC_FAULT &&
>> +kvm_vcpu_dabt_isvalid(vcpu) &&
>> +!kvm_vcpu_dabt_isextabt(vcpu) &&
>> +!kvm_vcpu_dabt_iss1tw(vcpu);
>> +
>> +if (valid &&  __vgic_v2_perform_cpuif_access(vcpu)) {
> 
> extra whitespace
> 
>> +__skip_instr(vcpu);
> 
> does this interact in any amusing way with single-step guest debugging?

Ouch. Good point. Actually, nothing that uses kvm_skip_instr() works for
singlestep/watchpoint either (trapped sysreg, WFx, spurious traps,
MMIO). I guess that's something we need to fix overall.

Thanks,

M.
-- 
Jazz is not dead. It just smells funny...
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu

[PATCH 3/5] arm64: KVM: vgic-v2: Add the GICV emulation infrastructure

[Marc Zyngier]

In order to efficiently perform the GICV access on behalf of the
guest, we need to be able to do avoid going back all the way to
the host kernel.

For this, we introduce a new hook in the world switch code,
conveniently placed just after populating the fault info.
At that point, we only have saved/restored the GP registers,
and we can quickly perform all the required checks (data abort,
translation fault, valid faulting syndrome, not an external
abort, not a PTW).

Coming back from the emulation code, we need to skip the emulated
instruction. This involves an additional bit of save/restore in
order to be able to access the guest's PC (and possibly CPSR if
this is a 32bit guest).

At this stage, no emulation code is provided.

Signed-off-by: Marc Zyngier 
---
 arch/arm64/include/asm/kvm_hyp.h |  1 +
 arch/arm64/kvm/hyp/switch.c  | 32 
 include/kvm/arm_vgic.h   |  3 +++
 virt/kvm/arm/hyp/vgic-v2-sr.c|  7 +++
 virt/kvm/arm/vgic/vgic-v2.c  |  2 ++
 5 files changed, 45 insertions(+)

diff --git a/arch/arm64/include/asm/kvm_hyp.h b/arch/arm64/include/asm/kvm_hyp.h
index cff5105..88ec3ac 100644
--- a/arch/arm64/include/asm/kvm_hyp.h
+++ b/arch/arm64/include/asm/kvm_hyp.h
@@ -123,6 +123,7 @@ typeof(orig) * __hyp_text fname(void)   
\
 
 void __vgic_v2_save_state(struct kvm_vcpu *vcpu);
 void __vgic_v2_restore_state(struct kvm_vcpu *vcpu);
+bool __vgic_v2_perform_cpuif_access(struct kvm_vcpu *vcpu);
 
 void __vgic_v3_save_state(struct kvm_vcpu *vcpu);
 void __vgic_v3_restore_state(struct kvm_vcpu *vcpu);
diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
index ae7855f..0be1594 100644
--- a/arch/arm64/kvm/hyp/switch.c
+++ b/arch/arm64/kvm/hyp/switch.c
@@ -17,6 +17,7 @@
 
 #include 
 #include 
+#include 
 #include 
 
 static bool __hyp_text __fpsimd_enabled_nvhe(void)
@@ -232,6 +233,21 @@ static bool __hyp_text __populate_fault_info(struct 
kvm_vcpu *vcpu)
return true;
 }
 
+static void __hyp_text __skip_instr(struct kvm_vcpu *vcpu)
+{
+   vcpu->arch.ctxt.gp_regs.regs.pc = read_sysreg_el2(elr);
+
+   if (vcpu_mode_is_32bit(vcpu)) {
+   vcpu->arch.ctxt.gp_regs.regs.pstate = read_sysreg_el2(spsr);
+   kvm_skip_aarch32_instr(vcpu, kvm_vcpu_trap_il_is32bit(vcpu));
+   write_sysreg_el2(vcpu->arch.ctxt.gp_regs.regs.pstate, spsr);
+   } else {
+   *vcpu_pc(vcpu) += 4;
+   }
+
+   write_sysreg_el2(vcpu->arch.ctxt.gp_regs.regs.pc, elr);
+}
+
 static int __hyp_text __guest_run(struct kvm_vcpu *vcpu)
 {
struct kvm_cpu_context *host_ctxt;
@@ -270,6 +286,22 @@ again:
if (exit_code == ARM_EXCEPTION_TRAP && !__populate_fault_info(vcpu))
goto again;
 
+   if (static_branch_unlikely(_v2_cpuif_trap) &&
+   exit_code == ARM_EXCEPTION_TRAP) {
+   bool valid;
+
+   valid = kvm_vcpu_trap_get_class(vcpu) == ESR_ELx_EC_DABT_LOW &&
+   kvm_vcpu_trap_get_fault_type(vcpu) == FSC_FAULT &&
+   kvm_vcpu_dabt_isvalid(vcpu) &&
+   !kvm_vcpu_dabt_isextabt(vcpu) &&
+   !kvm_vcpu_dabt_iss1tw(vcpu);
+
+   if (valid &&  __vgic_v2_perform_cpuif_access(vcpu)) {
+   __skip_instr(vcpu);
+   goto again;
+   }
+   }
+
fp_enabled = __fpsimd_enabled();
 
__sysreg_save_guest_state(guest_ctxt);
diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index 19b698e..8eb1501 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -20,6 +20,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -265,6 +266,8 @@ struct vgic_cpu {
bool lpis_enabled;
 };
 
+extern struct static_key_false vgic_v2_cpuif_trap;
+
 int kvm_vgic_addr(struct kvm *kvm, unsigned long type, u64 *addr, bool write);
 void kvm_vgic_early_init(struct kvm *kvm);
 int kvm_vgic_create(struct kvm *kvm, u32 type);
diff --git a/virt/kvm/arm/hyp/vgic-v2-sr.c b/virt/kvm/arm/hyp/vgic-v2-sr.c
index 7cffd93..3e2a62e 100644
--- a/virt/kvm/arm/hyp/vgic-v2-sr.c
+++ b/virt/kvm/arm/hyp/vgic-v2-sr.c
@@ -167,3 +167,10 @@ void __hyp_text __vgic_v2_restore_state(struct kvm_vcpu 
*vcpu)
writel_relaxed(cpu_if->vgic_vmcr, base + GICH_VMCR);
vcpu->arch.vgic_cpu.live_lrs = live_lrs;
 }
+
+#ifdef CONFIG_ARM64
+bool __hyp_text __vgic_v2_perform_cpuif_access(struct kvm_vcpu *vcpu)
+{
+   return false;
+}
+#endif
diff --git a/virt/kvm/arm/vgic/vgic-v2.c b/virt/kvm/arm/vgic/vgic-v2.c
index 0bf6709..b8da901 100644
--- a/virt/kvm/arm/vgic/vgic-v2.c
+++ b/virt/kvm/arm/vgic/vgic-v2.c
@@ -294,6 +294,8 @@ out:
return ret;
 }
 
+DEFINE_STATIC_KEY_FALSE(vgic_v2_cpuif_trap);
+
 /**
  * vgic_v2_probe - probe for a GICv2 compatible interrupt controller in DT
  * @node:  pointer to the DT node
-- 
2.1.4