From mboxrd@z Thu Jan 1 00:00:00 1970 From: julien.grall@arm.com (Julien Grall) Date: Thu, 22 Feb 2018 18:31:08 +0000 Subject: [PATCH v4 28/40] KVM: arm64: Defer saving/restoring 64-bit sysregs to vcpu load/put on VHE In-Reply-To: References: <20180215210332.8648-1-christoffer.dall@linaro.org> <20180215210332.8648-29-christoffer.dall@linaro.org> Message-ID: <40a10d77-85c8-9c74-c94c-57e4f8854fef@arm.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 22/02/18 18:30, Julien Grall wrote: > Hi Christoffer, > > On 15/02/18 21:03, Christoffer Dall wrote: >> Some system registers do not affect the host kernel's execution and can >> therefore be loaded when we are about to run a VCPU and we don't have to >> restore the host state to the hardware before the time when we are >> actually about to return to userspace or schedule out the VCPU thread. >> >> The EL1 system registers and the userspace state registers only >> affecting EL0 execution do not need to be saved and restored on every >> switch between the VM and the host, because they don't affect the host >> kernel's execution. >> >> We mark all registers which are now deffered as such in the > > NIT: s/deffered/deferred/ I think. > >> vcpu_{read,write}_sys_reg accessors in sys-regs.c to ensure the most >> up-to-date copy is always accessed. >> >> Note MPIDR_EL1 (controlled via VMPIDR_EL2) is accessed from other vcpu >> threads, for example via the GIC emulation, and therefore must be >> declared as immediate, which is fine as the guest cannot modify this >> value. I forgot to comment on this. I missed this paragraph at the first read and was wondering why MPIDR_EL1 was not accessed using sysreg in vcpu_{read,write}_sys_reg. It might be worth considering a comment in those functions. >> >> The 32-bit sysregs can also be deferred but we do this in a separate >> patch as it requires a bit more infrastructure. > > > [...] > >> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c >> index b3c3f014aa61..f060309337aa 100644 >> --- a/arch/arm64/kvm/sys_regs.c >> +++ b/arch/arm64/kvm/sys_regs.c >> @@ -87,6 +87,26 @@ u64 vcpu_read_sys_reg(struct kvm_vcpu *vcpu, int reg) >> ?????? * exit from the guest but are only saved on vcpu_put. >> ?????? */ >> ????? switch (reg) { >> +??? case CSSELR_EL1:??? return read_sysreg_s(SYS_CSSELR_EL1); >> +??? case SCTLR_EL1:??????? return read_sysreg_s(sctlr_EL12); >> +??? case ACTLR_EL1:??????? return read_sysreg_s(SYS_ACTLR_EL1); >> +??? case CPACR_EL1:??????? return read_sysreg_s(cpacr_EL12); >> +??? case TTBR0_EL1:??????? return read_sysreg_s(ttbr0_EL12); >> +??? case TTBR1_EL1:??????? return read_sysreg_s(ttbr1_EL12); >> +??? case TCR_EL1:??????? return read_sysreg_s(tcr_EL12); >> +??? case ESR_EL1:??????? return read_sysreg_s(esr_EL12); >> +??? case AFSR0_EL1:??????? return read_sysreg_s(afsr0_EL12); >> +??? case AFSR1_EL1:??????? return read_sysreg_s(afsr1_EL12); >> +??? case FAR_EL1:??????? return read_sysreg_s(far_EL12); >> +??? case MAIR_EL1:??????? return read_sysreg_s(mair_EL12); >> +??? case VBAR_EL1:??????? return read_sysreg_s(vbar_EL12); >> +??? case CONTEXTIDR_EL1:??? return read_sysreg_s(contextidr_EL12); >> +??? case TPIDR_EL0:??????? return read_sysreg_s(SYS_TPIDR_EL0); >> +??? case TPIDRRO_EL0:??? return read_sysreg_s(SYS_TPIDRRO_EL0); > > I find a bit confusing to have some EL0 registers in the middle of EL1 > ones. Is it because they are listed by encoding? > >> +??? case TPIDR_EL1:??????? return read_sysreg_s(SYS_TPIDR_EL1); >> +??? case AMAIR_EL1:??????? return read_sysreg_s(amair_EL12); >> +??? case CNTKCTL_EL1:??? return read_sysreg_s(cntkctl_EL12); >> +??? case PAR_EL1:??????? return read_sysreg_s(SYS_PAR_EL1); >> ????? } >> ? immediate_read: >> @@ -103,6 +123,26 @@ void vcpu_write_sys_reg(struct kvm_vcpu *vcpu, >> int reg, u64 val) >> ?????? * entry to the guest but are only restored on vcpu_load. >> ?????? */ >> ????? switch (reg) { >> +??? case CSSELR_EL1:??? write_sysreg_s(val, SYS_CSSELR_EL1);??? return; >> +??? case SCTLR_EL1:??????? write_sysreg_s(val, sctlr_EL12);??? return; >> +??? case ACTLR_EL1:??????? write_sysreg_s(val, SYS_ACTLR_EL1); >> return; >> +??? case CPACR_EL1:??????? write_sysreg_s(val, cpacr_EL12);??? return; >> +??? case TTBR0_EL1:??????? write_sysreg_s(val, ttbr0_EL12);??? return; >> +??? case TTBR1_EL1:??????? write_sysreg_s(val, ttbr1_EL12);??? return; >> +??? case TCR_EL1:??????? write_sysreg_s(val, tcr_EL12);??????? return; >> +??? case ESR_EL1:??????? write_sysreg_s(val, esr_EL12);??????? return; >> +??? case AFSR0_EL1:??????? write_sysreg_s(val, afsr0_EL12);??? return; >> +??? case AFSR1_EL1:??????? write_sysreg_s(val, afsr1_EL12);??? return; >> +??? case FAR_EL1:??????? write_sysreg_s(val, far_EL12);??????? return; >> +??? case MAIR_EL1:??????? write_sysreg_s(val, mair_EL12);??????? return; >> +??? case VBAR_EL1:??????? write_sysreg_s(val, vbar_EL12);??????? return; >> +??? case CONTEXTIDR_EL1:??? write_sysreg_s(val, contextidr_EL12); >> return; >> +??? case TPIDR_EL0:??????? write_sysreg_s(val, SYS_TPIDR_EL0); >> return; >> +??? case TPIDRRO_EL0:??? write_sysreg_s(val, SYS_TPIDRRO_EL0); >> return; >> +??? case TPIDR_EL1:??????? write_sysreg_s(val, SYS_TPIDR_EL1); >> return; >> +??? case AMAIR_EL1:??????? write_sysreg_s(val, amair_EL12);??? return; >> +??? case CNTKCTL_EL1:??? write_sysreg_s(val, cntkctl_EL12);??? return; >> +??? case PAR_EL1:??????? write_sysreg_s(val, SYS_PAR_EL1);??? return; >> ????? } >> ? immediate_write: >> > > Cheers, > -- Julien Grall