Re: [PATCH 10/26] KVM: arm64: Refactor vcpu_{read,write}_sys_reg
On 2020-05-26 17:28, James Morse wrote:
Hi Marc,
On 22/04/2020 13:00, Marc Zyngier wrote:
Extract the direct HW accessors for later reuse.
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 51db934702b64..46f218982df8c 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
+u64 vcpu_read_sys_reg(const struct kvm_vcpu *vcpu, int reg)
+{
+ u64 val = 0x8badf00d8badf00d;
+
+ if (!vcpu->arch.sysregs_loaded_on_cpu) {
+ goto memory_read;
}
-immediate_write:
+ if (__vcpu_read_sys_reg_from_cpu(reg, &val))
+ return val;
+
+memory_read:
+ return __vcpu_sys_reg(vcpu, reg);
+}
The goto here is a bit odd, is it just an artefact of how we got here?
That's because a lot of this changes when NV gets added to the mix,
see [1].
Is this easier on the eye?:
| u64 vcpu_read_sys_reg(const struct kvm_vcpu *vcpu, int reg)
| {
| u64 val = 0x8badf00d8badf00d;
|
| if (vcpu->arch.sysregs_loaded_on_cpu &&
| __vcpu_read_sys_reg_from_cpu(reg, &val))
| return val;
|
| return __vcpu_sys_reg(vcpu, reg);
| }
Definitely. I don't mind reworking the NV branch so that the label
gets introduced there.
+void vcpu_write_sys_reg(struct kvm_vcpu *vcpu, u64 val, int reg)
+{
+ if (!vcpu->arch.sysregs_loaded_on_cpu)
+ goto memory_write;
+
+ if (__vcpu_write_sys_reg_to_cpu(val, reg))
+ return;
+
+memory_write:
__vcpu_sys_reg(vcpu, reg) = val;
}
Again I think its clearer without the goto
Regardless:
Reviewed-by: James Morse
Thanks,
M.
[1]
https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/commit/?h=kvm-arm64/nv-5.7-rc1-WIP&id=11f3217d39a602cbfac7d08064c8b31afb57348e
--
Jazz is not dead. It just smells funny...
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH 10/26] KVM: arm64: Refactor vcpu_{read,write}_sys_reg
Hi Marc,
On 22/04/2020 13:00, Marc Zyngier wrote:
> Extract the direct HW accessors for later reuse.
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 51db934702b64..46f218982df8c 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> +u64 vcpu_read_sys_reg(const struct kvm_vcpu *vcpu, int reg)
> +{
> + u64 val = 0x8badf00d8badf00d;
> +
> + if (!vcpu->arch.sysregs_loaded_on_cpu) {
> + goto memory_read;
> }
>
> -immediate_write:
> + if (__vcpu_read_sys_reg_from_cpu(reg, &val))
> + return val;
> +
> +memory_read:
> + return __vcpu_sys_reg(vcpu, reg);
> +}
The goto here is a bit odd, is it just an artefact of how we got here?
Is this easier on the eye?:
| u64 vcpu_read_sys_reg(const struct kvm_vcpu *vcpu, int reg)
| {
| u64 val = 0x8badf00d8badf00d;
|
| if (vcpu->arch.sysregs_loaded_on_cpu &&
| __vcpu_read_sys_reg_from_cpu(reg, &val))
| return val;
|
| return __vcpu_sys_reg(vcpu, reg);
| }
> +void vcpu_write_sys_reg(struct kvm_vcpu *vcpu, u64 val, int reg)
> +{
> + if (!vcpu->arch.sysregs_loaded_on_cpu)
> + goto memory_write;
> +
> + if (__vcpu_write_sys_reg_to_cpu(val, reg))
> + return;
> +
> +memory_write:
>__vcpu_sys_reg(vcpu, reg) = val;
> }
Again I think its clearer without the goto
Regardless:
Reviewed-by: James Morse
Thanks,
James
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
[PATCH 10/26] KVM: arm64: Refactor vcpu_{read,write}_sys_reg
Extract the direct HW accessors for later reuse.
Signed-off-by: Marc Zyngier
---
arch/arm64/kvm/sys_regs.c | 135 ++
1 file changed, 78 insertions(+), 57 deletions(-)
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 51db934702b64..46f218982df8c 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -64,11 +64,8 @@ static bool write_to_read_only(struct kvm_vcpu *vcpu,
return false;
}
-u64 vcpu_read_sys_reg(const struct kvm_vcpu *vcpu, int reg)
+static bool __vcpu_read_sys_reg_from_cpu(int reg, u64 *val)
{
- if (!vcpu->arch.sysregs_loaded_on_cpu)
- goto immediate_read;
-
/*
* System registers listed in the switch are not saved on every
* exit from the guest but are only saved on vcpu_put.
@@ -79,40 +76,37 @@ u64 vcpu_read_sys_reg(const struct kvm_vcpu *vcpu, int reg)
* thread when emulating cross-VCPU communication.
*/
switch (reg) {
- case CSSELR_EL1:return read_sysreg_s(SYS_CSSELR_EL1);
- case SCTLR_EL1: return read_sysreg_s(SYS_SCTLR_EL12);
- case ACTLR_EL1: return read_sysreg_s(SYS_ACTLR_EL1);
- case CPACR_EL1: return read_sysreg_s(SYS_CPACR_EL12);
- case TTBR0_EL1: return read_sysreg_s(SYS_TTBR0_EL12);
- case TTBR1_EL1: return read_sysreg_s(SYS_TTBR1_EL12);
- case TCR_EL1: return read_sysreg_s(SYS_TCR_EL12);
- case ESR_EL1: return read_sysreg_s(SYS_ESR_EL12);
- case AFSR0_EL1: return read_sysreg_s(SYS_AFSR0_EL12);
- case AFSR1_EL1: return read_sysreg_s(SYS_AFSR1_EL12);
- case FAR_EL1: return read_sysreg_s(SYS_FAR_EL12);
- case MAIR_EL1: return read_sysreg_s(SYS_MAIR_EL12);
- case VBAR_EL1: return read_sysreg_s(SYS_VBAR_EL12);
- case CONTEXTIDR_EL1:return read_sysreg_s(SYS_CONTEXTIDR_EL12);
- case TPIDR_EL0: return read_sysreg_s(SYS_TPIDR_EL0);
- case TPIDRRO_EL0: return read_sysreg_s(SYS_TPIDRRO_EL0);
- case TPIDR_EL1: return read_sysreg_s(SYS_TPIDR_EL1);
- case AMAIR_EL1: return read_sysreg_s(SYS_AMAIR_EL12);
- case CNTKCTL_EL1: return read_sysreg_s(SYS_CNTKCTL_EL12);
- case PAR_EL1: return read_sysreg_s(SYS_PAR_EL1);
- case DACR32_EL2:return read_sysreg_s(SYS_DACR32_EL2);
- case IFSR32_EL2:return read_sysreg_s(SYS_IFSR32_EL2);
- case DBGVCR32_EL2: return read_sysreg_s(SYS_DBGVCR32_EL2);
+ case CSSELR_EL1:*val = read_sysreg_s(SYS_CSSELR_EL1); break;
+ case SCTLR_EL1: *val = read_sysreg_s(SYS_SCTLR_EL12); break;
+ case ACTLR_EL1: *val = read_sysreg_s(SYS_ACTLR_EL1);break;
+ case CPACR_EL1: *val = read_sysreg_s(SYS_CPACR_EL12); break;
+ case TTBR0_EL1: *val = read_sysreg_s(SYS_TTBR0_EL12); break;
+ case TTBR1_EL1: *val = read_sysreg_s(SYS_TTBR1_EL12); break;
+ case TCR_EL1: *val = read_sysreg_s(SYS_TCR_EL12); break;
+ case ESR_EL1: *val = read_sysreg_s(SYS_ESR_EL12); break;
+ case AFSR0_EL1: *val = read_sysreg_s(SYS_AFSR0_EL12); break;
+ case AFSR1_EL1: *val = read_sysreg_s(SYS_AFSR1_EL12); break;
+ case FAR_EL1: *val = read_sysreg_s(SYS_FAR_EL12); break;
+ case MAIR_EL1: *val = read_sysreg_s(SYS_MAIR_EL12);break;
+ case VBAR_EL1: *val = read_sysreg_s(SYS_VBAR_EL12);break;
+ case CONTEXTIDR_EL1:*val = read_sysreg_s(SYS_CONTEXTIDR_EL12);break;
+ case TPIDR_EL0: *val = read_sysreg_s(SYS_TPIDR_EL0);break;
+ case TPIDRRO_EL0: *val = read_sysreg_s(SYS_TPIDRRO_EL0); break;
+ case TPIDR_EL1: *val = read_sysreg_s(SYS_TPIDR_EL1);break;
+ case AMAIR_EL1: *val = read_sysreg_s(SYS_AMAIR_EL12); break;
+ case CNTKCTL_EL1: *val = read_sysreg_s(SYS_CNTKCTL_EL12); break;
+ case PAR_EL1: *val = read_sysreg_s(SYS_PAR_EL1); break;
+ case DACR32_EL2:*val = read_sysreg_s(SYS_DACR32_EL2); break;
+ case IFSR32_EL2:*val = read_sysreg_s(SYS_IFSR32_EL2); break;
+ case DBGVCR32_EL2: *val = read_sysreg_s(SYS_DBGVCR32_EL2); break;
+ default:return false;
}
-immediate_read:
- return __vcpu_sys_reg(vcpu, reg);
+ return true;
}
-void vcpu_write_sys_reg(struct kvm_vcpu *vcpu, u64 val, int reg)
+static bool __vcpu_write_sys_reg_to_cpu(u64 val, int reg)
{
- if (!vcpu->arch.sysregs_loaded_on_cpu)
- goto immediate_write;
-
/*
* System registers listed in the switch are not restored on every
* entry to the guest but are only restored on vcpu_load.
@@ -122,32 +116,59 @@ void vcpu_write_sys_reg(struct
