On Thu, Oct 12, 2017 at 12:41:39PM +0200, Christoffer Dall wrote:
> There is really no need to store the vgic_elrsr on the VGIC data
> structures as the only need we have for the elrsr is to figure out if an
> LR is inavtive when we save the VGIC state upon returning from the
> guest. We can might as well store this in a temporary local variable.
>
> This also gets rid of the endianness conversion in the VGIC save
> function, which is completely unnecessary and would actually result in
> incorrect functionality on big-endian systems,
Does it mean that existing code in mainline is broken for BE systems?
If so, I think that it should be fixed in separated patch...
> because we are only using
> typed values here and not converting pointers and reading different
> types here.
>
> Signed-off-by: Christoffer Dall <christoffer.dall@xxxxxxxxxx>
> ---
> include/kvm/arm_vgic.h | 2 --
> virt/kvm/arm/hyp/vgic-v3-sr.c | 6 +++---
> virt/kvm/arm/vgic/vgic-v2.c | 33 +++++++--------------------------
> virt/kvm/arm/vgic/vgic-v3.c | 1 -
> 4 files changed, 10 insertions(+), 32 deletions(-)
>
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index 34dba51..79c9e67 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -237,7 +237,6 @@ struct vgic_dist {
> struct vgic_v2_cpu_if {
> u32 vgic_hcr;
> u32 vgic_vmcr;
> - u64 vgic_elrsr; /* Saved only */
> u32 vgic_apr;
> u32 vgic_lr[VGIC_V2_MAX_LRS];
> };
> @@ -246,7 +245,6 @@ struct vgic_v3_cpu_if {
> u32 vgic_hcr;
> u32 vgic_vmcr;
> u32 vgic_sre; /* Restored only, change ignored */
> - u32 vgic_elrsr; /* Saved only */
> u32 vgic_ap0r[4];
> u32 vgic_ap1r[4];
> u64 vgic_lr[VGIC_V3_MAX_LRS];
> diff --git a/virt/kvm/arm/hyp/vgic-v3-sr.c b/virt/kvm/arm/hyp/vgic-v3-sr.c
> index 91728fa..05548b2 100644
> --- a/virt/kvm/arm/hyp/vgic-v3-sr.c
> +++ b/virt/kvm/arm/hyp/vgic-v3-sr.c
> @@ -222,15 +222,16 @@ void __hyp_text __vgic_v3_save_state(struct kvm_vcpu *vcpu)
> if (used_lrs) {
> int i;
> u32 nr_pre_bits;
> + u32 elrsr;
>
> - cpu_if->vgic_elrsr = read_gicreg(ICH_ELSR_EL2);
> + elrsr = read_gicreg(ICH_ELSR_EL2);
>
> write_gicreg(0, ICH_HCR_EL2);
> val = read_gicreg(ICH_VTR_EL2);
> nr_pre_bits = vtr_to_nr_pre_bits(val);
>
> for (i = 0; i < used_lrs; i++) {
> - if (cpu_if->vgic_elrsr & (1 << i))
> + if (elrsr & (1 << i))
Same comments as to patch 28. Here should be test_bit(), I think.
And if it's possible, for set bits in elrsr it's simpler not to write
~ICH_LR_STATE to cpu_if->vgic_lr, but directly to IO memory in
__vgic_v3_restore_state().
> cpu_if->vgic_lr[i] &= ~ICH_LR_STATE;
> else
> cpu_if->vgic_lr[i] = __gic_v3_get_lr(i);
> @@ -261,7 +262,6 @@ void __hyp_text __vgic_v3_save_state(struct kvm_vcpu *vcpu)
> if (static_branch_unlikely(&vgic_v3_cpuif_trap))
> write_gicreg(0, ICH_HCR_EL2);
>
> - cpu_if->vgic_elrsr = 0xffff;
> cpu_if->vgic_ap0r[0] = 0;
> cpu_if->vgic_ap0r[1] = 0;
> cpu_if->vgic_ap0r[2] = 0;
> diff --git a/virt/kvm/arm/vgic/vgic-v2.c b/virt/kvm/arm/vgic/vgic-v2.c
> index 259079b..df7e03b 100644
> --- a/virt/kvm/arm/vgic/vgic-v2.c
> +++ b/virt/kvm/arm/vgic/vgic-v2.c
> @@ -247,7 +247,6 @@ void vgic_v2_enable(struct kvm_vcpu *vcpu)
> * anyway.
> */
> vcpu->arch.vgic_cpu.vgic_v2.vgic_vmcr = 0;
> - vcpu->arch.vgic_cpu.vgic_v2.vgic_elrsr = ~0;
>
> /* Get the show on the road... */
> vcpu->arch.vgic_cpu.vgic_v2.vgic_hcr = GICH_HCR_EN;
> @@ -404,33 +403,19 @@ int vgic_v2_probe(const struct gic_kvm_info *info)
> return ret;
> }
>
> -static void save_elrsr(struct kvm_vcpu *vcpu, void __iomem *base)
> -{
> - struct vgic_v2_cpu_if *cpu_if = &vcpu->arch.vgic_cpu.vgic_v2;
> - int nr_lr = kvm_vgic_global_state.nr_lr;
> - u32 elrsr0, elrsr1;
> -
> - elrsr0 = readl_relaxed(base + GICH_ELRSR0);
> - if (unlikely(nr_lr > 32))
> - elrsr1 = readl_relaxed(base + GICH_ELRSR1);
> - else
> - elrsr1 = 0;
> -
> -#ifdef CONFIG_CPU_BIG_ENDIAN
> - cpu_if->vgic_elrsr = ((u64)elrsr0 << 32) | elrsr1;
> -#else
> - cpu_if->vgic_elrsr = ((u64)elrsr1 << 32) | elrsr0;
> -#endif
> -}
> -
> static void save_lrs(struct kvm_vcpu *vcpu, void __iomem *base)
> {
> struct vgic_v2_cpu_if *cpu_if = &vcpu->arch.vgic_cpu.vgic_v2;
> - int i;
> u64 used_lrs = vcpu->arch.vgic_cpu.used_lrs;
Not to this patch, but anyway, what for to reserve u64 for used_lrs
here and in struct vgic_cpu, when maximum value for it is 64?
> + u64 elrsr;
> + int i;
Nit. What for you move 'int i' declaration?
> +
> + elrsr = readl_relaxed(base + GICH_ELRSR0);
> + if (unlikely(used_lrs > 32))
> + elrsr |= ((u64)readl_relaxed(base + GICH_ELRSR1)) << 32;
>
> for (i = 0; i < used_lrs; i++) {
> - if (cpu_if->vgic_elrsr & (1UL << i))
> + if (elrsr & (1UL << i))
test_bit()
> cpu_if->vgic_lr[i] &= ~GICH_LR_STATE;
> else
> cpu_if->vgic_lr[i] = readl_relaxed(base + GICH_LR0 + (i * 4));
> @@ -452,13 +437,9 @@ void vgic_v2_save_state(struct kvm_vcpu *vcpu)
>
> if (used_lrs) {
> cpu_if->vgic_apr = readl_relaxed(base + GICH_APR);
> -
> - save_elrsr(vcpu, base);
> save_lrs(vcpu, base);
> -
> writel_relaxed(0, base + GICH_HCR);
> } else {
> - cpu_if->vgic_elrsr = ~0UL;
> cpu_if->vgic_apr = 0;
> }
> }
> diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
> index 863351c..0900649 100644
> --- a/virt/kvm/arm/vgic/vgic-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-v3.c
> @@ -237,7 +237,6 @@ void vgic_v3_enable(struct kvm_vcpu *vcpu)
> * anyway.
> */
> vgic_v3->vgic_vmcr = 0;
> - vgic_v3->vgic_elrsr = ~0;
>
> /*
> * If we are emulating a GICv3, we do it in an non-GICv2-compatible
> --
> 2.9.0
