Salut Marc,
I know that this patch is already merged, but ....
On 07/08/15 16:45, Marc Zyngier wrote:
> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> index 51c9900..9d009d2 100644
...
> @@ -1364,6 +1397,39 @@ static bool vgic_process_maintenance(struct kvm_vcpu *vcpu)
> return level_pending;
> }
>
> +/*
> + * Save the physical active state, and reset it to inactive.
> + *
> + * Return 1 if HW interrupt went from active to inactive, and 0 otherwise.
> + */
> +static int vgic_sync_hwirq(struct kvm_vcpu *vcpu, struct vgic_lr vlr)
> +{
> + struct irq_phys_map *map;
> + int ret;
> +
> + if (!(vlr.state & LR_HW))
> + return 0;
> +
> + map = vgic_irq_map_search(vcpu, vlr.irq);
> + BUG_ON(!map || !map->active);
> +
> + ret = irq_get_irqchip_state(map->irq,
> + IRQCHIP_STATE_ACTIVE,
> + &map->active);
> +
> + WARN_ON(ret);
> +
> + if (map->active) {
> + ret = irq_set_irqchip_state(map->irq,
> + IRQCHIP_STATE_ACTIVE,
> + false);
> + WARN_ON(ret);
> + return 0;
> + }
> +
> + return 1;
> +}
> +
> /* Sync back the VGIC state after a guest run */
> static void __kvm_vgic_sync_hwstate(struct kvm_vcpu *vcpu)
> {
> @@ -1378,14 +1444,31 @@ static void __kvm_vgic_sync_hwstate(struct kvm_vcpu *vcpu)
> elrsr = vgic_get_elrsr(vcpu);
> elrsr_ptr = u64_to_bitmask(&elrsr);
>
> - /* Clear mappings for empty LRs */
> - for_each_set_bit(lr, elrsr_ptr, vgic->nr_lr) {
> + /* Deal with HW interrupts, and clear mappings for empty LRs */
> + for (lr = 0; lr < vgic->nr_lr; lr++) {
> struct vgic_lr vlr;
>
> - if (!test_and_clear_bit(lr, vgic_cpu->lr_used))
> + if (!test_bit(lr, vgic_cpu->lr_used))
> continue;
>
> vlr = vgic_get_lr(vcpu, lr);
> + if (vgic_sync_hwirq(vcpu, vlr)) {
> + /*
> + * So this is a HW interrupt that the guest
> + * EOI-ed. Clean the LR state and allow the
> + * interrupt to be sampled again.
> + */
> + vlr.state = 0;
> + vlr.hwirq = 0;
> + vgic_set_lr(vcpu, lr, vlr);
> + vgic_irq_clear_queued(vcpu, vlr.irq);
Isn't this line altering common VGIC state without holding the lock?
Eric removed the coarse dist->lock around the whole
__kvm_vgic_sync_hwstate() function, we take it now in
vgic_process_maintenance(), but don't hold it here AFAICT.
As long as we are only dealing with private timer IRQs this is probably
not a problem, but the IRQ number could be a SPI as well, right?
Cheers,
Andre.
> + set_bit(lr, elrsr_ptr);
> + }
> +
> + if (!test_bit(lr, elrsr_ptr))
> + continue;
> +
> + clear_bit(lr, vgic_cpu->lr_used);
>
> BUG_ON(vlr.irq >= dist->nr_irqs);
> vgic_cpu->vgic_irq_lr_map[vlr.irq] = LR_EMPTY;
>
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
