Re: [PATCH 10/26] KVM: arm64: Refactor vcpu_{read,write}_sys_reg

2020-05-27 Thread Marc Zyngier

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

2020-05-26 Thread James Morse
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