| Message ID | 1448650215-15218-5-git-send-email-marc.zyngier@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Marc Zyngier <marc.zyngier@arm.com> writes: > Implement the vgic-v3 save restore as a direct translation of > the assembly code version. > > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> > --- > arch/arm64/kvm/hyp/Makefile | 1 + > arch/arm64/kvm/hyp/hyp.h | 3 + > arch/arm64/kvm/hyp/vgic-v3-sr.c | 222 ++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 226 insertions(+) > create mode 100644 arch/arm64/kvm/hyp/vgic-v3-sr.c > > diff --git a/arch/arm64/kvm/hyp/Makefile b/arch/arm64/kvm/hyp/Makefile > index d8d5968..d1e38ce 100644 > --- a/arch/arm64/kvm/hyp/Makefile > +++ b/arch/arm64/kvm/hyp/Makefile > @@ -3,3 +3,4 @@ > # > > obj-$(CONFIG_KVM_ARM_HOST) += vgic-v2-sr.o > +obj-$(CONFIG_KVM_ARM_HOST) += vgic-v3-sr.o > diff --git a/arch/arm64/kvm/hyp/hyp.h b/arch/arm64/kvm/hyp/hyp.h > index 78f25c4..a31cb6e 100644 > --- a/arch/arm64/kvm/hyp/hyp.h > +++ b/arch/arm64/kvm/hyp/hyp.h > @@ -30,5 +30,8 @@ > void __vgic_v2_save_state(struct kvm_vcpu *vcpu); > void __vgic_v2_restore_state(struct kvm_vcpu *vcpu); > > +void __vgic_v3_save_state(struct kvm_vcpu *vcpu); > +void __vgic_v3_restore_state(struct kvm_vcpu *vcpu); > + > #endif /* __ARM64_KVM_HYP_H__ */ > > diff --git a/arch/arm64/kvm/hyp/vgic-v3-sr.c b/arch/arm64/kvm/hyp/vgic-v3-sr.c > new file mode 100644 > index 0000000..b490db5 > --- /dev/null > +++ b/arch/arm64/kvm/hyp/vgic-v3-sr.c > @@ -0,0 +1,222 @@ > +/* > + * Copyright (C) 2012-2015 - ARM Ltd > + * Author: Marc Zyngier <marc.zyngier@arm.com> > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License > + * along with this program. If not, see <http://www.gnu.org/licenses/>. > + */ > + > +#include <linux/compiler.h> > +#include <linux/irqchip/arm-gic-v3.h> This include starts spitting out compiler warnings due to use of undefined barrier primitives. I'm not sure where the best place to: #include <asm/barrier.h> is. I added it to: arch/arm64/include/asm/arch_gicv3.h > +#include <linux/kvm_host.h> > + > +#include <asm/kvm_mmu.h> > + > +#include "hyp.h" > + > +/* > + * We store LRs in reverse order to let the CPU deal with streaming > + * access. Use this macro to make it look saner... > + */ > +#define LR_OFFSET(n) (15 - n) > + > +#define read_gicreg(r) \ > + ({ \ > + u64 reg; \ > + asm volatile("mrs_s %0, " __stringify(r) : "=r" (reg)); \ > + reg; \ > + }) > + > +#define write_gicreg(v,r) \ > + do { \ > + u64 __val = (v); \ > + asm volatile("msr_s " __stringify(r) ", %0" : : "r" (__val));\ > + } while (0) > + > +/* vcpu is already in the HYP VA space */ > +void __hyp_text __vgic_v3_save_state(struct kvm_vcpu *vcpu) > +{ > + struct vgic_v3_cpu_if *cpu_if = &vcpu->arch.vgic_cpu.vgic_v3; > + u64 val; > + u32 nr_lr, nr_pri; > + > + /* > + * Make sure stores to the GIC via the memory mapped interface > + * are now visible to the system register interface. > + */ > + dsb(st); > + > + cpu_if->vgic_vmcr = read_gicreg(ICH_VMCR_EL2); > + cpu_if->vgic_misr = read_gicreg(ICH_MISR_EL2); > + cpu_if->vgic_eisr = read_gicreg(ICH_EISR_EL2); > + cpu_if->vgic_elrsr = read_gicreg(ICH_ELSR_EL2); > + > + write_gicreg(0, ICH_HCR_EL2); > + val = read_gicreg(ICH_VTR_EL2); > + nr_lr = val & 0xf; > + nr_pri = ((u32)val >> 29) + 1; > + > + switch (nr_lr) { > + case 15: > + cpu_if->vgic_lr[LR_OFFSET(15)] = read_gicreg(ICH_LR15_EL2); > + case 14: > + cpu_if->vgic_lr[LR_OFFSET(14)] = read_gicreg(ICH_LR14_EL2); > + case 13: > + cpu_if->vgic_lr[LR_OFFSET(13)] = read_gicreg(ICH_LR13_EL2); > + case 12: > + cpu_if->vgic_lr[LR_OFFSET(12)] = read_gicreg(ICH_LR12_EL2); > + case 11: > + cpu_if->vgic_lr[LR_OFFSET(11)] = read_gicreg(ICH_LR11_EL2); > + case 10: > + cpu_if->vgic_lr[LR_OFFSET(10)] = read_gicreg(ICH_LR10_EL2); > + case 9: > + cpu_if->vgic_lr[LR_OFFSET(9)] = read_gicreg(ICH_LR9_EL2); > + case 8: > + cpu_if->vgic_lr[LR_OFFSET(8)] = read_gicreg(ICH_LR8_EL2); > + case 7: > + cpu_if->vgic_lr[LR_OFFSET(7)] = read_gicreg(ICH_LR7_EL2); > + case 6: > + cpu_if->vgic_lr[LR_OFFSET(6)] = read_gicreg(ICH_LR6_EL2); > + case 5: > + cpu_if->vgic_lr[LR_OFFSET(5)] = read_gicreg(ICH_LR5_EL2); > + case 4: > + cpu_if->vgic_lr[LR_OFFSET(4)] = read_gicreg(ICH_LR4_EL2); > + case 3: > + cpu_if->vgic_lr[LR_OFFSET(3)] = read_gicreg(ICH_LR3_EL2); > + case 2: > + cpu_if->vgic_lr[LR_OFFSET(2)] = read_gicreg(ICH_LR2_EL2); > + case 1: > + cpu_if->vgic_lr[LR_OFFSET(1)] = read_gicreg(ICH_LR1_EL2); > + case 0: > + cpu_if->vgic_lr[LR_OFFSET(0)] = read_gicreg(ICH_LR0_EL2); > + } > + > + switch (nr_pri) { > + case 7: > + cpu_if->vgic_ap0r[3] = read_gicreg(ICH_AP0R3_EL2); > + cpu_if->vgic_ap0r[2] = read_gicreg(ICH_AP0R2_EL2); > + case 6: > + cpu_if->vgic_ap0r[1] = read_gicreg(ICH_AP0R1_EL2); > + default: > + cpu_if->vgic_ap0r[0] = read_gicreg(ICH_AP0R0_EL2); > + } > + > + switch (nr_pri) { > + case 7: > + cpu_if->vgic_ap1r[3] = read_gicreg(ICH_AP1R3_EL2); > + cpu_if->vgic_ap1r[2] = read_gicreg(ICH_AP1R2_EL2); > + case 6: > + cpu_if->vgic_ap1r[1] = read_gicreg(ICH_AP1R1_EL2); > + default: > + cpu_if->vgic_ap1r[0] = read_gicreg(ICH_AP1R0_EL2); > + } > + > + write_gicreg(read_gicreg(ICC_SRE_EL2) | ICC_SRE_EL2_ENABLE, > + ICC_SRE_EL2); > + isb(); > + write_gicreg(1, ICC_SRE_EL1); > +} > + > +void __hyp_text __vgic_v3_restore_state(struct kvm_vcpu *vcpu) > +{ > + struct vgic_v3_cpu_if *cpu_if = &vcpu->arch.vgic_cpu.vgic_v3; > + u64 val; > + u32 nr_lr, nr_pri; > + > + /* Make sure SRE is valid before writing the other registers */ > + write_gicreg(cpu_if->vgic_sre, ICC_SRE_EL1); > + isb(); > + > + write_gicreg(cpu_if->vgic_hcr, ICH_HCR_EL2); > + write_gicreg(cpu_if->vgic_vmcr, ICH_VMCR_EL2); > + > + val = read_gicreg(ICH_VTR_EL2); > + nr_lr = val & 0xf; > + nr_pri = ((u32)val >> 29) + 1; > + > + switch (nr_pri) { > + case 7: > + write_gicreg(cpu_if->vgic_ap1r[3], ICH_AP1R3_EL2); > + write_gicreg(cpu_if->vgic_ap1r[2], ICH_AP1R2_EL2); > + case 6: > + write_gicreg(cpu_if->vgic_ap1r[1], ICH_AP1R1_EL2); > + default: > + write_gicreg(cpu_if->vgic_ap1r[0], ICH_AP1R0_EL2); > + } > + > + switch (nr_pri) { > + case 7: > + write_gicreg(cpu_if->vgic_ap0r[3], ICH_AP0R3_EL2); > + write_gicreg(cpu_if->vgic_ap0r[2], ICH_AP0R2_EL2); > + case 6: > + write_gicreg(cpu_if->vgic_ap0r[1], ICH_AP0R1_EL2); > + default: > + write_gicreg(cpu_if->vgic_ap0r[0], ICH_AP0R0_EL2); > + } > + > + switch (nr_lr) { > + case 15: > + write_gicreg(cpu_if->vgic_lr[LR_OFFSET(15)], ICH_LR15_EL2); > + case 14: > + write_gicreg(cpu_if->vgic_lr[LR_OFFSET(14)], ICH_LR14_EL2); > + case 13: > + write_gicreg(cpu_if->vgic_lr[LR_OFFSET(13)], ICH_LR13_EL2); > + case 12: > + write_gicreg(cpu_if->vgic_lr[LR_OFFSET(12)], ICH_LR12_EL2); > + case 11: > + write_gicreg(cpu_if->vgic_lr[LR_OFFSET(11)], ICH_LR11_EL2); > + case 10: > + write_gicreg(cpu_if->vgic_lr[LR_OFFSET(10)], ICH_LR10_EL2); > + case 9: > + write_gicreg(cpu_if->vgic_lr[LR_OFFSET(9)], ICH_LR9_EL2); > + case 8: > + write_gicreg(cpu_if->vgic_lr[LR_OFFSET(8)], ICH_LR8_EL2); > + case 7: > + write_gicreg(cpu_if->vgic_lr[LR_OFFSET(7)], ICH_LR7_EL2); > + case 6: > + write_gicreg(cpu_if->vgic_lr[LR_OFFSET(6)], ICH_LR6_EL2); > + case 5: > + write_gicreg(cpu_if->vgic_lr[LR_OFFSET(5)], ICH_LR5_EL2); > + case 4: > + write_gicreg(cpu_if->vgic_lr[LR_OFFSET(4)], ICH_LR4_EL2); > + case 3: > + write_gicreg(cpu_if->vgic_lr[LR_OFFSET(3)], ICH_LR3_EL2); > + case 2: > + write_gicreg(cpu_if->vgic_lr[LR_OFFSET(2)], ICH_LR2_EL2); > + case 1: > + write_gicreg(cpu_if->vgic_lr[LR_OFFSET(1)], ICH_LR1_EL2); > + case 0: > + write_gicreg(cpu_if->vgic_lr[LR_OFFSET(0)], ICH_LR0_EL2); > + } > + > + /* > + * Ensure that the above will have reached the > + * (re)distributors. This ensure the guest will read the > + * correct values from the memory-mapped interface. > + */ > + isb(); > + dsb(sy); > + > + /* > + * Prevent the guest from touching the GIC system registers if > + * SRE isn't enabled for GICv3 emulation. > + */ > + if (!cpu_if->vgic_sre) { > + write_gicreg(read_gicreg(ICC_SRE_EL2) & ~ICC_SRE_EL2_ENABLE, > + ICC_SRE_EL2); > + } > +} > + > +u64 __hyp_text __vgic_v3_read_ich_vtr_el2(void) > +{ > + return read_gicreg(ICH_VTR_EL2); > +} -- Alex Bennée -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 30 Nov 2015 09:59:32 +0000 Alex Bennée <alex.bennee@linaro.org> wrote: > > Marc Zyngier <marc.zyngier@arm.com> writes: > > > Implement the vgic-v3 save restore as a direct translation of > > the assembly code version. > > > > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> > > --- > > arch/arm64/kvm/hyp/Makefile | 1 + > > arch/arm64/kvm/hyp/hyp.h | 3 + > > arch/arm64/kvm/hyp/vgic-v3-sr.c | 222 ++++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 226 insertions(+) > > create mode 100644 arch/arm64/kvm/hyp/vgic-v3-sr.c > > > > diff --git a/arch/arm64/kvm/hyp/Makefile b/arch/arm64/kvm/hyp/Makefile > > index d8d5968..d1e38ce 100644 > > --- a/arch/arm64/kvm/hyp/Makefile > > +++ b/arch/arm64/kvm/hyp/Makefile > > @@ -3,3 +3,4 @@ > > # > > > > obj-$(CONFIG_KVM_ARM_HOST) += vgic-v2-sr.o > > +obj-$(CONFIG_KVM_ARM_HOST) += vgic-v3-sr.o > > diff --git a/arch/arm64/kvm/hyp/hyp.h b/arch/arm64/kvm/hyp/hyp.h > > index 78f25c4..a31cb6e 100644 > > --- a/arch/arm64/kvm/hyp/hyp.h > > +++ b/arch/arm64/kvm/hyp/hyp.h > > @@ -30,5 +30,8 @@ > > void __vgic_v2_save_state(struct kvm_vcpu *vcpu); > > void __vgic_v2_restore_state(struct kvm_vcpu *vcpu); > > > > +void __vgic_v3_save_state(struct kvm_vcpu *vcpu); > > +void __vgic_v3_restore_state(struct kvm_vcpu *vcpu); > > + > > #endif /* __ARM64_KVM_HYP_H__ */ > > > > diff --git a/arch/arm64/kvm/hyp/vgic-v3-sr.c b/arch/arm64/kvm/hyp/vgic-v3-sr.c > > new file mode 100644 > > index 0000000..b490db5 > > --- /dev/null > > +++ b/arch/arm64/kvm/hyp/vgic-v3-sr.c > > @@ -0,0 +1,222 @@ > > +/* > > + * Copyright (C) 2012-2015 - ARM Ltd > > + * Author: Marc Zyngier <marc.zyngier@arm.com> > > + * > > + * This program is free software; you can redistribute it and/or modify > > + * it under the terms of the GNU General Public License version 2 as > > + * published by the Free Software Foundation. > > + * > > + * This program is distributed in the hope that it will be useful, > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > + * GNU General Public License for more details. > > + * > > + * You should have received a copy of the GNU General Public License > > + * along with this program. If not, see <http://www.gnu.org/licenses/>. > > + */ > > + > > +#include <linux/compiler.h> > > +#include <linux/irqchip/arm-gic-v3.h> > > This include starts spitting out compiler warnings due to use of > undefined barrier primitives. I'm not sure where the best place to: > > #include <asm/barrier.h> > > is. I added it to: > > arch/arm64/include/asm/arch_gicv3.h I already have a couple of fixes queued to that effect in my tree, hopefully heading for 4.4-rc4. If you pull the branch I have on korg, you'll get the whole thing that should compile without warning. Thanks, M.
On Fri, Nov 27, 2015 at 06:49:58PM +0000, Marc Zyngier wrote: > Implement the vgic-v3 save restore as a direct translation of > the assembly code version. > > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> > --- > arch/arm64/kvm/hyp/Makefile | 1 + > arch/arm64/kvm/hyp/hyp.h | 3 + > arch/arm64/kvm/hyp/vgic-v3-sr.c | 222 ++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 226 insertions(+) > create mode 100644 arch/arm64/kvm/hyp/vgic-v3-sr.c > > diff --git a/arch/arm64/kvm/hyp/Makefile b/arch/arm64/kvm/hyp/Makefile > index d8d5968..d1e38ce 100644 > --- a/arch/arm64/kvm/hyp/Makefile > +++ b/arch/arm64/kvm/hyp/Makefile > @@ -3,3 +3,4 @@ > # > > obj-$(CONFIG_KVM_ARM_HOST) += vgic-v2-sr.o > +obj-$(CONFIG_KVM_ARM_HOST) += vgic-v3-sr.o > diff --git a/arch/arm64/kvm/hyp/hyp.h b/arch/arm64/kvm/hyp/hyp.h > index 78f25c4..a31cb6e 100644 > --- a/arch/arm64/kvm/hyp/hyp.h > +++ b/arch/arm64/kvm/hyp/hyp.h > @@ -30,5 +30,8 @@ > void __vgic_v2_save_state(struct kvm_vcpu *vcpu); > void __vgic_v2_restore_state(struct kvm_vcpu *vcpu); > > +void __vgic_v3_save_state(struct kvm_vcpu *vcpu); > +void __vgic_v3_restore_state(struct kvm_vcpu *vcpu); > + > #endif /* __ARM64_KVM_HYP_H__ */ > > diff --git a/arch/arm64/kvm/hyp/vgic-v3-sr.c b/arch/arm64/kvm/hyp/vgic-v3-sr.c > new file mode 100644 > index 0000000..b490db5 > --- /dev/null > +++ b/arch/arm64/kvm/hyp/vgic-v3-sr.c > @@ -0,0 +1,222 @@ > +/* > + * Copyright (C) 2012-2015 - ARM Ltd > + * Author: Marc Zyngier <marc.zyngier@arm.com> > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License > + * along with this program. If not, see <http://www.gnu.org/licenses/>. > + */ > + > +#include <linux/compiler.h> > +#include <linux/irqchip/arm-gic-v3.h> > +#include <linux/kvm_host.h> > + > +#include <asm/kvm_mmu.h> > + > +#include "hyp.h" > + > +/* > + * We store LRs in reverse order to let the CPU deal with streaming > + * access. Use this macro to make it look saner... > + */ > +#define LR_OFFSET(n) (15 - n) > + > +#define read_gicreg(r) \ > + ({ \ > + u64 reg; \ > + asm volatile("mrs_s %0, " __stringify(r) : "=r" (reg)); \ > + reg; \ > + }) > + > +#define write_gicreg(v,r) \ > + do { \ > + u64 __val = (v); \ > + asm volatile("msr_s " __stringify(r) ", %0" : : "r" (__val));\ > + } while (0) remind me what the msr_s and mrs_s do compared to msr and mrs? are these the reason why we need separate macros to access the gic registers compared to 'normal' sysregs? > + > +/* vcpu is already in the HYP VA space */ > +void __hyp_text __vgic_v3_save_state(struct kvm_vcpu *vcpu) > +{ > + struct vgic_v3_cpu_if *cpu_if = &vcpu->arch.vgic_cpu.vgic_v3; > + u64 val; > + u32 nr_lr, nr_pri; > + > + /* > + * Make sure stores to the GIC via the memory mapped interface > + * are now visible to the system register interface. > + */ > + dsb(st); > + > + cpu_if->vgic_vmcr = read_gicreg(ICH_VMCR_EL2); > + cpu_if->vgic_misr = read_gicreg(ICH_MISR_EL2); > + cpu_if->vgic_eisr = read_gicreg(ICH_EISR_EL2); > + cpu_if->vgic_elrsr = read_gicreg(ICH_ELSR_EL2); > + > + write_gicreg(0, ICH_HCR_EL2); > + val = read_gicreg(ICH_VTR_EL2); > + nr_lr = val & 0xf; this is not technically nr_lr, it's max_lr or max_lr_idx or something like that. > + nr_pri = ((u32)val >> 29) + 1; nit: nr_pri_bits > + > + switch (nr_lr) { > + case 15: > + cpu_if->vgic_lr[LR_OFFSET(15)] = read_gicreg(ICH_LR15_EL2); > + case 14: > + cpu_if->vgic_lr[LR_OFFSET(14)] = read_gicreg(ICH_LR14_EL2); > + case 13: > + cpu_if->vgic_lr[LR_OFFSET(13)] = read_gicreg(ICH_LR13_EL2); > + case 12: > + cpu_if->vgic_lr[LR_OFFSET(12)] = read_gicreg(ICH_LR12_EL2); > + case 11: > + cpu_if->vgic_lr[LR_OFFSET(11)] = read_gicreg(ICH_LR11_EL2); > + case 10: > + cpu_if->vgic_lr[LR_OFFSET(10)] = read_gicreg(ICH_LR10_EL2); > + case 9: > + cpu_if->vgic_lr[LR_OFFSET(9)] = read_gicreg(ICH_LR9_EL2); > + case 8: > + cpu_if->vgic_lr[LR_OFFSET(8)] = read_gicreg(ICH_LR8_EL2); > + case 7: > + cpu_if->vgic_lr[LR_OFFSET(7)] = read_gicreg(ICH_LR7_EL2); > + case 6: > + cpu_if->vgic_lr[LR_OFFSET(6)] = read_gicreg(ICH_LR6_EL2); > + case 5: > + cpu_if->vgic_lr[LR_OFFSET(5)] = read_gicreg(ICH_LR5_EL2); > + case 4: > + cpu_if->vgic_lr[LR_OFFSET(4)] = read_gicreg(ICH_LR4_EL2); > + case 3: > + cpu_if->vgic_lr[LR_OFFSET(3)] = read_gicreg(ICH_LR3_EL2); > + case 2: > + cpu_if->vgic_lr[LR_OFFSET(2)] = read_gicreg(ICH_LR2_EL2); > + case 1: > + cpu_if->vgic_lr[LR_OFFSET(1)] = read_gicreg(ICH_LR1_EL2); > + case 0: > + cpu_if->vgic_lr[LR_OFFSET(0)] = read_gicreg(ICH_LR0_EL2); I don't understand this; LR_OFFSET(0) == (15 - 0) == 15, so cpu_if->vgic_lr[15] = read_gicreg(ICH_LR0_EL2) ? > + } > + > + switch (nr_pri) { > + case 7: > + cpu_if->vgic_ap0r[3] = read_gicreg(ICH_AP0R3_EL2); > + cpu_if->vgic_ap0r[2] = read_gicreg(ICH_AP0R2_EL2); > + case 6: > + cpu_if->vgic_ap0r[1] = read_gicreg(ICH_AP0R1_EL2); > + default: > + cpu_if->vgic_ap0r[0] = read_gicreg(ICH_AP0R0_EL2); > + } > + > + switch (nr_pri) { > + case 7: > + cpu_if->vgic_ap1r[3] = read_gicreg(ICH_AP1R3_EL2); > + cpu_if->vgic_ap1r[2] = read_gicreg(ICH_AP1R2_EL2); > + case 6: > + cpu_if->vgic_ap1r[1] = read_gicreg(ICH_AP1R1_EL2); > + default: > + cpu_if->vgic_ap1r[0] = read_gicreg(ICH_AP1R0_EL2); > + } > + > + write_gicreg(read_gicreg(ICC_SRE_EL2) | ICC_SRE_EL2_ENABLE, > + ICC_SRE_EL2); nit: reading this out in a variable probably looks nicer. > + isb(); nit: should we comment on why the isb is required? > + write_gicreg(1, ICC_SRE_EL1); > +} > + > +void __hyp_text __vgic_v3_restore_state(struct kvm_vcpu *vcpu) > +{ > + struct vgic_v3_cpu_if *cpu_if = &vcpu->arch.vgic_cpu.vgic_v3; > + u64 val; > + u32 nr_lr, nr_pri; > + > + /* Make sure SRE is valid before writing the other registers */ I know that I've reviewed this code before (the asm version) but coming back to it I have no idea what the above really means... > + write_gicreg(cpu_if->vgic_sre, ICC_SRE_EL1); > + isb(); > + > + write_gicreg(cpu_if->vgic_hcr, ICH_HCR_EL2); > + write_gicreg(cpu_if->vgic_vmcr, ICH_VMCR_EL2); > + > + val = read_gicreg(ICH_VTR_EL2); > + nr_lr = val & 0xf; > + nr_pri = ((u32)val >> 29) + 1; you could have a define for this shift now that you use it more than once. > + > + switch (nr_pri) { > + case 7: > + write_gicreg(cpu_if->vgic_ap1r[3], ICH_AP1R3_EL2); > + write_gicreg(cpu_if->vgic_ap1r[2], ICH_AP1R2_EL2); > + case 6: > + write_gicreg(cpu_if->vgic_ap1r[1], ICH_AP1R1_EL2); > + default: > + write_gicreg(cpu_if->vgic_ap1r[0], ICH_AP1R0_EL2); > + } > + > + switch (nr_pri) { > + case 7: > + write_gicreg(cpu_if->vgic_ap0r[3], ICH_AP0R3_EL2); > + write_gicreg(cpu_if->vgic_ap0r[2], ICH_AP0R2_EL2); > + case 6: > + write_gicreg(cpu_if->vgic_ap0r[1], ICH_AP0R1_EL2); > + default: > + write_gicreg(cpu_if->vgic_ap0r[0], ICH_AP0R0_EL2); > + } > + > + switch (nr_lr) { > + case 15: > + write_gicreg(cpu_if->vgic_lr[LR_OFFSET(15)], ICH_LR15_EL2); > + case 14: > + write_gicreg(cpu_if->vgic_lr[LR_OFFSET(14)], ICH_LR14_EL2); > + case 13: > + write_gicreg(cpu_if->vgic_lr[LR_OFFSET(13)], ICH_LR13_EL2); > + case 12: > + write_gicreg(cpu_if->vgic_lr[LR_OFFSET(12)], ICH_LR12_EL2); > + case 11: > + write_gicreg(cpu_if->vgic_lr[LR_OFFSET(11)], ICH_LR11_EL2); > + case 10: > + write_gicreg(cpu_if->vgic_lr[LR_OFFSET(10)], ICH_LR10_EL2); > + case 9: > + write_gicreg(cpu_if->vgic_lr[LR_OFFSET(9)], ICH_LR9_EL2); > + case 8: > + write_gicreg(cpu_if->vgic_lr[LR_OFFSET(8)], ICH_LR8_EL2); > + case 7: > + write_gicreg(cpu_if->vgic_lr[LR_OFFSET(7)], ICH_LR7_EL2); > + case 6: > + write_gicreg(cpu_if->vgic_lr[LR_OFFSET(6)], ICH_LR6_EL2); > + case 5: > + write_gicreg(cpu_if->vgic_lr[LR_OFFSET(5)], ICH_LR5_EL2); > + case 4: > + write_gicreg(cpu_if->vgic_lr[LR_OFFSET(4)], ICH_LR4_EL2); > + case 3: > + write_gicreg(cpu_if->vgic_lr[LR_OFFSET(3)], ICH_LR3_EL2); > + case 2: > + write_gicreg(cpu_if->vgic_lr[LR_OFFSET(2)], ICH_LR2_EL2); > + case 1: > + write_gicreg(cpu_if->vgic_lr[LR_OFFSET(1)], ICH_LR1_EL2); > + case 0: > + write_gicreg(cpu_if->vgic_lr[LR_OFFSET(0)], ICH_LR0_EL2); same question as above. > + } loads of trailing whitespace all over above > + > + /* > + * Ensure that the above will have reached the > + * (re)distributors. This ensure the guest will read the s/ensure/ensures/ > + * correct values from the memory-mapped interface. > + */ > + isb(); > + dsb(sy); > + > + /* > + * Prevent the guest from touching the GIC system registers if > + * SRE isn't enabled for GICv3 emulation. so can we emulate a GICv3 to the guest without system register access? I.e. an MMIO only GICv3 interface? > + */ > + if (!cpu_if->vgic_sre) { > + write_gicreg(read_gicreg(ICC_SRE_EL2) & ~ICC_SRE_EL2_ENABLE, > + ICC_SRE_EL2); > + } > +} > + > +u64 __hyp_text __vgic_v3_read_ich_vtr_el2(void) > +{ > + return read_gicreg(ICH_VTR_EL2); > +} > -- > 2.1.4 > As for translating the assembly code to C, this patch looks correct. Nevertheless, I felt obliged to ask into the details above, now when you're touching all this code. -Christoffer -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 30/11/15 19:50, Christoffer Dall wrote: > On Fri, Nov 27, 2015 at 06:49:58PM +0000, Marc Zyngier wrote: >> Implement the vgic-v3 save restore as a direct translation of >> the assembly code version. >> >> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> >> --- >> arch/arm64/kvm/hyp/Makefile | 1 + >> arch/arm64/kvm/hyp/hyp.h | 3 + >> arch/arm64/kvm/hyp/vgic-v3-sr.c | 222 ++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 226 insertions(+) >> create mode 100644 arch/arm64/kvm/hyp/vgic-v3-sr.c >> >> diff --git a/arch/arm64/kvm/hyp/Makefile b/arch/arm64/kvm/hyp/Makefile >> index d8d5968..d1e38ce 100644 >> --- a/arch/arm64/kvm/hyp/Makefile >> +++ b/arch/arm64/kvm/hyp/Makefile >> @@ -3,3 +3,4 @@ >> # >> >> obj-$(CONFIG_KVM_ARM_HOST) += vgic-v2-sr.o >> +obj-$(CONFIG_KVM_ARM_HOST) += vgic-v3-sr.o >> diff --git a/arch/arm64/kvm/hyp/hyp.h b/arch/arm64/kvm/hyp/hyp.h >> index 78f25c4..a31cb6e 100644 >> --- a/arch/arm64/kvm/hyp/hyp.h >> +++ b/arch/arm64/kvm/hyp/hyp.h >> @@ -30,5 +30,8 @@ >> void __vgic_v2_save_state(struct kvm_vcpu *vcpu); >> void __vgic_v2_restore_state(struct kvm_vcpu *vcpu); >> >> +void __vgic_v3_save_state(struct kvm_vcpu *vcpu); >> +void __vgic_v3_restore_state(struct kvm_vcpu *vcpu); >> + >> #endif /* __ARM64_KVM_HYP_H__ */ >> >> diff --git a/arch/arm64/kvm/hyp/vgic-v3-sr.c b/arch/arm64/kvm/hyp/vgic-v3-sr.c >> new file mode 100644 >> index 0000000..b490db5 >> --- /dev/null >> +++ b/arch/arm64/kvm/hyp/vgic-v3-sr.c >> @@ -0,0 +1,222 @@ >> +/* >> + * Copyright (C) 2012-2015 - ARM Ltd >> + * Author: Marc Zyngier <marc.zyngier@arm.com> >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License version 2 as >> + * published by the Free Software Foundation. >> + * >> + * This program is distributed in the hope that it will be useful, >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> + * GNU General Public License for more details. >> + * >> + * You should have received a copy of the GNU General Public License >> + * along with this program. If not, see <http://www.gnu.org/licenses/>. >> + */ >> + >> +#include <linux/compiler.h> >> +#include <linux/irqchip/arm-gic-v3.h> >> +#include <linux/kvm_host.h> >> + >> +#include <asm/kvm_mmu.h> >> + >> +#include "hyp.h" >> + >> +/* >> + * We store LRs in reverse order to let the CPU deal with streaming >> + * access. Use this macro to make it look saner... >> + */ >> +#define LR_OFFSET(n) (15 - n) >> + >> +#define read_gicreg(r) \ >> + ({ \ >> + u64 reg; \ >> + asm volatile("mrs_s %0, " __stringify(r) : "=r" (reg)); \ >> + reg; \ >> + }) >> + >> +#define write_gicreg(v,r) \ >> + do { \ >> + u64 __val = (v); \ >> + asm volatile("msr_s " __stringify(r) ", %0" : : "r" (__val));\ >> + } while (0) > > remind me what the msr_s and mrs_s do compared to msr and mrs? They do the same job, only for the system registers which are not in the original ARMv8 architecture spec, and most likely not implemented by old(er) compilers. > are these the reason why we need separate macros to access the gic > registers compared to 'normal' sysregs? Indeed. >> + >> +/* vcpu is already in the HYP VA space */ >> +void __hyp_text __vgic_v3_save_state(struct kvm_vcpu *vcpu) >> +{ >> + struct vgic_v3_cpu_if *cpu_if = &vcpu->arch.vgic_cpu.vgic_v3; >> + u64 val; >> + u32 nr_lr, nr_pri; >> + >> + /* >> + * Make sure stores to the GIC via the memory mapped interface >> + * are now visible to the system register interface. >> + */ >> + dsb(st); >> + >> + cpu_if->vgic_vmcr = read_gicreg(ICH_VMCR_EL2); >> + cpu_if->vgic_misr = read_gicreg(ICH_MISR_EL2); >> + cpu_if->vgic_eisr = read_gicreg(ICH_EISR_EL2); >> + cpu_if->vgic_elrsr = read_gicreg(ICH_ELSR_EL2); >> + >> + write_gicreg(0, ICH_HCR_EL2); >> + val = read_gicreg(ICH_VTR_EL2); >> + nr_lr = val & 0xf; > > this is not technically nr_lr, it's max_lr or max_lr_idx or something > like that. Let's go for max_lr_idx then. >> + nr_pri = ((u32)val >> 29) + 1; > > nit: nr_pri_bits > >> + >> + switch (nr_lr) { >> + case 15: >> + cpu_if->vgic_lr[LR_OFFSET(15)] = read_gicreg(ICH_LR15_EL2); >> + case 14: >> + cpu_if->vgic_lr[LR_OFFSET(14)] = read_gicreg(ICH_LR14_EL2); >> + case 13: >> + cpu_if->vgic_lr[LR_OFFSET(13)] = read_gicreg(ICH_LR13_EL2); >> + case 12: >> + cpu_if->vgic_lr[LR_OFFSET(12)] = read_gicreg(ICH_LR12_EL2); >> + case 11: >> + cpu_if->vgic_lr[LR_OFFSET(11)] = read_gicreg(ICH_LR11_EL2); >> + case 10: >> + cpu_if->vgic_lr[LR_OFFSET(10)] = read_gicreg(ICH_LR10_EL2); >> + case 9: >> + cpu_if->vgic_lr[LR_OFFSET(9)] = read_gicreg(ICH_LR9_EL2); >> + case 8: >> + cpu_if->vgic_lr[LR_OFFSET(8)] = read_gicreg(ICH_LR8_EL2); >> + case 7: >> + cpu_if->vgic_lr[LR_OFFSET(7)] = read_gicreg(ICH_LR7_EL2); >> + case 6: >> + cpu_if->vgic_lr[LR_OFFSET(6)] = read_gicreg(ICH_LR6_EL2); >> + case 5: >> + cpu_if->vgic_lr[LR_OFFSET(5)] = read_gicreg(ICH_LR5_EL2); >> + case 4: >> + cpu_if->vgic_lr[LR_OFFSET(4)] = read_gicreg(ICH_LR4_EL2); >> + case 3: >> + cpu_if->vgic_lr[LR_OFFSET(3)] = read_gicreg(ICH_LR3_EL2); >> + case 2: >> + cpu_if->vgic_lr[LR_OFFSET(2)] = read_gicreg(ICH_LR2_EL2); >> + case 1: >> + cpu_if->vgic_lr[LR_OFFSET(1)] = read_gicreg(ICH_LR1_EL2); >> + case 0: >> + cpu_if->vgic_lr[LR_OFFSET(0)] = read_gicreg(ICH_LR0_EL2); > > I don't understand this; LR_OFFSET(0) == (15 - 0) == 15, so > > cpu_if->vgic_lr[15] = read_gicreg(ICH_LR0_EL2) ? Just like in the assembly version. We store the LRs in the order we read them so that we don't confuse the CPU by writing backward (believe it or not, CPUs do get horribly confused if you do that). >> + } >> + >> + switch (nr_pri) { >> + case 7: >> + cpu_if->vgic_ap0r[3] = read_gicreg(ICH_AP0R3_EL2); >> + cpu_if->vgic_ap0r[2] = read_gicreg(ICH_AP0R2_EL2); >> + case 6: >> + cpu_if->vgic_ap0r[1] = read_gicreg(ICH_AP0R1_EL2); >> + default: >> + cpu_if->vgic_ap0r[0] = read_gicreg(ICH_AP0R0_EL2); >> + } >> + >> + switch (nr_pri) { >> + case 7: >> + cpu_if->vgic_ap1r[3] = read_gicreg(ICH_AP1R3_EL2); >> + cpu_if->vgic_ap1r[2] = read_gicreg(ICH_AP1R2_EL2); >> + case 6: >> + cpu_if->vgic_ap1r[1] = read_gicreg(ICH_AP1R1_EL2); >> + default: >> + cpu_if->vgic_ap1r[0] = read_gicreg(ICH_AP1R0_EL2); >> + } >> + >> + write_gicreg(read_gicreg(ICC_SRE_EL2) | ICC_SRE_EL2_ENABLE, >> + ICC_SRE_EL2); > > nit: reading this out in a variable probably looks nicer. > >> + isb(); > > nit: should we comment on why the isb is required? > >> + write_gicreg(1, ICC_SRE_EL1); >> +} >> + >> +void __hyp_text __vgic_v3_restore_state(struct kvm_vcpu *vcpu) >> +{ >> + struct vgic_v3_cpu_if *cpu_if = &vcpu->arch.vgic_cpu.vgic_v3; >> + u64 val; >> + u32 nr_lr, nr_pri; >> + >> + /* Make sure SRE is valid before writing the other registers */ > > I know that I've reviewed this code before (the asm version) but coming > back to it I have no idea what the above really means... The meaning of some of the bits in ICH_VMCR_EL2 are conditioned by the configuration set in ICC_SRE_EL1. For example, VFIQEn is RES1 if ICC_SRE_EL1.SRE is 1. This causes a Group0 interrupt (as generated in GICv2 mode) to be delivered as a FIQ to the guest, with potentially fatal consequences. So we must make sure that ICC_SRE_EL1 has been actually programmed with the value we want before starting to mess with the rest of the GIC. >> + write_gicreg(cpu_if->vgic_sre, ICC_SRE_EL1); >> + isb(); >> + >> + write_gicreg(cpu_if->vgic_hcr, ICH_HCR_EL2); >> + write_gicreg(cpu_if->vgic_vmcr, ICH_VMCR_EL2); >> + >> + val = read_gicreg(ICH_VTR_EL2); >> + nr_lr = val & 0xf; >> + nr_pri = ((u32)val >> 29) + 1; > > you could have a define for this shift now that you use it more than > once. Sure. >> + >> + switch (nr_pri) { >> + case 7: >> + write_gicreg(cpu_if->vgic_ap1r[3], ICH_AP1R3_EL2); >> + write_gicreg(cpu_if->vgic_ap1r[2], ICH_AP1R2_EL2); >> + case 6: >> + write_gicreg(cpu_if->vgic_ap1r[1], ICH_AP1R1_EL2); >> + default: >> + write_gicreg(cpu_if->vgic_ap1r[0], ICH_AP1R0_EL2); >> + } >> + >> + switch (nr_pri) { >> + case 7: >> + write_gicreg(cpu_if->vgic_ap0r[3], ICH_AP0R3_EL2); >> + write_gicreg(cpu_if->vgic_ap0r[2], ICH_AP0R2_EL2); >> + case 6: >> + write_gicreg(cpu_if->vgic_ap0r[1], ICH_AP0R1_EL2); >> + default: >> + write_gicreg(cpu_if->vgic_ap0r[0], ICH_AP0R0_EL2); >> + } >> + >> + switch (nr_lr) { >> + case 15: >> + write_gicreg(cpu_if->vgic_lr[LR_OFFSET(15)], ICH_LR15_EL2); >> + case 14: >> + write_gicreg(cpu_if->vgic_lr[LR_OFFSET(14)], ICH_LR14_EL2); >> + case 13: >> + write_gicreg(cpu_if->vgic_lr[LR_OFFSET(13)], ICH_LR13_EL2); >> + case 12: >> + write_gicreg(cpu_if->vgic_lr[LR_OFFSET(12)], ICH_LR12_EL2); >> + case 11: >> + write_gicreg(cpu_if->vgic_lr[LR_OFFSET(11)], ICH_LR11_EL2); >> + case 10: >> + write_gicreg(cpu_if->vgic_lr[LR_OFFSET(10)], ICH_LR10_EL2); >> + case 9: >> + write_gicreg(cpu_if->vgic_lr[LR_OFFSET(9)], ICH_LR9_EL2); >> + case 8: >> + write_gicreg(cpu_if->vgic_lr[LR_OFFSET(8)], ICH_LR8_EL2); >> + case 7: >> + write_gicreg(cpu_if->vgic_lr[LR_OFFSET(7)], ICH_LR7_EL2); >> + case 6: >> + write_gicreg(cpu_if->vgic_lr[LR_OFFSET(6)], ICH_LR6_EL2); >> + case 5: >> + write_gicreg(cpu_if->vgic_lr[LR_OFFSET(5)], ICH_LR5_EL2); >> + case 4: >> + write_gicreg(cpu_if->vgic_lr[LR_OFFSET(4)], ICH_LR4_EL2); >> + case 3: >> + write_gicreg(cpu_if->vgic_lr[LR_OFFSET(3)], ICH_LR3_EL2); >> + case 2: >> + write_gicreg(cpu_if->vgic_lr[LR_OFFSET(2)], ICH_LR2_EL2); >> + case 1: >> + write_gicreg(cpu_if->vgic_lr[LR_OFFSET(1)], ICH_LR1_EL2); >> + case 0: >> + write_gicreg(cpu_if->vgic_lr[LR_OFFSET(0)], ICH_LR0_EL2); > > same question as above. > >> + } > > loads of trailing whitespace all over above > >> + >> + /* >> + * Ensure that the above will have reached the >> + * (re)distributors. This ensure the guest will read the > > s/ensure/ensures/ > >> + * correct values from the memory-mapped interface. >> + */ >> + isb(); >> + dsb(sy); >> + >> + /* >> + * Prevent the guest from touching the GIC system registers if >> + * SRE isn't enabled for GICv3 emulation. > > so can we emulate a GICv3 to the guest without system register access? > I.e. an MMIO only GICv3 interface? There is no such thing as a MMIO-based GICv3 CPU interface. The only purpose of this is to ensure that a guest presented with a GICv2 doesn't see the system registers as being available, because that both confusing and borderline illegal. >> + */ >> + if (!cpu_if->vgic_sre) { >> + write_gicreg(read_gicreg(ICC_SRE_EL2) & ~ICC_SRE_EL2_ENABLE, >> + ICC_SRE_EL2); >> + } >> +} >> + >> +u64 __hyp_text __vgic_v3_read_ich_vtr_el2(void) >> +{ >> + return read_gicreg(ICH_VTR_EL2); >> +} >> -- >> 2.1.4 >> > > As for translating the assembly code to C, this patch looks correct. > > Nevertheless, I felt obliged to ask into the details above, now when > you're touching all this code. No problem. Thanks for reviewing it. M.
On Tue, Dec 01, 2015 at 11:32:20AM +0000, Marc Zyngier wrote: > On 30/11/15 19:50, Christoffer Dall wrote: > > On Fri, Nov 27, 2015 at 06:49:58PM +0000, Marc Zyngier wrote: > >> Implement the vgic-v3 save restore as a direct translation of > >> the assembly code version. > >> > >> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> > >> --- > >> arch/arm64/kvm/hyp/Makefile | 1 + > >> arch/arm64/kvm/hyp/hyp.h | 3 + > >> arch/arm64/kvm/hyp/vgic-v3-sr.c | 222 ++++++++++++++++++++++++++++++++++++++++ > >> 3 files changed, 226 insertions(+) > >> create mode 100644 arch/arm64/kvm/hyp/vgic-v3-sr.c > >> > >> diff --git a/arch/arm64/kvm/hyp/Makefile b/arch/arm64/kvm/hyp/Makefile > >> index d8d5968..d1e38ce 100644 > >> --- a/arch/arm64/kvm/hyp/Makefile > >> +++ b/arch/arm64/kvm/hyp/Makefile > >> @@ -3,3 +3,4 @@ > >> # > >> > >> obj-$(CONFIG_KVM_ARM_HOST) += vgic-v2-sr.o > >> +obj-$(CONFIG_KVM_ARM_HOST) += vgic-v3-sr.o > >> diff --git a/arch/arm64/kvm/hyp/hyp.h b/arch/arm64/kvm/hyp/hyp.h > >> index 78f25c4..a31cb6e 100644 > >> --- a/arch/arm64/kvm/hyp/hyp.h > >> +++ b/arch/arm64/kvm/hyp/hyp.h > >> @@ -30,5 +30,8 @@ > >> void __vgic_v2_save_state(struct kvm_vcpu *vcpu); > >> void __vgic_v2_restore_state(struct kvm_vcpu *vcpu); > >> > >> +void __vgic_v3_save_state(struct kvm_vcpu *vcpu); > >> +void __vgic_v3_restore_state(struct kvm_vcpu *vcpu); > >> + > >> #endif /* __ARM64_KVM_HYP_H__ */ > >> > >> diff --git a/arch/arm64/kvm/hyp/vgic-v3-sr.c b/arch/arm64/kvm/hyp/vgic-v3-sr.c > >> new file mode 100644 > >> index 0000000..b490db5 > >> --- /dev/null > >> +++ b/arch/arm64/kvm/hyp/vgic-v3-sr.c > >> @@ -0,0 +1,222 @@ > >> +/* > >> + * Copyright (C) 2012-2015 - ARM Ltd > >> + * Author: Marc Zyngier <marc.zyngier@arm.com> > >> + * > >> + * This program is free software; you can redistribute it and/or modify > >> + * it under the terms of the GNU General Public License version 2 as > >> + * published by the Free Software Foundation. > >> + * > >> + * This program is distributed in the hope that it will be useful, > >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of > >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > >> + * GNU General Public License for more details. > >> + * > >> + * You should have received a copy of the GNU General Public License > >> + * along with this program. If not, see <http://www.gnu.org/licenses/>. > >> + */ > >> + > >> +#include <linux/compiler.h> > >> +#include <linux/irqchip/arm-gic-v3.h> > >> +#include <linux/kvm_host.h> > >> + > >> +#include <asm/kvm_mmu.h> > >> + > >> +#include "hyp.h" > >> + > >> +/* > >> + * We store LRs in reverse order to let the CPU deal with streaming > >> + * access. Use this macro to make it look saner... > >> + */ > >> +#define LR_OFFSET(n) (15 - n) > >> + > >> +#define read_gicreg(r) \ > >> + ({ \ > >> + u64 reg; \ > >> + asm volatile("mrs_s %0, " __stringify(r) : "=r" (reg)); \ > >> + reg; \ > >> + }) > >> + > >> +#define write_gicreg(v,r) \ > >> + do { \ > >> + u64 __val = (v); \ > >> + asm volatile("msr_s " __stringify(r) ", %0" : : "r" (__val));\ > >> + } while (0) > > > > remind me what the msr_s and mrs_s do compared to msr and mrs? > > They do the same job, only for the system registers which are not in the > original ARMv8 architecture spec, and most likely not implemented by > old(er) compilers. > > > are these the reason why we need separate macros to access the gic > > registers compared to 'normal' sysregs? > > Indeed. > > >> + > >> +/* vcpu is already in the HYP VA space */ > >> +void __hyp_text __vgic_v3_save_state(struct kvm_vcpu *vcpu) > >> +{ > >> + struct vgic_v3_cpu_if *cpu_if = &vcpu->arch.vgic_cpu.vgic_v3; > >> + u64 val; > >> + u32 nr_lr, nr_pri; > >> + > >> + /* > >> + * Make sure stores to the GIC via the memory mapped interface > >> + * are now visible to the system register interface. > >> + */ > >> + dsb(st); > >> + > >> + cpu_if->vgic_vmcr = read_gicreg(ICH_VMCR_EL2); > >> + cpu_if->vgic_misr = read_gicreg(ICH_MISR_EL2); > >> + cpu_if->vgic_eisr = read_gicreg(ICH_EISR_EL2); > >> + cpu_if->vgic_elrsr = read_gicreg(ICH_ELSR_EL2); > >> + > >> + write_gicreg(0, ICH_HCR_EL2); > >> + val = read_gicreg(ICH_VTR_EL2); > >> + nr_lr = val & 0xf; > > > > this is not technically nr_lr, it's max_lr or max_lr_idx or something > > like that. > > Let's go for max_lr_idx then. > > >> + nr_pri = ((u32)val >> 29) + 1; > > > > nit: nr_pri_bits > > > >> + > >> + switch (nr_lr) { > >> + case 15: > >> + cpu_if->vgic_lr[LR_OFFSET(15)] = read_gicreg(ICH_LR15_EL2); > >> + case 14: > >> + cpu_if->vgic_lr[LR_OFFSET(14)] = read_gicreg(ICH_LR14_EL2); > >> + case 13: > >> + cpu_if->vgic_lr[LR_OFFSET(13)] = read_gicreg(ICH_LR13_EL2); > >> + case 12: > >> + cpu_if->vgic_lr[LR_OFFSET(12)] = read_gicreg(ICH_LR12_EL2); > >> + case 11: > >> + cpu_if->vgic_lr[LR_OFFSET(11)] = read_gicreg(ICH_LR11_EL2); > >> + case 10: > >> + cpu_if->vgic_lr[LR_OFFSET(10)] = read_gicreg(ICH_LR10_EL2); > >> + case 9: > >> + cpu_if->vgic_lr[LR_OFFSET(9)] = read_gicreg(ICH_LR9_EL2); > >> + case 8: > >> + cpu_if->vgic_lr[LR_OFFSET(8)] = read_gicreg(ICH_LR8_EL2); > >> + case 7: > >> + cpu_if->vgic_lr[LR_OFFSET(7)] = read_gicreg(ICH_LR7_EL2); > >> + case 6: > >> + cpu_if->vgic_lr[LR_OFFSET(6)] = read_gicreg(ICH_LR6_EL2); > >> + case 5: > >> + cpu_if->vgic_lr[LR_OFFSET(5)] = read_gicreg(ICH_LR5_EL2); > >> + case 4: > >> + cpu_if->vgic_lr[LR_OFFSET(4)] = read_gicreg(ICH_LR4_EL2); > >> + case 3: > >> + cpu_if->vgic_lr[LR_OFFSET(3)] = read_gicreg(ICH_LR3_EL2); > >> + case 2: > >> + cpu_if->vgic_lr[LR_OFFSET(2)] = read_gicreg(ICH_LR2_EL2); > >> + case 1: > >> + cpu_if->vgic_lr[LR_OFFSET(1)] = read_gicreg(ICH_LR1_EL2); > >> + case 0: > >> + cpu_if->vgic_lr[LR_OFFSET(0)] = read_gicreg(ICH_LR0_EL2); > > > > I don't understand this; LR_OFFSET(0) == (15 - 0) == 15, so > > > > cpu_if->vgic_lr[15] = read_gicreg(ICH_LR0_EL2) ? > > Just like in the assembly version. We store the LRs in the order we read > them so that we don't confuse the CPU by writing backward (believe it or > not, CPUs do get horribly confused if you do that). but aren't we storing the wrong register to the wrong index in the array? Do we really access cpu_if->vgic_lr[15..12] in the C-code if the system only has 4 LRs? -Christoffer -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Dec 01, 2015 at 12:44:26PM +0100, Christoffer Dall wrote: > On Tue, Dec 01, 2015 at 11:32:20AM +0000, Marc Zyngier wrote: > > On 30/11/15 19:50, Christoffer Dall wrote: > > > On Fri, Nov 27, 2015 at 06:49:58PM +0000, Marc Zyngier wrote: > > >> Implement the vgic-v3 save restore as a direct translation of > > >> the assembly code version. > > >> > > >> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> > > >> --- > > >> arch/arm64/kvm/hyp/Makefile | 1 + > > >> arch/arm64/kvm/hyp/hyp.h | 3 + > > >> arch/arm64/kvm/hyp/vgic-v3-sr.c | 222 ++++++++++++++++++++++++++++++++++++++++ > > >> 3 files changed, 226 insertions(+) > > >> create mode 100644 arch/arm64/kvm/hyp/vgic-v3-sr.c > > >> > > >> diff --git a/arch/arm64/kvm/hyp/Makefile b/arch/arm64/kvm/hyp/Makefile > > >> index d8d5968..d1e38ce 100644 > > >> --- a/arch/arm64/kvm/hyp/Makefile > > >> +++ b/arch/arm64/kvm/hyp/Makefile > > >> @@ -3,3 +3,4 @@ > > >> # > > >> > > >> obj-$(CONFIG_KVM_ARM_HOST) += vgic-v2-sr.o > > >> +obj-$(CONFIG_KVM_ARM_HOST) += vgic-v3-sr.o > > >> diff --git a/arch/arm64/kvm/hyp/hyp.h b/arch/arm64/kvm/hyp/hyp.h > > >> index 78f25c4..a31cb6e 100644 > > >> --- a/arch/arm64/kvm/hyp/hyp.h > > >> +++ b/arch/arm64/kvm/hyp/hyp.h > > >> @@ -30,5 +30,8 @@ > > >> void __vgic_v2_save_state(struct kvm_vcpu *vcpu); > > >> void __vgic_v2_restore_state(struct kvm_vcpu *vcpu); > > >> > > >> +void __vgic_v3_save_state(struct kvm_vcpu *vcpu); > > >> +void __vgic_v3_restore_state(struct kvm_vcpu *vcpu); > > >> + > > >> #endif /* __ARM64_KVM_HYP_H__ */ > > >> > > >> diff --git a/arch/arm64/kvm/hyp/vgic-v3-sr.c b/arch/arm64/kvm/hyp/vgic-v3-sr.c > > >> new file mode 100644 > > >> index 0000000..b490db5 > > >> --- /dev/null > > >> +++ b/arch/arm64/kvm/hyp/vgic-v3-sr.c > > >> @@ -0,0 +1,222 @@ > > >> +/* > > >> + * Copyright (C) 2012-2015 - ARM Ltd > > >> + * Author: Marc Zyngier <marc.zyngier@arm.com> > > >> + * > > >> + * This program is free software; you can redistribute it and/or modify > > >> + * it under the terms of the GNU General Public License version 2 as > > >> + * published by the Free Software Foundation. > > >> + * > > >> + * This program is distributed in the hope that it will be useful, > > >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of > > >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > >> + * GNU General Public License for more details. > > >> + * > > >> + * You should have received a copy of the GNU General Public License > > >> + * along with this program. If not, see <http://www.gnu.org/licenses/>. > > >> + */ > > >> + > > >> +#include <linux/compiler.h> > > >> +#include <linux/irqchip/arm-gic-v3.h> > > >> +#include <linux/kvm_host.h> > > >> + > > >> +#include <asm/kvm_mmu.h> > > >> + > > >> +#include "hyp.h" > > >> + > > >> +/* > > >> + * We store LRs in reverse order to let the CPU deal with streaming > > >> + * access. Use this macro to make it look saner... > > >> + */ > > >> +#define LR_OFFSET(n) (15 - n) > > >> + > > >> +#define read_gicreg(r) \ > > >> + ({ \ > > >> + u64 reg; \ > > >> + asm volatile("mrs_s %0, " __stringify(r) : "=r" (reg)); \ > > >> + reg; \ > > >> + }) > > >> + > > >> +#define write_gicreg(v,r) \ > > >> + do { \ > > >> + u64 __val = (v); \ > > >> + asm volatile("msr_s " __stringify(r) ", %0" : : "r" (__val));\ > > >> + } while (0) > > > > > > remind me what the msr_s and mrs_s do compared to msr and mrs? > > > > They do the same job, only for the system registers which are not in the > > original ARMv8 architecture spec, and most likely not implemented by > > old(er) compilers. > > > > > are these the reason why we need separate macros to access the gic > > > registers compared to 'normal' sysregs? > > > > Indeed. > > > > >> + > > >> +/* vcpu is already in the HYP VA space */ > > >> +void __hyp_text __vgic_v3_save_state(struct kvm_vcpu *vcpu) > > >> +{ > > >> + struct vgic_v3_cpu_if *cpu_if = &vcpu->arch.vgic_cpu.vgic_v3; > > >> + u64 val; > > >> + u32 nr_lr, nr_pri; > > >> + > > >> + /* > > >> + * Make sure stores to the GIC via the memory mapped interface > > >> + * are now visible to the system register interface. > > >> + */ > > >> + dsb(st); > > >> + > > >> + cpu_if->vgic_vmcr = read_gicreg(ICH_VMCR_EL2); > > >> + cpu_if->vgic_misr = read_gicreg(ICH_MISR_EL2); > > >> + cpu_if->vgic_eisr = read_gicreg(ICH_EISR_EL2); > > >> + cpu_if->vgic_elrsr = read_gicreg(ICH_ELSR_EL2); > > >> + > > >> + write_gicreg(0, ICH_HCR_EL2); > > >> + val = read_gicreg(ICH_VTR_EL2); > > >> + nr_lr = val & 0xf; > > > > > > this is not technically nr_lr, it's max_lr or max_lr_idx or something > > > like that. > > > > Let's go for max_lr_idx then. > > > > >> + nr_pri = ((u32)val >> 29) + 1; > > > > > > nit: nr_pri_bits > > > > > >> + > > >> + switch (nr_lr) { > > >> + case 15: > > >> + cpu_if->vgic_lr[LR_OFFSET(15)] = read_gicreg(ICH_LR15_EL2); > > >> + case 14: > > >> + cpu_if->vgic_lr[LR_OFFSET(14)] = read_gicreg(ICH_LR14_EL2); > > >> + case 13: > > >> + cpu_if->vgic_lr[LR_OFFSET(13)] = read_gicreg(ICH_LR13_EL2); > > >> + case 12: > > >> + cpu_if->vgic_lr[LR_OFFSET(12)] = read_gicreg(ICH_LR12_EL2); > > >> + case 11: > > >> + cpu_if->vgic_lr[LR_OFFSET(11)] = read_gicreg(ICH_LR11_EL2); > > >> + case 10: > > >> + cpu_if->vgic_lr[LR_OFFSET(10)] = read_gicreg(ICH_LR10_EL2); > > >> + case 9: > > >> + cpu_if->vgic_lr[LR_OFFSET(9)] = read_gicreg(ICH_LR9_EL2); > > >> + case 8: > > >> + cpu_if->vgic_lr[LR_OFFSET(8)] = read_gicreg(ICH_LR8_EL2); > > >> + case 7: > > >> + cpu_if->vgic_lr[LR_OFFSET(7)] = read_gicreg(ICH_LR7_EL2); > > >> + case 6: > > >> + cpu_if->vgic_lr[LR_OFFSET(6)] = read_gicreg(ICH_LR6_EL2); > > >> + case 5: > > >> + cpu_if->vgic_lr[LR_OFFSET(5)] = read_gicreg(ICH_LR5_EL2); > > >> + case 4: > > >> + cpu_if->vgic_lr[LR_OFFSET(4)] = read_gicreg(ICH_LR4_EL2); > > >> + case 3: > > >> + cpu_if->vgic_lr[LR_OFFSET(3)] = read_gicreg(ICH_LR3_EL2); > > >> + case 2: > > >> + cpu_if->vgic_lr[LR_OFFSET(2)] = read_gicreg(ICH_LR2_EL2); > > >> + case 1: > > >> + cpu_if->vgic_lr[LR_OFFSET(1)] = read_gicreg(ICH_LR1_EL2); > > >> + case 0: > > >> + cpu_if->vgic_lr[LR_OFFSET(0)] = read_gicreg(ICH_LR0_EL2); > > > > > > I don't understand this; LR_OFFSET(0) == (15 - 0) == 15, so > > > > > > cpu_if->vgic_lr[15] = read_gicreg(ICH_LR0_EL2) ? > > > > Just like in the assembly version. We store the LRs in the order we read > > them so that we don't confuse the CPU by writing backward (believe it or > > not, CPUs do get horribly confused if you do that). > > but aren't we storing the wrong register to the wrong index in the > array? > > Do we really access cpu_if->vgic_lr[15..12] in the C-code if the system > only has 4 LRs? > ok, I looked at the code myself (not sure why I didn't do that in the first place) and indeed we use a different but with similar results macro to access the array from the C code. This is just insane to me, and we don't have a comment on the data structure saying "this is not stored the way you'd think it is". Why can't we just do: cpu_if->vgic_lr[3] = read_gicreg(ICH_LR3_EL2); cpu_if->vgic_lr[2] = read_gicreg(ICH_LR2_EL2); cpu_if->vgic_lr[1] = read_gicreg(ICH_LR1_EL2); cpu_if->vgic_lr[0] = read_gicreg(ICH_LR0_EL2); ? -Christoffer -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/12/15 11:44, Christoffer Dall wrote: > On Tue, Dec 01, 2015 at 11:32:20AM +0000, Marc Zyngier wrote: >> On 30/11/15 19:50, Christoffer Dall wrote: >>> On Fri, Nov 27, 2015 at 06:49:58PM +0000, Marc Zyngier wrote: >>>> Implement the vgic-v3 save restore as a direct translation of >>>> the assembly code version. >>>> >>>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> >>>> --- >>>> arch/arm64/kvm/hyp/Makefile | 1 + >>>> arch/arm64/kvm/hyp/hyp.h | 3 + >>>> arch/arm64/kvm/hyp/vgic-v3-sr.c | 222 ++++++++++++++++++++++++++++++++++++++++ >>>> 3 files changed, 226 insertions(+) >>>> create mode 100644 arch/arm64/kvm/hyp/vgic-v3-sr.c >>>> >>>> diff --git a/arch/arm64/kvm/hyp/Makefile b/arch/arm64/kvm/hyp/Makefile >>>> index d8d5968..d1e38ce 100644 >>>> --- a/arch/arm64/kvm/hyp/Makefile >>>> +++ b/arch/arm64/kvm/hyp/Makefile >>>> @@ -3,3 +3,4 @@ >>>> # >>>> >>>> obj-$(CONFIG_KVM_ARM_HOST) += vgic-v2-sr.o >>>> +obj-$(CONFIG_KVM_ARM_HOST) += vgic-v3-sr.o >>>> diff --git a/arch/arm64/kvm/hyp/hyp.h b/arch/arm64/kvm/hyp/hyp.h >>>> index 78f25c4..a31cb6e 100644 >>>> --- a/arch/arm64/kvm/hyp/hyp.h >>>> +++ b/arch/arm64/kvm/hyp/hyp.h >>>> @@ -30,5 +30,8 @@ >>>> void __vgic_v2_save_state(struct kvm_vcpu *vcpu); >>>> void __vgic_v2_restore_state(struct kvm_vcpu *vcpu); >>>> >>>> +void __vgic_v3_save_state(struct kvm_vcpu *vcpu); >>>> +void __vgic_v3_restore_state(struct kvm_vcpu *vcpu); >>>> + >>>> #endif /* __ARM64_KVM_HYP_H__ */ >>>> >>>> diff --git a/arch/arm64/kvm/hyp/vgic-v3-sr.c b/arch/arm64/kvm/hyp/vgic-v3-sr.c >>>> new file mode 100644 >>>> index 0000000..b490db5 >>>> --- /dev/null >>>> +++ b/arch/arm64/kvm/hyp/vgic-v3-sr.c >>>> @@ -0,0 +1,222 @@ >>>> +/* >>>> + * Copyright (C) 2012-2015 - ARM Ltd >>>> + * Author: Marc Zyngier <marc.zyngier@arm.com> >>>> + * >>>> + * This program is free software; you can redistribute it and/or modify >>>> + * it under the terms of the GNU General Public License version 2 as >>>> + * published by the Free Software Foundation. >>>> + * >>>> + * This program is distributed in the hope that it will be useful, >>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >>>> + * GNU General Public License for more details. >>>> + * >>>> + * You should have received a copy of the GNU General Public License >>>> + * along with this program. If not, see <http://www.gnu.org/licenses/>. >>>> + */ >>>> + >>>> +#include <linux/compiler.h> >>>> +#include <linux/irqchip/arm-gic-v3.h> >>>> +#include <linux/kvm_host.h> >>>> + >>>> +#include <asm/kvm_mmu.h> >>>> + >>>> +#include "hyp.h" >>>> + >>>> +/* >>>> + * We store LRs in reverse order to let the CPU deal with streaming >>>> + * access. Use this macro to make it look saner... >>>> + */ >>>> +#define LR_OFFSET(n) (15 - n) >>>> + >>>> +#define read_gicreg(r) \ >>>> + ({ \ >>>> + u64 reg; \ >>>> + asm volatile("mrs_s %0, " __stringify(r) : "=r" (reg)); \ >>>> + reg; \ >>>> + }) >>>> + >>>> +#define write_gicreg(v,r) \ >>>> + do { \ >>>> + u64 __val = (v); \ >>>> + asm volatile("msr_s " __stringify(r) ", %0" : : "r" (__val));\ >>>> + } while (0) >>> >>> remind me what the msr_s and mrs_s do compared to msr and mrs? >> >> They do the same job, only for the system registers which are not in the >> original ARMv8 architecture spec, and most likely not implemented by >> old(er) compilers. >> >>> are these the reason why we need separate macros to access the gic >>> registers compared to 'normal' sysregs? >> >> Indeed. >> >>>> + >>>> +/* vcpu is already in the HYP VA space */ >>>> +void __hyp_text __vgic_v3_save_state(struct kvm_vcpu *vcpu) >>>> +{ >>>> + struct vgic_v3_cpu_if *cpu_if = &vcpu->arch.vgic_cpu.vgic_v3; >>>> + u64 val; >>>> + u32 nr_lr, nr_pri; >>>> + >>>> + /* >>>> + * Make sure stores to the GIC via the memory mapped interface >>>> + * are now visible to the system register interface. >>>> + */ >>>> + dsb(st); >>>> + >>>> + cpu_if->vgic_vmcr = read_gicreg(ICH_VMCR_EL2); >>>> + cpu_if->vgic_misr = read_gicreg(ICH_MISR_EL2); >>>> + cpu_if->vgic_eisr = read_gicreg(ICH_EISR_EL2); >>>> + cpu_if->vgic_elrsr = read_gicreg(ICH_ELSR_EL2); >>>> + >>>> + write_gicreg(0, ICH_HCR_EL2); >>>> + val = read_gicreg(ICH_VTR_EL2); >>>> + nr_lr = val & 0xf; >>> >>> this is not technically nr_lr, it's max_lr or max_lr_idx or something >>> like that. >> >> Let's go for max_lr_idx then. >> >>>> + nr_pri = ((u32)val >> 29) + 1; >>> >>> nit: nr_pri_bits >>> >>>> + >>>> + switch (nr_lr) { >>>> + case 15: >>>> + cpu_if->vgic_lr[LR_OFFSET(15)] = read_gicreg(ICH_LR15_EL2); >>>> + case 14: >>>> + cpu_if->vgic_lr[LR_OFFSET(14)] = read_gicreg(ICH_LR14_EL2); >>>> + case 13: >>>> + cpu_if->vgic_lr[LR_OFFSET(13)] = read_gicreg(ICH_LR13_EL2); >>>> + case 12: >>>> + cpu_if->vgic_lr[LR_OFFSET(12)] = read_gicreg(ICH_LR12_EL2); >>>> + case 11: >>>> + cpu_if->vgic_lr[LR_OFFSET(11)] = read_gicreg(ICH_LR11_EL2); >>>> + case 10: >>>> + cpu_if->vgic_lr[LR_OFFSET(10)] = read_gicreg(ICH_LR10_EL2); >>>> + case 9: >>>> + cpu_if->vgic_lr[LR_OFFSET(9)] = read_gicreg(ICH_LR9_EL2); >>>> + case 8: >>>> + cpu_if->vgic_lr[LR_OFFSET(8)] = read_gicreg(ICH_LR8_EL2); >>>> + case 7: >>>> + cpu_if->vgic_lr[LR_OFFSET(7)] = read_gicreg(ICH_LR7_EL2); >>>> + case 6: >>>> + cpu_if->vgic_lr[LR_OFFSET(6)] = read_gicreg(ICH_LR6_EL2); >>>> + case 5: >>>> + cpu_if->vgic_lr[LR_OFFSET(5)] = read_gicreg(ICH_LR5_EL2); >>>> + case 4: >>>> + cpu_if->vgic_lr[LR_OFFSET(4)] = read_gicreg(ICH_LR4_EL2); >>>> + case 3: >>>> + cpu_if->vgic_lr[LR_OFFSET(3)] = read_gicreg(ICH_LR3_EL2); >>>> + case 2: >>>> + cpu_if->vgic_lr[LR_OFFSET(2)] = read_gicreg(ICH_LR2_EL2); >>>> + case 1: >>>> + cpu_if->vgic_lr[LR_OFFSET(1)] = read_gicreg(ICH_LR1_EL2); >>>> + case 0: >>>> + cpu_if->vgic_lr[LR_OFFSET(0)] = read_gicreg(ICH_LR0_EL2); >>> >>> I don't understand this; LR_OFFSET(0) == (15 - 0) == 15, so >>> >>> cpu_if->vgic_lr[15] = read_gicreg(ICH_LR0_EL2) ? >> >> Just like in the assembly version. We store the LRs in the order we read >> them so that we don't confuse the CPU by writing backward (believe it or >> not, CPUs do get horribly confused if you do that). > > but aren't we storing the wrong register to the wrong index in the > array? I don't think so. If we did, GICv3 support wouldn't get off the ground at all... > Do we really access cpu_if->vgic_lr[15..12] in the C-code if the system > only has 4 LRs? Yes we do. See how the vgic_v3_{get,set}_lr functions are using the exact same macro to get to the right LR. Thanks, M.
On 01/12/15 11:50, Christoffer Dall wrote: > On Tue, Dec 01, 2015 at 12:44:26PM +0100, Christoffer Dall wrote: >> On Tue, Dec 01, 2015 at 11:32:20AM +0000, Marc Zyngier wrote: >>> On 30/11/15 19:50, Christoffer Dall wrote: >>>> On Fri, Nov 27, 2015 at 06:49:58PM +0000, Marc Zyngier wrote: >>>>> Implement the vgic-v3 save restore as a direct translation of >>>>> the assembly code version. >>>>> >>>>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> >>>>> --- >>>>> arch/arm64/kvm/hyp/Makefile | 1 + >>>>> arch/arm64/kvm/hyp/hyp.h | 3 + >>>>> arch/arm64/kvm/hyp/vgic-v3-sr.c | 222 ++++++++++++++++++++++++++++++++++++++++ >>>>> 3 files changed, 226 insertions(+) >>>>> create mode 100644 arch/arm64/kvm/hyp/vgic-v3-sr.c >>>>> >>>>> diff --git a/arch/arm64/kvm/hyp/Makefile b/arch/arm64/kvm/hyp/Makefile >>>>> index d8d5968..d1e38ce 100644 >>>>> --- a/arch/arm64/kvm/hyp/Makefile >>>>> +++ b/arch/arm64/kvm/hyp/Makefile >>>>> @@ -3,3 +3,4 @@ >>>>> # >>>>> >>>>> obj-$(CONFIG_KVM_ARM_HOST) += vgic-v2-sr.o >>>>> +obj-$(CONFIG_KVM_ARM_HOST) += vgic-v3-sr.o >>>>> diff --git a/arch/arm64/kvm/hyp/hyp.h b/arch/arm64/kvm/hyp/hyp.h >>>>> index 78f25c4..a31cb6e 100644 >>>>> --- a/arch/arm64/kvm/hyp/hyp.h >>>>> +++ b/arch/arm64/kvm/hyp/hyp.h >>>>> @@ -30,5 +30,8 @@ >>>>> void __vgic_v2_save_state(struct kvm_vcpu *vcpu); >>>>> void __vgic_v2_restore_state(struct kvm_vcpu *vcpu); >>>>> >>>>> +void __vgic_v3_save_state(struct kvm_vcpu *vcpu); >>>>> +void __vgic_v3_restore_state(struct kvm_vcpu *vcpu); >>>>> + >>>>> #endif /* __ARM64_KVM_HYP_H__ */ >>>>> >>>>> diff --git a/arch/arm64/kvm/hyp/vgic-v3-sr.c b/arch/arm64/kvm/hyp/vgic-v3-sr.c >>>>> new file mode 100644 >>>>> index 0000000..b490db5 >>>>> --- /dev/null >>>>> +++ b/arch/arm64/kvm/hyp/vgic-v3-sr.c >>>>> @@ -0,0 +1,222 @@ >>>>> +/* >>>>> + * Copyright (C) 2012-2015 - ARM Ltd >>>>> + * Author: Marc Zyngier <marc.zyngier@arm.com> >>>>> + * >>>>> + * This program is free software; you can redistribute it and/or modify >>>>> + * it under the terms of the GNU General Public License version 2 as >>>>> + * published by the Free Software Foundation. >>>>> + * >>>>> + * This program is distributed in the hope that it will be useful, >>>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >>>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >>>>> + * GNU General Public License for more details. >>>>> + * >>>>> + * You should have received a copy of the GNU General Public License >>>>> + * along with this program. If not, see <http://www.gnu.org/licenses/>. >>>>> + */ >>>>> + >>>>> +#include <linux/compiler.h> >>>>> +#include <linux/irqchip/arm-gic-v3.h> >>>>> +#include <linux/kvm_host.h> >>>>> + >>>>> +#include <asm/kvm_mmu.h> >>>>> + >>>>> +#include "hyp.h" >>>>> + >>>>> +/* >>>>> + * We store LRs in reverse order to let the CPU deal with streaming >>>>> + * access. Use this macro to make it look saner... >>>>> + */ >>>>> +#define LR_OFFSET(n) (15 - n) >>>>> + >>>>> +#define read_gicreg(r) \ >>>>> + ({ \ >>>>> + u64 reg; \ >>>>> + asm volatile("mrs_s %0, " __stringify(r) : "=r" (reg)); \ >>>>> + reg; \ >>>>> + }) >>>>> + >>>>> +#define write_gicreg(v,r) \ >>>>> + do { \ >>>>> + u64 __val = (v); \ >>>>> + asm volatile("msr_s " __stringify(r) ", %0" : : "r" (__val));\ >>>>> + } while (0) >>>> >>>> remind me what the msr_s and mrs_s do compared to msr and mrs? >>> >>> They do the same job, only for the system registers which are not in the >>> original ARMv8 architecture spec, and most likely not implemented by >>> old(er) compilers. >>> >>>> are these the reason why we need separate macros to access the gic >>>> registers compared to 'normal' sysregs? >>> >>> Indeed. >>> >>>>> + >>>>> +/* vcpu is already in the HYP VA space */ >>>>> +void __hyp_text __vgic_v3_save_state(struct kvm_vcpu *vcpu) >>>>> +{ >>>>> + struct vgic_v3_cpu_if *cpu_if = &vcpu->arch.vgic_cpu.vgic_v3; >>>>> + u64 val; >>>>> + u32 nr_lr, nr_pri; >>>>> + >>>>> + /* >>>>> + * Make sure stores to the GIC via the memory mapped interface >>>>> + * are now visible to the system register interface. >>>>> + */ >>>>> + dsb(st); >>>>> + >>>>> + cpu_if->vgic_vmcr = read_gicreg(ICH_VMCR_EL2); >>>>> + cpu_if->vgic_misr = read_gicreg(ICH_MISR_EL2); >>>>> + cpu_if->vgic_eisr = read_gicreg(ICH_EISR_EL2); >>>>> + cpu_if->vgic_elrsr = read_gicreg(ICH_ELSR_EL2); >>>>> + >>>>> + write_gicreg(0, ICH_HCR_EL2); >>>>> + val = read_gicreg(ICH_VTR_EL2); >>>>> + nr_lr = val & 0xf; >>>> >>>> this is not technically nr_lr, it's max_lr or max_lr_idx or something >>>> like that. >>> >>> Let's go for max_lr_idx then. >>> >>>>> + nr_pri = ((u32)val >> 29) + 1; >>>> >>>> nit: nr_pri_bits >>>> >>>>> + >>>>> + switch (nr_lr) { >>>>> + case 15: >>>>> + cpu_if->vgic_lr[LR_OFFSET(15)] = read_gicreg(ICH_LR15_EL2); >>>>> + case 14: >>>>> + cpu_if->vgic_lr[LR_OFFSET(14)] = read_gicreg(ICH_LR14_EL2); >>>>> + case 13: >>>>> + cpu_if->vgic_lr[LR_OFFSET(13)] = read_gicreg(ICH_LR13_EL2); >>>>> + case 12: >>>>> + cpu_if->vgic_lr[LR_OFFSET(12)] = read_gicreg(ICH_LR12_EL2); >>>>> + case 11: >>>>> + cpu_if->vgic_lr[LR_OFFSET(11)] = read_gicreg(ICH_LR11_EL2); >>>>> + case 10: >>>>> + cpu_if->vgic_lr[LR_OFFSET(10)] = read_gicreg(ICH_LR10_EL2); >>>>> + case 9: >>>>> + cpu_if->vgic_lr[LR_OFFSET(9)] = read_gicreg(ICH_LR9_EL2); >>>>> + case 8: >>>>> + cpu_if->vgic_lr[LR_OFFSET(8)] = read_gicreg(ICH_LR8_EL2); >>>>> + case 7: >>>>> + cpu_if->vgic_lr[LR_OFFSET(7)] = read_gicreg(ICH_LR7_EL2); >>>>> + case 6: >>>>> + cpu_if->vgic_lr[LR_OFFSET(6)] = read_gicreg(ICH_LR6_EL2); >>>>> + case 5: >>>>> + cpu_if->vgic_lr[LR_OFFSET(5)] = read_gicreg(ICH_LR5_EL2); >>>>> + case 4: >>>>> + cpu_if->vgic_lr[LR_OFFSET(4)] = read_gicreg(ICH_LR4_EL2); >>>>> + case 3: >>>>> + cpu_if->vgic_lr[LR_OFFSET(3)] = read_gicreg(ICH_LR3_EL2); >>>>> + case 2: >>>>> + cpu_if->vgic_lr[LR_OFFSET(2)] = read_gicreg(ICH_LR2_EL2); >>>>> + case 1: >>>>> + cpu_if->vgic_lr[LR_OFFSET(1)] = read_gicreg(ICH_LR1_EL2); >>>>> + case 0: >>>>> + cpu_if->vgic_lr[LR_OFFSET(0)] = read_gicreg(ICH_LR0_EL2); >>>> >>>> I don't understand this; LR_OFFSET(0) == (15 - 0) == 15, so >>>> >>>> cpu_if->vgic_lr[15] = read_gicreg(ICH_LR0_EL2) ? >>> >>> Just like in the assembly version. We store the LRs in the order we read >>> them so that we don't confuse the CPU by writing backward (believe it or >>> not, CPUs do get horribly confused if you do that). >> >> but aren't we storing the wrong register to the wrong index in the >> array? >> >> Do we really access cpu_if->vgic_lr[15..12] in the C-code if the system >> only has 4 LRs? >> > ok, I looked at the code myself (not sure why I didn't do that in the > first place) and indeed we use a different but with similar results > macro to access the array from the C code. > > This is just insane to me, and we don't have a comment on the data > structure saying "this is not stored the way you'd think it is". > > Why can't we just do: > > cpu_if->vgic_lr[3] = read_gicreg(ICH_LR3_EL2); > cpu_if->vgic_lr[2] = read_gicreg(ICH_LR2_EL2); > cpu_if->vgic_lr[1] = read_gicreg(ICH_LR1_EL2); > cpu_if->vgic_lr[0] = read_gicreg(ICH_LR0_EL2); > > ? Because you're *really* killing performance by doing what are essentially streaming read/writes in the opposite direction. CPU prefetchers only work in one direction (incrementing the address). Doing it backwards breaks it. M.
On Tue, Dec 01, 2015 at 11:57:16AM +0000, Marc Zyngier wrote: > On 01/12/15 11:50, Christoffer Dall wrote: > > On Tue, Dec 01, 2015 at 12:44:26PM +0100, Christoffer Dall wrote: > >> On Tue, Dec 01, 2015 at 11:32:20AM +0000, Marc Zyngier wrote: > >>> On 30/11/15 19:50, Christoffer Dall wrote: > >>>> On Fri, Nov 27, 2015 at 06:49:58PM +0000, Marc Zyngier wrote: > >>>>> Implement the vgic-v3 save restore as a direct translation of > >>>>> the assembly code version. > >>>>> > >>>>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> > >>>>> --- > >>>>> arch/arm64/kvm/hyp/Makefile | 1 + > >>>>> arch/arm64/kvm/hyp/hyp.h | 3 + > >>>>> arch/arm64/kvm/hyp/vgic-v3-sr.c | 222 ++++++++++++++++++++++++++++++++++++++++ > >>>>> 3 files changed, 226 insertions(+) > >>>>> create mode 100644 arch/arm64/kvm/hyp/vgic-v3-sr.c > >>>>> > >>>>> diff --git a/arch/arm64/kvm/hyp/Makefile b/arch/arm64/kvm/hyp/Makefile > >>>>> index d8d5968..d1e38ce 100644 > >>>>> --- a/arch/arm64/kvm/hyp/Makefile > >>>>> +++ b/arch/arm64/kvm/hyp/Makefile > >>>>> @@ -3,3 +3,4 @@ > >>>>> # > >>>>> > >>>>> obj-$(CONFIG_KVM_ARM_HOST) += vgic-v2-sr.o > >>>>> +obj-$(CONFIG_KVM_ARM_HOST) += vgic-v3-sr.o > >>>>> diff --git a/arch/arm64/kvm/hyp/hyp.h b/arch/arm64/kvm/hyp/hyp.h > >>>>> index 78f25c4..a31cb6e 100644 > >>>>> --- a/arch/arm64/kvm/hyp/hyp.h > >>>>> +++ b/arch/arm64/kvm/hyp/hyp.h > >>>>> @@ -30,5 +30,8 @@ > >>>>> void __vgic_v2_save_state(struct kvm_vcpu *vcpu); > >>>>> void __vgic_v2_restore_state(struct kvm_vcpu *vcpu); > >>>>> > >>>>> +void __vgic_v3_save_state(struct kvm_vcpu *vcpu); > >>>>> +void __vgic_v3_restore_state(struct kvm_vcpu *vcpu); > >>>>> + > >>>>> #endif /* __ARM64_KVM_HYP_H__ */ > >>>>> > >>>>> diff --git a/arch/arm64/kvm/hyp/vgic-v3-sr.c b/arch/arm64/kvm/hyp/vgic-v3-sr.c > >>>>> new file mode 100644 > >>>>> index 0000000..b490db5 > >>>>> --- /dev/null > >>>>> +++ b/arch/arm64/kvm/hyp/vgic-v3-sr.c > >>>>> @@ -0,0 +1,222 @@ > >>>>> +/* > >>>>> + * Copyright (C) 2012-2015 - ARM Ltd > >>>>> + * Author: Marc Zyngier <marc.zyngier@arm.com> > >>>>> + * > >>>>> + * This program is free software; you can redistribute it and/or modify > >>>>> + * it under the terms of the GNU General Public License version 2 as > >>>>> + * published by the Free Software Foundation. > >>>>> + * > >>>>> + * This program is distributed in the hope that it will be useful, > >>>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of > >>>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > >>>>> + * GNU General Public License for more details. > >>>>> + * > >>>>> + * You should have received a copy of the GNU General Public License > >>>>> + * along with this program. If not, see <http://www.gnu.org/licenses/>. > >>>>> + */ > >>>>> + > >>>>> +#include <linux/compiler.h> > >>>>> +#include <linux/irqchip/arm-gic-v3.h> > >>>>> +#include <linux/kvm_host.h> > >>>>> + > >>>>> +#include <asm/kvm_mmu.h> > >>>>> + > >>>>> +#include "hyp.h" > >>>>> + > >>>>> +/* > >>>>> + * We store LRs in reverse order to let the CPU deal with streaming > >>>>> + * access. Use this macro to make it look saner... > >>>>> + */ > >>>>> +#define LR_OFFSET(n) (15 - n) > >>>>> + > >>>>> +#define read_gicreg(r) \ > >>>>> + ({ \ > >>>>> + u64 reg; \ > >>>>> + asm volatile("mrs_s %0, " __stringify(r) : "=r" (reg)); \ > >>>>> + reg; \ > >>>>> + }) > >>>>> + > >>>>> +#define write_gicreg(v,r) \ > >>>>> + do { \ > >>>>> + u64 __val = (v); \ > >>>>> + asm volatile("msr_s " __stringify(r) ", %0" : : "r" (__val));\ > >>>>> + } while (0) > >>>> > >>>> remind me what the msr_s and mrs_s do compared to msr and mrs? > >>> > >>> They do the same job, only for the system registers which are not in the > >>> original ARMv8 architecture spec, and most likely not implemented by > >>> old(er) compilers. > >>> > >>>> are these the reason why we need separate macros to access the gic > >>>> registers compared to 'normal' sysregs? > >>> > >>> Indeed. > >>> > >>>>> + > >>>>> +/* vcpu is already in the HYP VA space */ > >>>>> +void __hyp_text __vgic_v3_save_state(struct kvm_vcpu *vcpu) > >>>>> +{ > >>>>> + struct vgic_v3_cpu_if *cpu_if = &vcpu->arch.vgic_cpu.vgic_v3; > >>>>> + u64 val; > >>>>> + u32 nr_lr, nr_pri; > >>>>> + > >>>>> + /* > >>>>> + * Make sure stores to the GIC via the memory mapped interface > >>>>> + * are now visible to the system register interface. > >>>>> + */ > >>>>> + dsb(st); > >>>>> + > >>>>> + cpu_if->vgic_vmcr = read_gicreg(ICH_VMCR_EL2); > >>>>> + cpu_if->vgic_misr = read_gicreg(ICH_MISR_EL2); > >>>>> + cpu_if->vgic_eisr = read_gicreg(ICH_EISR_EL2); > >>>>> + cpu_if->vgic_elrsr = read_gicreg(ICH_ELSR_EL2); > >>>>> + > >>>>> + write_gicreg(0, ICH_HCR_EL2); > >>>>> + val = read_gicreg(ICH_VTR_EL2); > >>>>> + nr_lr = val & 0xf; > >>>> > >>>> this is not technically nr_lr, it's max_lr or max_lr_idx or something > >>>> like that. > >>> > >>> Let's go for max_lr_idx then. > >>> > >>>>> + nr_pri = ((u32)val >> 29) + 1; > >>>> > >>>> nit: nr_pri_bits > >>>> > >>>>> + > >>>>> + switch (nr_lr) { > >>>>> + case 15: > >>>>> + cpu_if->vgic_lr[LR_OFFSET(15)] = read_gicreg(ICH_LR15_EL2); > >>>>> + case 14: > >>>>> + cpu_if->vgic_lr[LR_OFFSET(14)] = read_gicreg(ICH_LR14_EL2); > >>>>> + case 13: > >>>>> + cpu_if->vgic_lr[LR_OFFSET(13)] = read_gicreg(ICH_LR13_EL2); > >>>>> + case 12: > >>>>> + cpu_if->vgic_lr[LR_OFFSET(12)] = read_gicreg(ICH_LR12_EL2); > >>>>> + case 11: > >>>>> + cpu_if->vgic_lr[LR_OFFSET(11)] = read_gicreg(ICH_LR11_EL2); > >>>>> + case 10: > >>>>> + cpu_if->vgic_lr[LR_OFFSET(10)] = read_gicreg(ICH_LR10_EL2); > >>>>> + case 9: > >>>>> + cpu_if->vgic_lr[LR_OFFSET(9)] = read_gicreg(ICH_LR9_EL2); > >>>>> + case 8: > >>>>> + cpu_if->vgic_lr[LR_OFFSET(8)] = read_gicreg(ICH_LR8_EL2); > >>>>> + case 7: > >>>>> + cpu_if->vgic_lr[LR_OFFSET(7)] = read_gicreg(ICH_LR7_EL2); > >>>>> + case 6: > >>>>> + cpu_if->vgic_lr[LR_OFFSET(6)] = read_gicreg(ICH_LR6_EL2); > >>>>> + case 5: > >>>>> + cpu_if->vgic_lr[LR_OFFSET(5)] = read_gicreg(ICH_LR5_EL2); > >>>>> + case 4: > >>>>> + cpu_if->vgic_lr[LR_OFFSET(4)] = read_gicreg(ICH_LR4_EL2); > >>>>> + case 3: > >>>>> + cpu_if->vgic_lr[LR_OFFSET(3)] = read_gicreg(ICH_LR3_EL2); > >>>>> + case 2: > >>>>> + cpu_if->vgic_lr[LR_OFFSET(2)] = read_gicreg(ICH_LR2_EL2); > >>>>> + case 1: > >>>>> + cpu_if->vgic_lr[LR_OFFSET(1)] = read_gicreg(ICH_LR1_EL2); > >>>>> + case 0: > >>>>> + cpu_if->vgic_lr[LR_OFFSET(0)] = read_gicreg(ICH_LR0_EL2); > >>>> > >>>> I don't understand this; LR_OFFSET(0) == (15 - 0) == 15, so > >>>> > >>>> cpu_if->vgic_lr[15] = read_gicreg(ICH_LR0_EL2) ? > >>> > >>> Just like in the assembly version. We store the LRs in the order we read > >>> them so that we don't confuse the CPU by writing backward (believe it or > >>> not, CPUs do get horribly confused if you do that). > >> > >> but aren't we storing the wrong register to the wrong index in the > >> array? > >> > >> Do we really access cpu_if->vgic_lr[15..12] in the C-code if the system > >> only has 4 LRs? > >> > > ok, I looked at the code myself (not sure why I didn't do that in the > > first place) and indeed we use a different but with similar results > > macro to access the array from the C code. > > > > This is just insane to me, and we don't have a comment on the data > > structure saying "this is not stored the way you'd think it is". > > > > Why can't we just do: > > > > cpu_if->vgic_lr[3] = read_gicreg(ICH_LR3_EL2); > > cpu_if->vgic_lr[2] = read_gicreg(ICH_LR2_EL2); > > cpu_if->vgic_lr[1] = read_gicreg(ICH_LR1_EL2); > > cpu_if->vgic_lr[0] = read_gicreg(ICH_LR0_EL2); > > > > ? > > Because you're *really* killing performance by doing what are > essentially streaming read/writes in the opposite direction. CPU > prefetchers only work in one direction (incrementing the address). Doing > it backwards breaks it. > hmm, and what are prefetching with the stores here? Did anyone actually measure this or is it theoretically really slow? Anyway, for the purposes of rewriting the world-switch in C, this looks fine. I hope we can come up with something less convoluted some time, perhaps at least using the same macro for LR_INDEX and making the comments more clear. -Christoffer -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/12/15 12:24, Christoffer Dall wrote: > On Tue, Dec 01, 2015 at 11:57:16AM +0000, Marc Zyngier wrote: >> On 01/12/15 11:50, Christoffer Dall wrote: >>> On Tue, Dec 01, 2015 at 12:44:26PM +0100, Christoffer Dall wrote: >>>> On Tue, Dec 01, 2015 at 11:32:20AM +0000, Marc Zyngier wrote: >>>>> On 30/11/15 19:50, Christoffer Dall wrote: >>>>>> On Fri, Nov 27, 2015 at 06:49:58PM +0000, Marc Zyngier wrote: >>>>>>> Implement the vgic-v3 save restore as a direct translation of >>>>>>> the assembly code version. >>>>>>> >>>>>>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> >>>>>>> --- >>>>>>> arch/arm64/kvm/hyp/Makefile | 1 + >>>>>>> arch/arm64/kvm/hyp/hyp.h | 3 + >>>>>>> arch/arm64/kvm/hyp/vgic-v3-sr.c | 222 ++++++++++++++++++++++++++++++++++++++++ >>>>>>> 3 files changed, 226 insertions(+) >>>>>>> create mode 100644 arch/arm64/kvm/hyp/vgic-v3-sr.c >>>>>>> >>>>>>> diff --git a/arch/arm64/kvm/hyp/Makefile b/arch/arm64/kvm/hyp/Makefile >>>>>>> index d8d5968..d1e38ce 100644 >>>>>>> --- a/arch/arm64/kvm/hyp/Makefile >>>>>>> +++ b/arch/arm64/kvm/hyp/Makefile >>>>>>> @@ -3,3 +3,4 @@ >>>>>>> # >>>>>>> >>>>>>> obj-$(CONFIG_KVM_ARM_HOST) += vgic-v2-sr.o >>>>>>> +obj-$(CONFIG_KVM_ARM_HOST) += vgic-v3-sr.o >>>>>>> diff --git a/arch/arm64/kvm/hyp/hyp.h b/arch/arm64/kvm/hyp/hyp.h >>>>>>> index 78f25c4..a31cb6e 100644 >>>>>>> --- a/arch/arm64/kvm/hyp/hyp.h >>>>>>> +++ b/arch/arm64/kvm/hyp/hyp.h >>>>>>> @@ -30,5 +30,8 @@ >>>>>>> void __vgic_v2_save_state(struct kvm_vcpu *vcpu); >>>>>>> void __vgic_v2_restore_state(struct kvm_vcpu *vcpu); >>>>>>> >>>>>>> +void __vgic_v3_save_state(struct kvm_vcpu *vcpu); >>>>>>> +void __vgic_v3_restore_state(struct kvm_vcpu *vcpu); >>>>>>> + >>>>>>> #endif /* __ARM64_KVM_HYP_H__ */ >>>>>>> >>>>>>> diff --git a/arch/arm64/kvm/hyp/vgic-v3-sr.c b/arch/arm64/kvm/hyp/vgic-v3-sr.c >>>>>>> new file mode 100644 >>>>>>> index 0000000..b490db5 >>>>>>> --- /dev/null >>>>>>> +++ b/arch/arm64/kvm/hyp/vgic-v3-sr.c >>>>>>> @@ -0,0 +1,222 @@ >>>>>>> +/* >>>>>>> + * Copyright (C) 2012-2015 - ARM Ltd >>>>>>> + * Author: Marc Zyngier <marc.zyngier@arm.com> >>>>>>> + * >>>>>>> + * This program is free software; you can redistribute it and/or modify >>>>>>> + * it under the terms of the GNU General Public License version 2 as >>>>>>> + * published by the Free Software Foundation. >>>>>>> + * >>>>>>> + * This program is distributed in the hope that it will be useful, >>>>>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >>>>>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >>>>>>> + * GNU General Public License for more details. >>>>>>> + * >>>>>>> + * You should have received a copy of the GNU General Public License >>>>>>> + * along with this program. If not, see <http://www.gnu.org/licenses/>. >>>>>>> + */ >>>>>>> + >>>>>>> +#include <linux/compiler.h> >>>>>>> +#include <linux/irqchip/arm-gic-v3.h> >>>>>>> +#include <linux/kvm_host.h> >>>>>>> + >>>>>>> +#include <asm/kvm_mmu.h> >>>>>>> + >>>>>>> +#include "hyp.h" >>>>>>> + >>>>>>> +/* >>>>>>> + * We store LRs in reverse order to let the CPU deal with streaming >>>>>>> + * access. Use this macro to make it look saner... >>>>>>> + */ >>>>>>> +#define LR_OFFSET(n) (15 - n) >>>>>>> + >>>>>>> +#define read_gicreg(r) \ >>>>>>> + ({ \ >>>>>>> + u64 reg; \ >>>>>>> + asm volatile("mrs_s %0, " __stringify(r) : "=r" (reg)); \ >>>>>>> + reg; \ >>>>>>> + }) >>>>>>> + >>>>>>> +#define write_gicreg(v,r) \ >>>>>>> + do { \ >>>>>>> + u64 __val = (v); \ >>>>>>> + asm volatile("msr_s " __stringify(r) ", %0" : : "r" (__val));\ >>>>>>> + } while (0) >>>>>> >>>>>> remind me what the msr_s and mrs_s do compared to msr and mrs? >>>>> >>>>> They do the same job, only for the system registers which are not in the >>>>> original ARMv8 architecture spec, and most likely not implemented by >>>>> old(er) compilers. >>>>> >>>>>> are these the reason why we need separate macros to access the gic >>>>>> registers compared to 'normal' sysregs? >>>>> >>>>> Indeed. >>>>> >>>>>>> + >>>>>>> +/* vcpu is already in the HYP VA space */ >>>>>>> +void __hyp_text __vgic_v3_save_state(struct kvm_vcpu *vcpu) >>>>>>> +{ >>>>>>> + struct vgic_v3_cpu_if *cpu_if = &vcpu->arch.vgic_cpu.vgic_v3; >>>>>>> + u64 val; >>>>>>> + u32 nr_lr, nr_pri; >>>>>>> + >>>>>>> + /* >>>>>>> + * Make sure stores to the GIC via the memory mapped interface >>>>>>> + * are now visible to the system register interface. >>>>>>> + */ >>>>>>> + dsb(st); >>>>>>> + >>>>>>> + cpu_if->vgic_vmcr = read_gicreg(ICH_VMCR_EL2); >>>>>>> + cpu_if->vgic_misr = read_gicreg(ICH_MISR_EL2); >>>>>>> + cpu_if->vgic_eisr = read_gicreg(ICH_EISR_EL2); >>>>>>> + cpu_if->vgic_elrsr = read_gicreg(ICH_ELSR_EL2); >>>>>>> + >>>>>>> + write_gicreg(0, ICH_HCR_EL2); >>>>>>> + val = read_gicreg(ICH_VTR_EL2); >>>>>>> + nr_lr = val & 0xf; >>>>>> >>>>>> this is not technically nr_lr, it's max_lr or max_lr_idx or something >>>>>> like that. >>>>> >>>>> Let's go for max_lr_idx then. >>>>> >>>>>>> + nr_pri = ((u32)val >> 29) + 1; >>>>>> >>>>>> nit: nr_pri_bits >>>>>> >>>>>>> + >>>>>>> + switch (nr_lr) { >>>>>>> + case 15: >>>>>>> + cpu_if->vgic_lr[LR_OFFSET(15)] = read_gicreg(ICH_LR15_EL2); >>>>>>> + case 14: >>>>>>> + cpu_if->vgic_lr[LR_OFFSET(14)] = read_gicreg(ICH_LR14_EL2); >>>>>>> + case 13: >>>>>>> + cpu_if->vgic_lr[LR_OFFSET(13)] = read_gicreg(ICH_LR13_EL2); >>>>>>> + case 12: >>>>>>> + cpu_if->vgic_lr[LR_OFFSET(12)] = read_gicreg(ICH_LR12_EL2); >>>>>>> + case 11: >>>>>>> + cpu_if->vgic_lr[LR_OFFSET(11)] = read_gicreg(ICH_LR11_EL2); >>>>>>> + case 10: >>>>>>> + cpu_if->vgic_lr[LR_OFFSET(10)] = read_gicreg(ICH_LR10_EL2); >>>>>>> + case 9: >>>>>>> + cpu_if->vgic_lr[LR_OFFSET(9)] = read_gicreg(ICH_LR9_EL2); >>>>>>> + case 8: >>>>>>> + cpu_if->vgic_lr[LR_OFFSET(8)] = read_gicreg(ICH_LR8_EL2); >>>>>>> + case 7: >>>>>>> + cpu_if->vgic_lr[LR_OFFSET(7)] = read_gicreg(ICH_LR7_EL2); >>>>>>> + case 6: >>>>>>> + cpu_if->vgic_lr[LR_OFFSET(6)] = read_gicreg(ICH_LR6_EL2); >>>>>>> + case 5: >>>>>>> + cpu_if->vgic_lr[LR_OFFSET(5)] = read_gicreg(ICH_LR5_EL2); >>>>>>> + case 4: >>>>>>> + cpu_if->vgic_lr[LR_OFFSET(4)] = read_gicreg(ICH_LR4_EL2); >>>>>>> + case 3: >>>>>>> + cpu_if->vgic_lr[LR_OFFSET(3)] = read_gicreg(ICH_LR3_EL2); >>>>>>> + case 2: >>>>>>> + cpu_if->vgic_lr[LR_OFFSET(2)] = read_gicreg(ICH_LR2_EL2); >>>>>>> + case 1: >>>>>>> + cpu_if->vgic_lr[LR_OFFSET(1)] = read_gicreg(ICH_LR1_EL2); >>>>>>> + case 0: >>>>>>> + cpu_if->vgic_lr[LR_OFFSET(0)] = read_gicreg(ICH_LR0_EL2); >>>>>> >>>>>> I don't understand this; LR_OFFSET(0) == (15 - 0) == 15, so >>>>>> >>>>>> cpu_if->vgic_lr[15] = read_gicreg(ICH_LR0_EL2) ? >>>>> >>>>> Just like in the assembly version. We store the LRs in the order we read >>>>> them so that we don't confuse the CPU by writing backward (believe it or >>>>> not, CPUs do get horribly confused if you do that). >>>> >>>> but aren't we storing the wrong register to the wrong index in the >>>> array? >>>> >>>> Do we really access cpu_if->vgic_lr[15..12] in the C-code if the system >>>> only has 4 LRs? >>>> >>> ok, I looked at the code myself (not sure why I didn't do that in the >>> first place) and indeed we use a different but with similar results >>> macro to access the array from the C code. >>> >>> This is just insane to me, and we don't have a comment on the data >>> structure saying "this is not stored the way you'd think it is". >>> >>> Why can't we just do: >>> >>> cpu_if->vgic_lr[3] = read_gicreg(ICH_LR3_EL2); >>> cpu_if->vgic_lr[2] = read_gicreg(ICH_LR2_EL2); >>> cpu_if->vgic_lr[1] = read_gicreg(ICH_LR1_EL2); >>> cpu_if->vgic_lr[0] = read_gicreg(ICH_LR0_EL2); >>> >>> ? >> >> Because you're *really* killing performance by doing what are >> essentially streaming read/writes in the opposite direction. CPU >> prefetchers only work in one direction (incrementing the address). Doing >> it backwards breaks it. >> > hmm, and what are prefetching with the stores here? Cache lines. When you increase the address, the core can trigger a prefetch of the next cache line so that it is ready by the time you get to start clobbering it. > Did anyone actually measure this or is it theoretically really slow? I did some measurements around that in a previous life. > Anyway, for the purposes of rewriting the world-switch in C, this looks > fine. I hope we can come up with something less convoluted some time, > perhaps at least using the same macro for LR_INDEX and making the > comments more clear. Sure. Thanks, M.
diff --git a/arch/arm64/kvm/hyp/Makefile b/arch/arm64/kvm/hyp/Makefile index d8d5968..d1e38ce 100644 --- a/arch/arm64/kvm/hyp/Makefile +++ b/arch/arm64/kvm/hyp/Makefile @@ -3,3 +3,4 @@ # obj-$(CONFIG_KVM_ARM_HOST) += vgic-v2-sr.o +obj-$(CONFIG_KVM_ARM_HOST) += vgic-v3-sr.o diff --git a/arch/arm64/kvm/hyp/hyp.h b/arch/arm64/kvm/hyp/hyp.h index 78f25c4..a31cb6e 100644 --- a/arch/arm64/kvm/hyp/hyp.h +++ b/arch/arm64/kvm/hyp/hyp.h @@ -30,5 +30,8 @@ void __vgic_v2_save_state(struct kvm_vcpu *vcpu); void __vgic_v2_restore_state(struct kvm_vcpu *vcpu); +void __vgic_v3_save_state(struct kvm_vcpu *vcpu); +void __vgic_v3_restore_state(struct kvm_vcpu *vcpu); + #endif /* __ARM64_KVM_HYP_H__ */ diff --git a/arch/arm64/kvm/hyp/vgic-v3-sr.c b/arch/arm64/kvm/hyp/vgic-v3-sr.c new file mode 100644 index 0000000..b490db5 --- /dev/null +++ b/arch/arm64/kvm/hyp/vgic-v3-sr.c @@ -0,0 +1,222 @@ +/* + * Copyright (C) 2012-2015 - ARM Ltd + * Author: Marc Zyngier <marc.zyngier@arm.com> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see <http://www.gnu.org/licenses/>. + */ + +#include <linux/compiler.h> +#include <linux/irqchip/arm-gic-v3.h> +#include <linux/kvm_host.h> + +#include <asm/kvm_mmu.h> + +#include "hyp.h" + +/* + * We store LRs in reverse order to let the CPU deal with streaming + * access. Use this macro to make it look saner... + */ +#define LR_OFFSET(n) (15 - n) + +#define read_gicreg(r) \ + ({ \ + u64 reg; \ + asm volatile("mrs_s %0, " __stringify(r) : "=r" (reg)); \ + reg; \ + }) + +#define write_gicreg(v,r) \ + do { \ + u64 __val = (v); \ + asm volatile("msr_s " __stringify(r) ", %0" : : "r" (__val));\ + } while (0) + +/* vcpu is already in the HYP VA space */ +void __hyp_text __vgic_v3_save_state(struct kvm_vcpu *vcpu) +{ + struct vgic_v3_cpu_if *cpu_if = &vcpu->arch.vgic_cpu.vgic_v3; + u64 val; + u32 nr_lr, nr_pri; + + /* + * Make sure stores to the GIC via the memory mapped interface + * are now visible to the system register interface. + */ + dsb(st); + + cpu_if->vgic_vmcr = read_gicreg(ICH_VMCR_EL2); + cpu_if->vgic_misr = read_gicreg(ICH_MISR_EL2); + cpu_if->vgic_eisr = read_gicreg(ICH_EISR_EL2); + cpu_if->vgic_elrsr = read_gicreg(ICH_ELSR_EL2); + + write_gicreg(0, ICH_HCR_EL2); + val = read_gicreg(ICH_VTR_EL2); + nr_lr = val & 0xf; + nr_pri = ((u32)val >> 29) + 1; + + switch (nr_lr) { + case 15: + cpu_if->vgic_lr[LR_OFFSET(15)] = read_gicreg(ICH_LR15_EL2); + case 14: + cpu_if->vgic_lr[LR_OFFSET(14)] = read_gicreg(ICH_LR14_EL2); + case 13: + cpu_if->vgic_lr[LR_OFFSET(13)] = read_gicreg(ICH_LR13_EL2); + case 12: + cpu_if->vgic_lr[LR_OFFSET(12)] = read_gicreg(ICH_LR12_EL2); + case 11: + cpu_if->vgic_lr[LR_OFFSET(11)] = read_gicreg(ICH_LR11_EL2); + case 10: + cpu_if->vgic_lr[LR_OFFSET(10)] = read_gicreg(ICH_LR10_EL2); + case 9: + cpu_if->vgic_lr[LR_OFFSET(9)] = read_gicreg(ICH_LR9_EL2); + case 8: + cpu_if->vgic_lr[LR_OFFSET(8)] = read_gicreg(ICH_LR8_EL2); + case 7: + cpu_if->vgic_lr[LR_OFFSET(7)] = read_gicreg(ICH_LR7_EL2); + case 6: + cpu_if->vgic_lr[LR_OFFSET(6)] = read_gicreg(ICH_LR6_EL2); + case 5: + cpu_if->vgic_lr[LR_OFFSET(5)] = read_gicreg(ICH_LR5_EL2); + case 4: + cpu_if->vgic_lr[LR_OFFSET(4)] = read_gicreg(ICH_LR4_EL2); + case 3: + cpu_if->vgic_lr[LR_OFFSET(3)] = read_gicreg(ICH_LR3_EL2); + case 2: + cpu_if->vgic_lr[LR_OFFSET(2)] = read_gicreg(ICH_LR2_EL2); + case 1: + cpu_if->vgic_lr[LR_OFFSET(1)] = read_gicreg(ICH_LR1_EL2); + case 0: + cpu_if->vgic_lr[LR_OFFSET(0)] = read_gicreg(ICH_LR0_EL2); + } + + switch (nr_pri) { + case 7: + cpu_if->vgic_ap0r[3] = read_gicreg(ICH_AP0R3_EL2); + cpu_if->vgic_ap0r[2] = read_gicreg(ICH_AP0R2_EL2); + case 6: + cpu_if->vgic_ap0r[1] = read_gicreg(ICH_AP0R1_EL2); + default: + cpu_if->vgic_ap0r[0] = read_gicreg(ICH_AP0R0_EL2); + } + + switch (nr_pri) { + case 7: + cpu_if->vgic_ap1r[3] = read_gicreg(ICH_AP1R3_EL2); + cpu_if->vgic_ap1r[2] = read_gicreg(ICH_AP1R2_EL2); + case 6: + cpu_if->vgic_ap1r[1] = read_gicreg(ICH_AP1R1_EL2); + default: + cpu_if->vgic_ap1r[0] = read_gicreg(ICH_AP1R0_EL2); + } + + write_gicreg(read_gicreg(ICC_SRE_EL2) | ICC_SRE_EL2_ENABLE, + ICC_SRE_EL2); + isb(); + write_gicreg(1, ICC_SRE_EL1); +} + +void __hyp_text __vgic_v3_restore_state(struct kvm_vcpu *vcpu) +{ + struct vgic_v3_cpu_if *cpu_if = &vcpu->arch.vgic_cpu.vgic_v3; + u64 val; + u32 nr_lr, nr_pri; + + /* Make sure SRE is valid before writing the other registers */ + write_gicreg(cpu_if->vgic_sre, ICC_SRE_EL1); + isb(); + + write_gicreg(cpu_if->vgic_hcr, ICH_HCR_EL2); + write_gicreg(cpu_if->vgic_vmcr, ICH_VMCR_EL2); + + val = read_gicreg(ICH_VTR_EL2); + nr_lr = val & 0xf; + nr_pri = ((u32)val >> 29) + 1; + + switch (nr_pri) { + case 7: + write_gicreg(cpu_if->vgic_ap1r[3], ICH_AP1R3_EL2); + write_gicreg(cpu_if->vgic_ap1r[2], ICH_AP1R2_EL2); + case 6: + write_gicreg(cpu_if->vgic_ap1r[1], ICH_AP1R1_EL2); + default: + write_gicreg(cpu_if->vgic_ap1r[0], ICH_AP1R0_EL2); + } + + switch (nr_pri) { + case 7: + write_gicreg(cpu_if->vgic_ap0r[3], ICH_AP0R3_EL2); + write_gicreg(cpu_if->vgic_ap0r[2], ICH_AP0R2_EL2); + case 6: + write_gicreg(cpu_if->vgic_ap0r[1], ICH_AP0R1_EL2); + default: + write_gicreg(cpu_if->vgic_ap0r[0], ICH_AP0R0_EL2); + } + + switch (nr_lr) { + case 15: + write_gicreg(cpu_if->vgic_lr[LR_OFFSET(15)], ICH_LR15_EL2); + case 14: + write_gicreg(cpu_if->vgic_lr[LR_OFFSET(14)], ICH_LR14_EL2); + case 13: + write_gicreg(cpu_if->vgic_lr[LR_OFFSET(13)], ICH_LR13_EL2); + case 12: + write_gicreg(cpu_if->vgic_lr[LR_OFFSET(12)], ICH_LR12_EL2); + case 11: + write_gicreg(cpu_if->vgic_lr[LR_OFFSET(11)], ICH_LR11_EL2); + case 10: + write_gicreg(cpu_if->vgic_lr[LR_OFFSET(10)], ICH_LR10_EL2); + case 9: + write_gicreg(cpu_if->vgic_lr[LR_OFFSET(9)], ICH_LR9_EL2); + case 8: + write_gicreg(cpu_if->vgic_lr[LR_OFFSET(8)], ICH_LR8_EL2); + case 7: + write_gicreg(cpu_if->vgic_lr[LR_OFFSET(7)], ICH_LR7_EL2); + case 6: + write_gicreg(cpu_if->vgic_lr[LR_OFFSET(6)], ICH_LR6_EL2); + case 5: + write_gicreg(cpu_if->vgic_lr[LR_OFFSET(5)], ICH_LR5_EL2); + case 4: + write_gicreg(cpu_if->vgic_lr[LR_OFFSET(4)], ICH_LR4_EL2); + case 3: + write_gicreg(cpu_if->vgic_lr[LR_OFFSET(3)], ICH_LR3_EL2); + case 2: + write_gicreg(cpu_if->vgic_lr[LR_OFFSET(2)], ICH_LR2_EL2); + case 1: + write_gicreg(cpu_if->vgic_lr[LR_OFFSET(1)], ICH_LR1_EL2); + case 0: + write_gicreg(cpu_if->vgic_lr[LR_OFFSET(0)], ICH_LR0_EL2); + } + + /* + * Ensure that the above will have reached the + * (re)distributors. This ensure the guest will read the + * correct values from the memory-mapped interface. + */ + isb(); + dsb(sy); + + /* + * Prevent the guest from touching the GIC system registers if + * SRE isn't enabled for GICv3 emulation. + */ + if (!cpu_if->vgic_sre) { + write_gicreg(read_gicreg(ICC_SRE_EL2) & ~ICC_SRE_EL2_ENABLE, + ICC_SRE_EL2); + } +} + +u64 __hyp_text __vgic_v3_read_ich_vtr_el2(void) +{ + return read_gicreg(ICH_VTR_EL2); +}
Implement the vgic-v3 save restore as a direct translation of the assembly code version. Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> --- arch/arm64/kvm/hyp/Makefile | 1 + arch/arm64/kvm/hyp/hyp.h | 3 + arch/arm64/kvm/hyp/vgic-v3-sr.c | 222 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 226 insertions(+) create mode 100644 arch/arm64/kvm/hyp/vgic-v3-sr.c