On 12 May 2014 18:28, Marc Zyngier <marc.zyngier@xxxxxxx> wrote: > On Fri, May 09 2014 at 3:05:54 pm BST, Christoffer Dall <christoffer.dall@xxxxxxxxxx> wrote: >> On Wed, Apr 16, 2014 at 02:39:38PM +0100, Marc Zyngier wrote: [...] >>> +static struct vgic_lr vgic_v2_get_lr(const struct kvm_vcpu *vcpu, int lr) >>> +{ >>> + struct vgic_lr lr_desc; >>> + u32 val = vcpu->arch.vgic_cpu.vgic_v2.vgic_lr[lr]; >>> + >>> + lr_desc.irq = val & GICH_LR_VIRTUALID; >>> + lr_desc.source = (val >> GICH_LR_PHYSID_CPUID_SHIFT) & 0xff; >> >> shouldn't the mask here be 0xf according to the GICv2 spec? > > Actually, looks like it should be 7 instead (bits [12:10], and only when > lr_desc.irq is within 0..15). Well spotted anyway. > yes, 7, duh :) >> >>> + lr_desc.state = 0; >>> + >>> + if (val & GICH_LR_PENDING_BIT) >>> + lr_desc.state |= LR_STATE_PENDING; >>> + if (val & GICH_LR_ACTIVE_BIT) >>> + lr_desc.state |= LR_STATE_ACTIVE; >>> + if (val & GICH_LR_EOI) >>> + lr_desc.state |= LR_EOI_INT; >>> + >>> + return lr_desc; >>> +} >>> + >>> #define MK_LR_PEND(src, irq) \ >>> (GICH_LR_PENDING_BIT | ((src) << GICH_LR_PHYSID_CPUID_SHIFT) | (irq)) >>> >>> +static void vgic_v2_set_lr(struct kvm_vcpu *vcpu, int lr, >>> + struct vgic_lr lr_desc) >>> +{ >>> + u32 lr_val = MK_LR_PEND(lr_desc.source, lr_desc.irq); >> >> this looks a bit weird, the check just below suggests that you can >> convert a struct vgic_lr into an lr register for, for example, an lr >> which is just active and not pending. > > Ah, this is probably a leftover from some previous implementation. I'll > get rid of MR_LR_PEND altogether, and rely solely on lr_desc.state. > thanks >>> + >>> + if (lr_desc.state & LR_STATE_PENDING) >>> + lr_val |= GICH_LR_PENDING_BIT; >>> + if (lr_desc.state & LR_STATE_ACTIVE) >>> + lr_val |= GICH_LR_ACTIVE_BIT; >>> + if (lr_desc.state & LR_EOI_INT) >>> + lr_val |= GICH_LR_EOI; >>> + >>> + vcpu->arch.vgic_cpu.vgic_v2.vgic_lr[lr] = lr_val; >>> + >>> + /* >>> + * Despite being EOIed, the LR may not have been marked as >>> + * empty. >>> + */ >>> + if (!(lr_val & GICH_LR_STATE)) >>> + set_bit(lr, (unsigned long *)vcpu->arch.vgic_cpu.vgic_v2.vgic_elrsr); >> >> This feels weird in vgic_v2_set_lr, the name/style suggests that these >> get/set functions are just converters from a struct vgic_lr (that >> presumably common code can deal with) and architecture-specific LR >> register formats. >> >> If these functions have richer semantics than that (like maintaining the >> elrsr register), please comment that on the function. > > Yeah, this is one of the corners I really dislike, but making this a > separate vector makes things even more ugly than they already are. > > I'll add some comments, but feel free to suggest an alternative approach. > I think adding an api function called something like vgic_sync_lr_elrsr() and calling that from vgic_process_maintenance() - and move the comment about EOIed there - would be much nicer. >>> +} >>> + >>> +static const struct vgic_ops vgic_ops = { >>> + .get_lr = vgic_v2_get_lr, >>> + .set_lr = vgic_v2_set_lr, >>> +}; >>> + >>> +static inline struct vgic_lr vgic_get_lr(const struct kvm_vcpu *vcpu, int lr) >>> +{ >>> + return vgic_ops.get_lr(vcpu, lr); >>> +} >>> + >>> +static inline void vgic_set_lr(struct kvm_vcpu *vcpu, int lr, >>> + struct vgic_lr vlr) >>> +{ >>> + vgic_ops.set_lr(vcpu, lr, vlr); >>> +} >> >> inline statics in a C file? Surely the compiler is smart enough to >> inline this without any hints. > > Yup, brain fart here. > >>> + >>> +static void vgic_retire_lr(int lr_nr, int irq, struct kvm_vcpu *vcpu) >>> +{ >>> + struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu; >>> + struct vgic_lr vlr = vgic_get_lr(vcpu, lr_nr); >> >> seems like we're doing a lot of copying back and forward between the >> struct vgic_lr and the LRs on the vgic_cpu struct, I wonder if it makes >> more sense to only deal with it during the sync/flush functions, or >> maybe we end up messing with more state than necessary then? > > We're doing a lot of them, but we actually don't have much copying. The > structure is small enough to fit in a single register (even on AArch32), > and we're passing it by value. So the whole state computation actually > occurs in registers. > > But overall yes, I agree that we should probably try to do things in a > more staged way. I'll have a look. > not too important, we can always clean it up, measure it etc., later. >>> + >>> + vlr.state = 0; >>> + vgic_set_lr(vcpu, lr_nr, vlr); >>> + clear_bit(lr_nr, vgic_cpu->lr_used); >>> + vgic_cpu->vgic_irq_lr_map[irq] = LR_EMPTY; >>> +} >>> + >>> /* >>> * An interrupt may have been disabled after being made pending on the >>> * CPU interface (the classic case is a timer running while we're >>> @@ -1007,12 +1064,12 @@ static void vgic_retire_disabled_irqs(struct kvm_vcpu *vcpu) >>> int lr; >>> >>> for_each_set_bit(lr, vgic_cpu->lr_used, vgic_cpu->nr_lr) { >>> - int irq = vgic_cpu->vgic_v2.vgic_lr[lr] & GICH_LR_VIRTUALID; >>> + struct vgic_lr vlr = vgic_get_lr(vcpu, lr); >>> >>> - if (!vgic_irq_is_enabled(vcpu, irq)) { >>> - vgic_retire_lr(lr, irq, vgic_cpu); >>> - if (vgic_irq_is_active(vcpu, irq)) >>> - vgic_irq_clear_active(vcpu, irq); >>> + if (!vgic_irq_is_enabled(vcpu, vlr.irq)) { >>> + vgic_retire_lr(lr, vlr.irq, vcpu); >>> + if (vgic_irq_is_active(vcpu, vlr.irq)) >>> + vgic_irq_clear_active(vcpu, vlr.irq); >>> } >>> } >>> } >>> @@ -1024,6 +1081,7 @@ static void vgic_retire_disabled_irqs(struct kvm_vcpu *vcpu) >>> static bool vgic_queue_irq(struct kvm_vcpu *vcpu, u8 sgi_source_id, int irq) >>> { >>> struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu; >>> + struct vgic_lr vlr; >>> int lr; >>> >>> /* Sanitize the input... */ >>> @@ -1036,13 +1094,15 @@ static bool vgic_queue_irq(struct kvm_vcpu *vcpu, u8 sgi_source_id, int irq) >>> lr = vgic_cpu->vgic_irq_lr_map[irq]; >>> >>> /* Do we have an active interrupt for the same CPUID? */ >>> - if (lr != LR_EMPTY && >>> - (LR_CPUID(vgic_cpu->vgic_v2.vgic_lr[lr]) == sgi_source_id)) { >>> - kvm_debug("LR%d piggyback for IRQ%d %x\n", >>> - lr, irq, vgic_cpu->vgic_v2.vgic_lr[lr]); >>> - BUG_ON(!test_bit(lr, vgic_cpu->lr_used)); >>> - vgic_cpu->vgic_v2.vgic_lr[lr] |= GICH_LR_PENDING_BIT; >>> - return true; >>> + if (lr != LR_EMPTY) { >>> + vlr = vgic_get_lr(vcpu, lr); >>> + if (vlr.source == sgi_source_id) { >>> + kvm_debug("LR%d piggyback for IRQ%d\n", lr, vlr.irq); >>> + BUG_ON(!test_bit(lr, vgic_cpu->lr_used)); >>> + vlr.state |= LR_STATE_PENDING; >>> + vgic_set_lr(vcpu, lr, vlr); >>> + return true; >>> + } >>> } >>> >>> /* Try to use another LR for this interrupt */ >>> @@ -1052,12 +1112,16 @@ static bool vgic_queue_irq(struct kvm_vcpu *vcpu, u8 sgi_source_id, int irq) >>> return false; >>> >>> kvm_debug("LR%d allocated for IRQ%d %x\n", lr, irq, sgi_source_id); >>> - vgic_cpu->vgic_v2.vgic_lr[lr] = MK_LR_PEND(sgi_source_id, irq); >>> vgic_cpu->vgic_irq_lr_map[irq] = lr; >>> set_bit(lr, vgic_cpu->lr_used); >>> >>> + vlr.irq = irq; >>> + vlr.source = sgi_source_id; >>> + vlr.state = LR_STATE_PENDING; >>> if (!vgic_irq_is_edge(vcpu, irq)) >>> - vgic_cpu->vgic_v2.vgic_lr[lr] |= GICH_LR_EOI; >>> + vlr.state |= LR_EOI_INT; >>> + >>> + vgic_set_lr(vcpu, lr, vlr); >>> >>> return true; >>> } >>> @@ -1180,29 +1244,23 @@ static bool vgic_process_maintenance(struct kvm_vcpu *vcpu) >>> * Some level interrupts have been EOIed. Clear their >>> * active bit. >>> */ >>> - int lr, irq; >>> + int lr; >>> >>> for_each_set_bit(lr, (unsigned long *)vgic_cpu->vgic_v2.vgic_eisr, >>> vgic_cpu->nr_lr) { >>> - irq = vgic_cpu->vgic_v2.vgic_lr[lr] & GICH_LR_VIRTUALID; >>> + struct vgic_lr vlr = vgic_get_lr(vcpu, lr); >>> >>> - vgic_irq_clear_active(vcpu, irq); >>> - vgic_cpu->vgic_v2.vgic_lr[lr] &= ~GICH_LR_EOI; >>> + vgic_irq_clear_active(vcpu, vlr.irq); >>> + vlr.state = 0; >> >> slight change of semantics here. It is still correct, but only because >> we never set the pending bit on an already active level interrupt, but I >> guess this could technically be closer to real hardware by allowing >> world-switching VMs that are processing active interrupts to pick up an >> additional pending state, in which case just setting state = 0 would be >> incorrect here, and you should instead lower the EOI mask like you did >> before. >> >> That being said, it is a pseudo-theoretical point, and you can make me >> more happy by adding: >> >> BUG_ON(vlr.state & LR_STATE_MASK); >> >> before clearing the state completely. > > Putting a BUG_ON() seems a bit harsh. WARN_ON()? > yeah, let's take it easy. >>> + vgic_set_lr(vcpu, lr, vlr); >>> >>> /* Any additional pending interrupt? */ >>> - if (vgic_dist_irq_is_pending(vcpu, irq)) { >>> - vgic_cpu_irq_set(vcpu, irq); >>> + if (vgic_dist_irq_is_pending(vcpu, vlr.irq)) { >>> + vgic_cpu_irq_set(vcpu, vlr.irq); >>> level_pending = true; >>> } else { >>> - vgic_cpu_irq_clear(vcpu, irq); >>> + vgic_cpu_irq_clear(vcpu, vlr.irq); >>> } >>> - >>> - /* >>> - * Despite being EOIed, the LR may not have >>> - * been marked as empty. >>> - */ >>> - set_bit(lr, (unsigned long *)vgic_cpu->vgic_v2.vgic_elrsr); >>> - vgic_cpu->vgic_v2.vgic_lr[lr] &= ~GICH_LR_ACTIVE_BIT; >>> } >>> } >>> >>> @@ -1228,15 +1286,15 @@ static void __kvm_vgic_sync_hwstate(struct kvm_vcpu *vcpu) >>> /* Clear mappings for empty LRs */ >>> for_each_set_bit(lr, (unsigned long *)vgic_cpu->vgic_v2.vgic_elrsr, >>> vgic_cpu->nr_lr) { >>> - int irq; >>> + struct vgic_lr vlr; >>> >>> if (!test_and_clear_bit(lr, vgic_cpu->lr_used)) >>> continue; >>> >>> - irq = vgic_cpu->vgic_v2.vgic_lr[lr] & GICH_LR_VIRTUALID; >>> + vlr = vgic_get_lr(vcpu, lr); >>> >>> - BUG_ON(irq >= VGIC_NR_IRQS); >>> - vgic_cpu->vgic_irq_lr_map[irq] = LR_EMPTY; >>> + BUG_ON(vlr.irq >= VGIC_NR_IRQS); >>> + vgic_cpu->vgic_irq_lr_map[vlr.irq] = LR_EMPTY; >>> } >>> >>> /* Check if we still have something up our sleeve... */ >>> -- >>> 1.8.3.4 >>> -Christoffer _______________________________________________ kvmarm mailing list kvmarm@xxxxxxxxxxxxxxxxxxxxx https://lists.cs.columbia.edu/mailman/listinfo/kvmarm