On 22/06/12 21:59, Christoffer Dall wrote: > On Mon, May 14, 2012 at 9:05 AM, Marc Zyngier <marc.zyngier at arm.com> wrote: >> Enable the VGIC control interface to be save-restored on world switch. >> >> Signed-off-by: Marc Zyngier <marc.zyngier at arm.com> >> --- >> arch/arm/include/asm/kvm_arm.h | 10 ++++++ >> arch/arm/kernel/asm-offsets.c | 10 ++++++ >> arch/arm/kvm/interrupts.S | 62 ++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 82 insertions(+), 0 deletions(-) >> >> diff --git a/arch/arm/include/asm/kvm_arm.h b/arch/arm/include/asm/kvm_arm.h >> index b9e4197..ab13a89 100644 >> --- a/arch/arm/include/asm/kvm_arm.h >> +++ b/arch/arm/include/asm/kvm_arm.h >> @@ -128,4 +128,14 @@ >> #define HSR_EC_DABT (0x24) >> #define HSR_EC_DABT_HYP (0x25) >> >> +/* GICH offsets */ >> +#define GICH_HCR 0x0 >> +#define GICH_VTR 0x4 >> +#define GICH_VMCR 0x8 >> +#define GICH_MISR 0x10 >> +#define GICH_ELSR0 0x30 >> +#define GICH_ELSR1 0x34 >> +#define GICH_APR 0xf0 >> +#define GICH_LR0 0x100 >> + >> #endif /* __KVM_ARM_H__ */ >> diff --git a/arch/arm/kernel/asm-offsets.c b/arch/arm/kernel/asm-offsets.c >> index c8c1b91..d5f4ddf 100644 >> --- a/arch/arm/kernel/asm-offsets.c >> +++ b/arch/arm/kernel/asm-offsets.c >> @@ -176,6 +176,16 @@ int main(void) >> DEFINE(VCPU_HIFAR, offsetof(struct kvm_vcpu, arch.hifar)); >> DEFINE(VCPU_HPFAR, offsetof(struct kvm_vcpu, arch.hpfar)); >> DEFINE(VCPU_PC_IPA, offsetof(struct kvm_vcpu, arch.pc_ipa)); >> +#ifdef CONFIG_KVM_ARM_VGIC >> + DEFINE(VCPU_VGIC_HCR, offsetof(struct kvm_vcpu, arch.vgic_cpu.vgic_hcr)); >> + DEFINE(VCPU_VGIC_MCR, offsetof(struct kvm_vcpu, arch.vgic_cpu.vgic_mcr)); >> + DEFINE(VCPU_VGIC_MISR, offsetof(struct kvm_vcpu, arch.vgic_cpu.vgic_misr)); >> + DEFINE(VCPU_VGIC_ELSR, offsetof(struct kvm_vcpu, arch.vgic_cpu.vgic_elsr)); >> + DEFINE(VCPU_VGIC_APR, offsetof(struct kvm_vcpu, arch.vgic_cpu.vgic_apr)); >> + DEFINE(VCPU_VGIC_LR, offsetof(struct kvm_vcpu, arch.vgic_cpu.vgic_lr)); >> + DEFINE(VCPU_VGIC_NR_LR, offsetof(struct kvm_vcpu, arch.vgic_cpu.nr_lr)); >> + DEFINE(KVM_VGIC_VCTRL, offsetof(struct kvm, arch.vgic.vctrl_base)); >> +#endif >> DEFINE(KVM_VTTBR, offsetof(struct kvm, arch.vttbr)); >> #endif >> return 0; >> diff --git a/arch/arm/kvm/interrupts.S b/arch/arm/kvm/interrupts.S >> index 2e19b34..541532b 100644 >> --- a/arch/arm/kvm/interrupts.S >> +++ b/arch/arm/kvm/interrupts.S >> @@ -241,6 +241,40 @@ ENTRY(__kvm_flush_vm_context) >> * @vcpup: Register pointing to VCPU struct >> */ >> .macro save_vgic_state vcpup >> +#ifdef CONFIG_KVM_ARM_VGIC >> + /* Get VGIC VCTRL base into r2 */ >> + ldr r2, [\vcpup, #VCPU_KVM] >> + ldr r2, [r2, #KVM_VGIC_VCTRL] >> + cmp r2, #0 >> + beq 2f >> + >> + /* Save all interesting registers */ >> + ldr r3, [r2, #GICH_HCR] >> + ldr r4, [r2, #GICH_VMCR] >> + ldr r5, [r2, #GICH_MISR] >> + ldr r6, [r2, #GICH_ELSR0] >> + ldr r7, [r2, #GICH_ELSR1] >> + ldr r8, [r2, #GICH_APR] >> + >> + str r3, [\vcpup, #VCPU_VGIC_HCR] >> + str r4, [\vcpup, #VCPU_VGIC_MCR] >> + str r5, [\vcpup, #VCPU_VGIC_MISR] >> + str r6, [\vcpup, #VCPU_VGIC_ELSR] >> + str r7, [\vcpup, #(VCPU_VGIC_ELSR + 4)] >> + str r8, [\vcpup, #VCPU_VGIC_APR] > > shouldn't we be disabling the virtual interface here? (or restore the > host VGICH_HCR)? The host doesn't use the VGIC interface, so there is no need to disable it. I have a fix in my tree that set VGICH_HCR to zero though, as we could end up with the maintenance interrupt firing spuriously otherwise. >> + >> + /* Save list registers */ >> + add r2, r2, #GICH_LR0 >> + add r3, \vcpup, #VCPU_VGIC_LR >> + ldr r4, [\vcpup, #VCPU_VGIC_NR_LR] >> + mov r5, #0 >> +1: ldr r6, [r2], #4 >> + str r6, [r3], #4 >> + add r5, r5, #1 >> + cmp r5, r4 > > see the comment below for saving an instruction > >> + bne 1b >> +2: >> +#endif >> .endm >> >> /* >> @@ -248,6 +282,34 @@ ENTRY(__kvm_flush_vm_context) >> * @vcpup: Register pointing to VCPU struct >> */ >> .macro restore_vgic_state vcpup >> +#ifdef CONFIG_KVM_ARM_VGIC >> + /* Get VGIC VCTRL base into r2 */ >> + ldr r2, [\vcpup, #VCPU_KVM] >> + ldr r2, [r2, #KVM_VGIC_VCTRL] >> + cmp r2, #0 >> + beq 2f >> + >> + /* We only restore a minimal set of registers */ >> + ldr r3, [\vcpup, #VCPU_VGIC_HCR] >> + ldr r4, [\vcpup, #VCPU_VGIC_MCR] >> + ldr r8, [\vcpup, #VCPU_VGIC_APR] >> + >> + str r3, [r2, #GICH_HCR] >> + str r4, [r2, #GICH_VMCR] >> + str r8, [r2, #GICH_APR] >> + >> + /* Restore list registers */ >> + add r2, r2, #GICH_LR0 >> + add r3, \vcpup, #VCPU_VGIC_LR >> + ldr r4, [\vcpup, #VCPU_VGIC_NR_LR] >> + mov r5, #0 >> +1: ldr r6, [r3], #4 >> + str r6, [r2], #4 >> + add r5, r5, #1 >> + cmp r5, r4 > > can't you do this instead: > > + ldr r4, [\vcpup, #VCPU_VGIC_NR_LR] > +1: ldr r6, [r3], #4 > + str r6, [r2], #4 > + subs r4, r4, #1 > > ? Indeed. Nice! >> + bne 1b >> +2: >> +#endif >> .endm >> >> /* Configures the HSTR (Hyp System Trap Register) on entry/return >> -- >> 1.7.7.1 >> > > Overall looks good, I'm concerned about doing all this MMIO work on > every world switch - especially in the case where you have a single > guest more or less dedicated to a single physical CPU, so maybe this > whole thing could be done lazily by keeping some affinity between > vcpus and pcpus, but it's probably premature. I'm not too concerned about the MMIO, really. It is still very close to the core, and probably doesn't cost much. And on A15, we only save 9 registers, and restore 7 of them. VFP save/restore scares me a lot more! ;-) We could certainly do things more lazily. And actually, I think we could even move the save/restore code to the host... M. -- Jazz is not dead. It just smells funny...
