Re: [PATCH v5 07/12] ARM: KVM: VGIC virtual CPU interface management

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

 



On Tue, Jan 15, 2013 at 6:00 AM, Marc Zyngier <marc.zyngier@xxxxxxx> wrote:
> On 14/01/13 22:02, Christoffer Dall wrote:
>> On Mon, Jan 14, 2013 at 10:42 AM, Will Deacon <will.deacon@xxxxxxx> wrote:
>>> On Tue, Jan 08, 2013 at 06:42:11PM +0000, Christoffer Dall wrote:
>>>> From: Marc Zyngier <marc.zyngier@xxxxxxx>
>>>>
>>>> Add VGIC virtual CPU interface code, picking pending interrupts
>>>> from the distributor and stashing them in the VGIC control interface
>>>> list registers.
>>>>
>>>> Signed-off-by: Marc Zyngier <marc.zyngier@xxxxxxx>
>>>> Signed-off-by: Christoffer Dall <c.dall@xxxxxxxxxxxxxxxxxxxxxx>
>>>> ---
>>>>  arch/arm/include/asm/kvm_vgic.h |   30 ++++
>>>>  arch/arm/kvm/vgic.c             |  327 +++++++++++++++++++++++++++++++++++++++
>>>>  2 files changed, 356 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/arch/arm/include/asm/kvm_vgic.h b/arch/arm/include/asm/kvm_vgic.h
>>>> index 9ff0d9c..b3133c4 100644
>>>> --- a/arch/arm/include/asm/kvm_vgic.h
>>>> +++ b/arch/arm/include/asm/kvm_vgic.h
>>>> @@ -110,8 +110,33 @@ struct vgic_dist {
>>>>  };
>>>>
>>>>  struct vgic_cpu {
>>>> +#ifdef CONFIG_KVM_ARM_VGIC
>>>> +       /* per IRQ to LR mapping */
>>>> +       u8              vgic_irq_lr_map[VGIC_NR_IRQS];
>>>> +
>>>> +       /* Pending interrupts on this VCPU */
>>>> +       DECLARE_BITMAP( pending_percpu, VGIC_NR_PRIVATE_IRQS);
>>>> +       DECLARE_BITMAP( pending_shared, VGIC_NR_SHARED_IRQS);
>>>> +
>>>> +       /* Bitmap of used/free list registers */
>>>> +       DECLARE_BITMAP( lr_used, 64);
>>>> +
>>>> +       /* Number of list registers on this CPU */
>>>> +       int             nr_lr;
>>>> +
>>>> +       /* CPU vif control registers for world switch */
>>>> +       u32             vgic_hcr;
>>>> +       u32             vgic_vmcr;
>>>> +       u32             vgic_misr;      /* Saved only */
>>>> +       u32             vgic_eisr[2];   /* Saved only */
>>>> +       u32             vgic_elrsr[2];  /* Saved only */
>>>> +       u32             vgic_apr;
>>>> +       u32             vgic_lr[64];    /* A15 has only 4... */
>>>
>>> Have a #define for the maximum number of list registers.
>>>
>>>> +#endif
>>>>  };
>>>>
>>>> +#define LR_EMPTY       0xff
>>>> +
>>>>  struct kvm;
>>>>  struct kvm_vcpu;
>>>>  struct kvm_run;
>>>> @@ -119,9 +144,14 @@ struct kvm_exit_mmio;
>>>>
>>>>  #ifdef CONFIG_KVM_ARM_VGIC
>>>>  int kvm_vgic_set_addr(struct kvm *kvm, unsigned long type, u64 addr);
>>>> +void kvm_vgic_sync_to_cpu(struct kvm_vcpu *vcpu);
>>>> +void kvm_vgic_sync_from_cpu(struct kvm_vcpu *vcpu);
>>>
>>> Same comment as for the arch timer (flush/sync).
>>>
>>>> +int kvm_vgic_vcpu_pending_irq(struct kvm_vcpu *vcpu);
>>>>  bool vgic_handle_mmio(struct kvm_vcpu *vcpu, struct kvm_run *run,
>>>>                       struct kvm_exit_mmio *mmio);
>>>>
>>>> +#define irqchip_in_kernel(k)   (!!((k)->arch.vgic.vctrl_base))
>>>> +
>>>>  #else
>>>>  static inline int kvm_vgic_hyp_init(void)
>>>>  {
>>>> diff --git a/arch/arm/kvm/vgic.c b/arch/arm/kvm/vgic.c
>>>> index bd2bd7f..58237d5 100644
>>>> --- a/arch/arm/kvm/vgic.c
>>>> +++ b/arch/arm/kvm/vgic.c
>>>> @@ -152,6 +152,34 @@ static int vgic_irq_is_enabled(struct kvm_vcpu *vcpu, int irq)
>>>>         return vgic_bitmap_get_irq_val(&dist->irq_enabled, vcpu->vcpu_id, irq);
>>>>  }
>>>>
>>>> +static int vgic_irq_is_active(struct kvm_vcpu *vcpu, int irq)
>>>> +{
>>>> +       struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
>>>> +
>>>> +       return vgic_bitmap_get_irq_val(&dist->irq_active, vcpu->vcpu_id, irq);
>>>> +}
>>>> +
>>>> +static void vgic_irq_set_active(struct kvm_vcpu *vcpu, int irq)
>>>> +{
>>>> +       struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
>>>> +
>>>> +       vgic_bitmap_set_irq_val(&dist->irq_active, vcpu->vcpu_id, irq, 1);
>>>> +}
>>>> +
>>>> +static void vgic_irq_clear_active(struct kvm_vcpu *vcpu, int irq)
>>>> +{
>>>> +       struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
>>>> +
>>>> +       vgic_bitmap_set_irq_val(&dist->irq_active, vcpu->vcpu_id, irq, 0);
>>>> +}
>>>> +
>>>> +static int vgic_dist_irq_is_pending(struct kvm_vcpu *vcpu, int irq)
>>>> +{
>>>> +       struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
>>>> +
>>>> +       return vgic_bitmap_get_irq_val(&dist->irq_state, vcpu->vcpu_id, irq);
>>>> +}
>>>> +
>>>>  static void vgic_dist_irq_set(struct kvm_vcpu *vcpu, int irq)
>>>>  {
>>>>         struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
>>>> @@ -711,7 +739,30 @@ static void vgic_dispatch_sgi(struct kvm_vcpu *vcpu, u32 reg)
>>>>
>>>>  static int compute_pending_for_cpu(struct kvm_vcpu *vcpu)
>>>>  {
>>>> -       return 0;
>>>> +       struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
>>>> +       unsigned long *pending, *enabled, *pend_percpu, *pend_shared;
>>>> +       unsigned long pending_private, pending_shared;
>>>> +       int vcpu_id;
>>>> +
>>>> +       vcpu_id = vcpu->vcpu_id;
>>>> +       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_percpu, pending, enabled, VGIC_NR_PRIVATE_IRQS);
>>>> +
>>>> +       pending = vgic_bitmap_get_shared_map(&dist->irq_state);
>>>> +       enabled = vgic_bitmap_get_shared_map(&dist->irq_enabled);
>>>> +       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);
>>>> +
>>>> +       pending_private = find_first_bit(pend_percpu, VGIC_NR_PRIVATE_IRQS);
>>>> +       pending_shared = find_first_bit(pend_shared, VGIC_NR_SHARED_IRQS);
>>>> +       return (pending_private < VGIC_NR_PRIVATE_IRQS ||
>>>> +               pending_shared < VGIC_NR_SHARED_IRQS);
>>>>  }
>>>>
>>>>  /*
>>>> @@ -737,6 +788,280 @@ static void vgic_update_state(struct kvm *kvm)
>>>>         }
>>>>  }
>>>>
>>>> +#define LR_CPUID(lr)   \
>>>> +       (((lr) & GICH_LR_PHYSID_CPUID) >> GICH_LR_PHYSID_CPUID_SHIFT)
>>>> +#define MK_LR_PEND(src, irq)   \
>>>> +       (GICH_LR_PENDING_BIT | ((src) << GICH_LR_PHYSID_CPUID_SHIFT) | (irq))
>>>> +/*
>>>> + * Queue an interrupt to a CPU virtual interface. Return true on success,
>>>> + * or false if it wasn't possible to queue it.
>>>> + */
>>>> +static bool vgic_queue_irq(struct kvm_vcpu *vcpu, u8 sgi_source_id, int irq)
>>>> +{
>>>> +       struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
>>>> +       int lr;
>>>> +
>>>> +       /* Sanitize the input... */
>>>> +       BUG_ON(sgi_source_id & ~7);
>>>> +       BUG_ON(sgi_source_id && irq > 15);
>>>
>>> You can use your new NR_SGIS definition here.
>>>
>>
>> This should address the remaining comments:
>>
>> commit 43957095ec5476beb198f4c4630dfc3e2f3951db
>> Author: Christoffer Dall <c.dall@xxxxxxxxxxxxxxxxxxxxxx>
>> Date:   Mon Jan 14 16:59:38 2013 -0500
>>
>>     KVM: ARM: vgic: Define VGIC_MAX_LRS
>>
>>     Define maximum number of link registers we can handle instead of using
>>     literals in the code.  If an architecture reports more link registers
>>     than we support, only use the number we can support.
>>
>>     Signed-off-by: Christoffer Dall <c.dall@xxxxxxxxxxxxxxxxxxxxxx>
>>
>> diff --git a/arch/arm/include/asm/kvm_vgic.h b/arch/arm/include/asm/kvm_vgic.h
>> index 1ace491..f9d1977 100644
>> --- a/arch/arm/include/asm/kvm_vgic.h
>> +++ b/arch/arm/include/asm/kvm_vgic.h
>> @@ -33,6 +33,7 @@
>>  #define VGIC_NR_PRIVATE_IRQS (VGIC_NR_SGIS + VGIC_NR_PPIS)
>>  #define VGIC_NR_SHARED_IRQS  (VGIC_NR_IRQS - VGIC_NR_PRIVATE_IRQS)
>>  #define VGIC_MAX_CPUS                KVM_MAX_VCPUS
>> +#define VGIC_MAX_LRS         64
>
> Consider this instead (for the reason below)
> #define VGIC_MAX_LRS    (1 << 7)
>
>>  /* Sanity checks... */
>>  #if (VGIC_MAX_CPUS > 8)
>> @@ -120,7 +121,7 @@ struct vgic_cpu {
>>       DECLARE_BITMAP( pending_shared, VGIC_NR_SHARED_IRQS);
>>
>>       /* Bitmap of used/free list registers */
>> -     DECLARE_BITMAP( lr_used, 64);
>> +     DECLARE_BITMAP( lr_used, VGIC_MAX_LRS);
>>
>>       /* Number of list registers on this CPU */
>>       int             nr_lr;
>> @@ -132,7 +133,7 @@ struct vgic_cpu {
>>       u32             vgic_eisr[2];   /* Saved only */
>>       u32             vgic_elrsr[2];  /* Saved only */
>>       u32             vgic_apr;
>> -     u32             vgic_lr[64];    /* A15 has only 4... */
>> +     u32             vgic_lr[VGIC_MAX_LRS];
>>  #endif
>>  };
>>
>> diff --git a/arch/arm/kvm/vgic.c b/arch/arm/kvm/vgic.c
>> index a0d283c..90a99fd 100644
>> --- a/arch/arm/kvm/vgic.c
>> +++ b/arch/arm/kvm/vgic.c
>> @@ -1345,6 +1345,8 @@ int kvm_vgic_hyp_init(void)
>>
>>       vgic_nr_lr = readl_relaxed(vgic_vctrl_base + GICH_VTR);
>>       vgic_nr_lr = (vgic_nr_lr & 0x1f) + 1;
>
> There is a bug here. It should be:
>         vgic_nr_lr = (vgic_nr_lr & 0x2f) + 1;
>
>> +     if (vgic_nr_lr > VGIC_MAX_LRS)
>> +             vgic_nr_lr = VGIC_MAX_LRS; /* TODO: Clear remaining LRs */
>
> Why? VGIC_MAX_LRS isn't a configurable value, but a maximum value
> defined by the specification. This is the maximum you can fit in a 6 bit
> field, plus one (1 << 7, exactly).
>

I figures this was something that could potentially change with a
newer gic architecture etc., and then it was good to have the check,
but you're right, then everything would break.

-Christoffer
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux