On Wed, Nov 15, 2017 at 05:50:07PM +0000, Andre Przywara wrote: > Hi, > > those last few patches are actually helpful for the Xen port ...
[...] > > +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 > > I have some gut feeling that this is really broken, since we mix up > endian *byte* ordering with *bit* ordering here, don't we? Good feeling indeed. :) We have bitmap_{from,to)_u32array for things like this. But it was considered bad-designed, and I proposed new bitmap_{from,to)_arr32(). https://lkml.org/lkml/2017/11/15/592 What else I have in mind, to introduce something like bitmap_{from,to}_pair_32() as most of current users of bitmap_{from,to)_u32array(), (and those who should use it but don't, like this one) have only 2 32-bit halfwords to be copied from/to bitmap. Also, it will be complementary to bitmap_from_u64(). More reading about bitmap/array conversion is in comment to BITMAP_FROM_U64 macro. > I understand it's just copied and gets removed later on, so I was > wondering if you could actually move patch 35/37 ("Get rid of > vgic_elrsr") before this patch here, to avoid copying bogus code around? > Or does 35/37 depend on 34/37 to be correct? > > > +} > > + > > +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; > > + > > + for (i = 0; i < used_lrs; i++) { > > + if (cpu_if->vgic_elrsr & (1UL << i)) So, the vgic_elrsr is naturally bitmap, and bitmap API is preferred if no other considerations: if (test_bit(i, cpu_if->vgic_elrsr)) > > + cpu_if->vgic_lr[i] &= ~GICH_LR_STATE; > > + else > > + cpu_if->vgic_lr[i] = readl_relaxed(base + GICH_LR0 + (i > > * 4)); > > + > > + writel_relaxed(0, base + GICH_LR0 + (i * 4)); > > + } > > +} I'd also headscratch about using for_each_clear_bit() here: /* * Setup default vgic_lr values somewhere earlier. * Not needed at all if you take my suggestion for * vgic_v2_restore_state() below */ for (i = 0; i < used_lrs; i++) cpu_if->vgic_lr[i] &= ~GICH_LR_STATE; static void save_lrs(struct kvm_vcpu *vcpu, void __iomem *base) { [...] for_each_clear_bit (i, cpu_if->vgic_elrsr, used_lrs) cpu_if->vgic_lr[i] = readl_relaxed(base + GICH_LR0 + (i * 4)); for (i = 0; i < used_lrs; i++) writel_relaxed(0, base + GICH_LR0 + (i * 4)); } Not sure how performance-critical this path is, but sometimes things get really faster with bitmaps. [...] > > +void vgic_v2_restore_state(struct kvm_vcpu *vcpu) > > +{ > > + struct kvm *kvm = vcpu->kvm; > > + struct vgic_dist *vgic = &kvm->arch.vgic; > > + struct vgic_v2_cpu_if *cpu_if = &vcpu->arch.vgic_cpu.vgic_v2; > > + void __iomem *base = vgic->vctrl_base; > > + u64 used_lrs = vcpu->arch.vgic_cpu.used_lrs; > > + int i; > > + > > + if (!base) > > + return; > > + > > + if (used_lrs) { > > + writel_relaxed(cpu_if->vgic_hcr, base + GICH_HCR); > > + writel_relaxed(cpu_if->vgic_apr, base + GICH_APR); > > + for (i = 0; i < used_lrs; i++) { > > + writel_relaxed(cpu_if->vgic_lr[i], > > + base + GICH_LR0 + (i * 4)); > > + } > > + } > > +} The alternative approach would be: for (i = 0; i < used_lrs; i++) { if (test_bit(i, cpu_if->vgic_elrsr)) writel_relaxed(~GICH_LR_STATE, base + GICH_LR0 + (i * 4)); else writel_relaxed(cpu_if->vgic_lr[i], base + GICH_LR0 + (i * 4)); } If cpu_if->vgic_elrsr is untouched in-between of course. It will make save_lrs() simpler and this function more verbose. Thanks, Yury _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm