Re: [PATCH 06/10] ARM: KVM: vgic: sanitize use of bitmaps

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Nov 21, 2012 at 1:15 PM, Marc Zyngier <marc.zyngier@xxxxxxx> wrote:
> Various bits of the vgic code blindly assume that the size of u32
> and unsigned long are the same. This leads to amusing side effects
> on 64bit platforms, and makes the code hard to follow.
>
> Move to proper bitmaps instead of the home grown version, and split
> the vcpu specific "pending" bitmap into pending_percpu and pending_shared.
>
> Signed-off-by: Marc Zyngier <marc.zyngier@xxxxxxx>
> ---
>  arch/arm/include/asm/kvm_vgic.h |  7 +++---
>  arch/arm/kvm/vgic.c             | 55 +++++++++++++++++++++++++++--------------
>  2 files changed, 41 insertions(+), 21 deletions(-)
>
> diff --git a/arch/arm/include/asm/kvm_vgic.h b/arch/arm/include/asm/kvm_vgic.h
> index e371100..7a5e196 100644
> --- a/arch/arm/include/asm/kvm_vgic.h
> +++ b/arch/arm/include/asm/kvm_vgic.h
> @@ -51,11 +51,11 @@
>  struct vgic_bitmap {
>         union {
>                 u32 reg[1];
> -               unsigned long reg_ul[0];
> +               DECLARE_BITMAP(reg_ul, 32);
>         } percpu[VGIC_MAX_CPUS];
>         union {
>                 u32 reg[VGIC_NR_SHARED_IRQS / 32];
> -               unsigned long reg_ul[0];
> +               DECLARE_BITMAP(reg_ul, VGIC_NR_SHARED_IRQS);
>         } shared;
>  };
>
> @@ -199,7 +199,8 @@ struct vgic_cpu {
>         u8              vgic_irq_lr_map[VGIC_NR_IRQS];
>
>         /* Pending interrupts on this VCPU */
> -       DECLARE_BITMAP( pending, VGIC_NR_IRQS);
> +       DECLARE_BITMAP( pending_percpu, 32);
> +       DECLARE_BITMAP( pending_shared, VGIC_NR_SHARED_IRQS);
>
>         /* Bitmap of used/free list registers */
>         DECLARE_BITMAP( lr_used, 64);
> diff --git a/arch/arm/kvm/vgic.c b/arch/arm/kvm/vgic.c
> index e1d25b4..57929db 100644
> --- a/arch/arm/kvm/vgic.c
> +++ b/arch/arm/kvm/vgic.c
> @@ -100,6 +100,22 @@ static inline int vgic_irq_is_edge(struct vgic_dist *dist, int irq)
>         return vgic_bitmap_get_irq_val(&dist->irq_cfg, 0, irq);
>  }
>
> +static inline void kvm_vgic_vcpu_set_pending_irq(struct kvm_vcpu *vcpu, int irq)
> +{
> +       if (irq < 32)
> +               set_bit(irq, vcpu->arch.vgic_cpu.pending_percpu);
> +       else
> +               set_bit(irq - 32, vcpu->arch.vgic_cpu.pending_shared);
> +}
> +
> +static inline void kvm_vgic_vcpu_clear_pending_irq(struct kvm_vcpu *vcpu, int irq)
> +{
> +       if (irq < 32)
> +               clear_bit(irq, vcpu->arch.vgic_cpu.pending_percpu);
> +       else
> +               clear_bit(irq - 32, vcpu->arch.vgic_cpu.pending_shared);
> +}
> +
>  /**
>   * vgic_reg_access - access vgic register
>   * @mmio:   pointer to the data describing the mmio access
> @@ -613,24 +629,26 @@ static void vgic_dispatch_sgi(struct kvm_vcpu *vcpu, u32 reg)
>  static int compute_pending_for_cpu(struct kvm_vcpu *vcpu)
>  {
>         struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
> -       unsigned long *pending, *enabled, *pend;
> +       unsigned long *pending, *enabled, *pend_percpu, *pend_shared;
>         int vcpu_id;
>
>         vcpu_id = vcpu->vcpu_id;
> -       pend = vcpu->arch.vgic_cpu.pending;
> +       pend_percpu = vcpu->arch.vgic_cpu.pending_percpu;
> +       pend_shared = vcpu->arch.vgic_cpu.pending_shared;
>
>         pending = vgic_bitmap_get_cpu_map(&dist->irq_state, vcpu_id);
>         enabled = vgic_bitmap_get_cpu_map(&dist->irq_enabled, vcpu_id);
> -       bitmap_and(pend, pending, enabled, 32);
> +       bitmap_and(pend_percpu, pending, enabled, 32);
>
>         pending = vgic_bitmap_get_shared_map(&dist->irq_state);
>         enabled = vgic_bitmap_get_shared_map(&dist->irq_enabled);
> -       bitmap_and(pend + 1, pending, enabled, VGIC_NR_SHARED_IRQS);
> -       bitmap_and(pend + 1, pend + 1,
> +       bitmap_and(pend_shared, pending, enabled, VGIC_NR_SHARED_IRQS);
> +       bitmap_and(pend_shared, pend_shared,
>                    vgic_bitmap_get_shared_map(&dist->irq_spi_target[vcpu_id]),
>                    VGIC_NR_SHARED_IRQS);
>
> -       return (find_first_bit(pend, VGIC_NR_IRQS) < VGIC_NR_IRQS);
> +       return (find_first_bit(pend_percpu, 32) < 32 ||
> +               find_first_bit(pend_shared, VGIC_NR_SHARED_IRQS) < VGIC_NR_SHARED_IRQS);
>  }
>
>  /*
> @@ -767,7 +785,7 @@ static void __kvm_vgic_sync_to_cpu(struct kvm_vcpu *vcpu)
>
>         /* SGIs */
>         pending = vgic_bitmap_get_cpu_map(&dist->irq_state, vcpu_id);
> -       for_each_set_bit(i, vgic_cpu->pending, 16) {
> +       for_each_set_bit(i, vgic_cpu->pending_percpu, 16) {
>                 unsigned long sources;
>
>                 sources = dist->irq_sgi_sources[vcpu_id][i];
> @@ -787,7 +805,7 @@ static void __kvm_vgic_sync_to_cpu(struct kvm_vcpu *vcpu)
>         }
>
>         /* PPIs */
> -       for_each_set_bit_from(i, vgic_cpu->pending, 32) {
> +       for_each_set_bit_from(i, vgic_cpu->pending_percpu, 32) {
>                 if (!vgic_queue_irq(vcpu, 0, i)) {
>                         overflow = 1;
>                         continue;
> @@ -799,21 +817,22 @@ static void __kvm_vgic_sync_to_cpu(struct kvm_vcpu *vcpu)
>
>         /* SPIs */
>         pending = vgic_bitmap_get_shared_map(&dist->irq_state);
> -       for_each_set_bit_from(i, vgic_cpu->pending, VGIC_NR_IRQS) {
> -               if (vgic_bitmap_get_irq_val(&dist->irq_active, 0, i))
> +       for_each_set_bit(i, vgic_cpu->pending_shared, VGIC_NR_SHARED_IRQS) {
> +               int irq = i + 32;
> +               if (vgic_bitmap_get_irq_val(&dist->irq_active, 0, irq))
>                         continue; /* level interrupt, already queued */
>
> -               if (!vgic_queue_irq(vcpu, 0, i)) {
> +               if (!vgic_queue_irq(vcpu, 0, irq)) {
>                         overflow = 1;
>                         continue;
>                 }
>
>                 /* Immediate clear on edge, set active on level */
> -               if (vgic_irq_is_edge(dist, i)) {
> -                       clear_bit(i - 32, pending);
> -                       clear_bit(i, vgic_cpu->pending);
> +               if (vgic_irq_is_edge(dist, irq)) {
> +                       clear_bit(i, pending);
> +                       kvm_vgic_vcpu_clear_pending_irq(vcpu, irq);
>                 } else {
> -                       vgic_bitmap_set_irq_val(&dist->irq_active, 0, i, 1);
> +                       vgic_bitmap_set_irq_val(&dist->irq_active, 0, irq, 1);
>                 }
>         }
>
> @@ -963,7 +982,7 @@ static bool vgic_update_irq_state(struct kvm *kvm, int cpuid,
>         vcpu = kvm_get_vcpu(kvm, cpuid);
>
>         if (level) {
> -               set_bit(irq_num, vcpu->arch.vgic_cpu.pending);
> +               kvm_vgic_vcpu_set_pending_irq(vcpu, irq_num);
>                 set_bit(cpuid, &dist->irq_pending_on_cpu);
>         }
>
> @@ -1031,11 +1050,11 @@ static irqreturn_t vgic_maintenance_handler(int irq, void *data)
>                         /* Any additionnal pending interrupt? */
>                         if (vgic_bitmap_get_irq_val(&dist->irq_state,
>                                                     vcpu->vcpu_id, irq)) {
> -                               set_bit(irq, vcpu->arch.vgic_cpu.pending);
> +                               kvm_vgic_vcpu_set_pending_irq(vcpu, irq);
>                                 set_bit(vcpu->vcpu_id,
>                                         &dist->irq_pending_on_cpu);
>                         } else {
> -                               clear_bit(irq, vgic_cpu->pending);
> +                               kvm_vgic_vcpu_clear_pending_irq(vcpu, irq);
>                         }
>                 }
>         }
> --
> 1.7.12
>
>

man, that was surprisingly hard to review (supports the case for your
following pending patch).

thanks, applied.
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm


[Index of Archives]     [Linux KVM]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux