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
