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).
> ret = create_hyp_io_mappings(vgic_vctrl_base,
> vgic_vctrl_base + resource_size(&vctrl_res),
> --
>
> Thanks,
> -Christoffer
>
--
Jazz is not dead. It just smells funny...
--
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
