On Mon, Aug 20, 2012 at 8:48 AM, Marc Zyngier <marc.zyngier@xxxxxxx> wrote:
> On 18/08/12 20:25, Christoffer Dall wrote:
>> On Thu, Jul 5, 2012 at 11:28 AM, Marc Zyngier <marc.zyngier@xxxxxxx> wrote:
>>> 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>
>>> ---
>>> arch/arm/include/asm/kvm_vgic.h | 42 ++++++++
>>> arch/arm/kvm/vgic.c | 227 ++++++++++++++++++++++++++++++++++++++-
>>> 2 files changed, 268 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/arm/include/asm/kvm_vgic.h b/arch/arm/include/asm/kvm_vgic.h
>>> index 02f50b7..481f4a9 100644
>>> --- a/arch/arm/include/asm/kvm_vgic.h
>>> +++ b/arch/arm/include/asm/kvm_vgic.h
>>> @@ -173,16 +173,58 @@ 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, VGIC_NR_IRQS);
>>> +
>>> + /* Bitmap of used/free list registers */
>>> + DECLARE_BITMAP( lr_used, 64);
>>
>> my mind spins again, is the lr_used a duplication of the state stored
>> in vgic_elrsr?
>
> Related, but not the same. lr_used drives the LR allocation, and elrsr
> gives you the state of the CPU interface on exit. Of course, elrsr and
> lr_used have similar meaning before the vcpu has run.
>
I see, thanks for clearing that up.
>>> +
>>> + /* 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... */
>>> +#endif
>>> };
>>>
>>> +#define VGIC_HCR_EN (1 << 0)
>>> +#define VGIC_HCR_UIE (1 << 1)
>>> +
>>> +#define VGIC_LR_VIRTUALID (0x3ff << 0)
>>> +#define VGIC_LR_PHYSID_CPUID (7 << 10)
>>> +#define VGIC_LR_STATE (3 << 28)
>>> +#define VGIC_LR_PENDING_BIT (1 << 28)
>>> +#define VGIC_LR_ACTIVE_BIT (1 << 29)
>>> +#define VGIC_LR_EOI (1 << 19)
>>> +
>>> +#define VGIC_MISR_EOI (1 << 0)
>>> +#define VGIC_MISR_U (1 << 1)
>>> +
>>> +#define LR_EMPTY 0xff
>>> +
>>> struct kvm;
>>> struct kvm_vcpu;
>>> struct kvm_run;
>>> struct kvm_exit_mmio;
>>>
>>> #ifdef CONFIG_KVM_ARM_VGIC
>>> +void kvm_vgic_sync_to_cpu(struct kvm_vcpu *vcpu);
>>> +void kvm_vgic_sync_from_cpu(struct kvm_vcpu *vcpu);
>>> +int kvm_vgic_vcpu_pending_irq(struct kvm_vcpu *vcpu);
>>> int 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 ad48c89..8722efc 100644
>>> --- a/arch/arm/kvm/vgic.c
>>> +++ b/arch/arm/kvm/vgic.c
>>> @@ -545,7 +545,25 @@ 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;
>>> + int vcpu_id;
>>> +
>>> + vcpu_id = vcpu->vcpu_id;
>>> + pend = vcpu->arch.vgic_cpu.pending;
>>> +
>>> + 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);
>>> +
>>> + 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,
>>> + 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);
>>> }
>>>
>>> /*
>>> @@ -570,3 +588,210 @@ static void vgic_update_state(struct kvm *kvm)
>>> }
>>> }
>>> }
>>> +
>>> +/*
>>> + * Queue an interrupt to a CPU virtual interface. Return 0 on success,
>>> + * or 1 if it wasn't possible to queue it.
>>> + */
>>
>> consider a bool?
>
> OK.
>
>>> +static int vgic_queue_irq(struct kvm_vcpu *vcpu, u8 sgi_source_id, int irq)
>>> +{
>>> + struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
>>> + struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
>>> + int lr, is_level;
>>> +
>>> + /* Sanitize the input... */
>>> + BUG_ON(sgi_source_id & ~7);
>>> + BUG_ON(sgi_source_id && irq > 15);
>>> + BUG_ON(irq >= VGIC_NR_IRQS);
>>> +
>>> + kvm_debug("Queue IRQ%d\n", irq);
>>> +
>>> + lr = vgic_cpu->vgic_irq_lr_map[irq];
>>> + is_level = !vgic_irq_is_edge(dist, irq);
>>> +
>>> + /* Do we have an active interrupt for the same CPUID? */
>>> + if (lr != LR_EMPTY &&
>>> + (vgic_cpu->vgic_lr[lr] & VGIC_LR_PHYSID_CPUID) == (sgi_source_id << 10)) {
>>
>> this line is too long, could we have a macro
>>
>> #define LR_PHYSID(_x) ((_x & VGIC_LR_PHYSID) >> 10)
>>
>> and make the line
>>
>> (LR_PHYSID(vgic_cpu->vgic_lr[lr]) == sgi_source_id) {
>>
>
> OK.
>
>>
>>> + kvm_debug("LR%d piggyback for IRQ%d %x\n", lr, irq, vgic_cpu->vgic_lr[lr]);
>>> + BUG_ON(!test_bit(lr, vgic_cpu->lr_used));
>>> + vgic_cpu->vgic_lr[lr] |= VGIC_LR_PENDING_BIT;
>>> + if (is_level)
>>> + vgic_cpu->vgic_lr[lr] |= VGIC_LR_EOI;
>>> + return 0;
>>> + }
>>> +
>>> + /* Try to use another LR for this interrupt */
>>> + lr = find_first_bit((unsigned long *)vgic_cpu->vgic_elrsr,
>>> + vgic_cpu->nr_lr);
>>
>> so here, could we just as well find the first zero bit in vgic_cpu->lr_used?
>
> Indeed.
>
>>> + if (lr >= vgic_cpu->nr_lr)
>>> + return 1;
>>> +
>>> + kvm_debug("LR%d allocated for IRQ%d %x\n", lr, irq, sgi_source_id);
>>> + vgic_cpu->vgic_lr[lr] = (VGIC_LR_PENDING_BIT | (sgi_source_id << 10) | irq);
>>
>> nit, consider a macro to create an lr, and hide the 10 magic number, like:
>>
>> #define MK_LR_PEND(_src_id, _irq, _level) (VGIC_LR_PENDING_BIT |
>> (_src_id << 10) | _irq)
>
> OK.
>
>> maybe it's ridiculous, your choice.
>>
>>> + if (is_level)
>>> + vgic_cpu->vgic_lr[lr] |= VGIC_LR_EOI;
>>> +
>>> + vgic_cpu->vgic_irq_lr_map[irq] = lr;
>>> + clear_bit(lr, (unsigned long *)vgic_cpu->vgic_elrsr);
>>> + set_bit(lr, vgic_cpu->lr_used);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +/*
>>> + * Fill the list registers with pending interrupts before running the
>>> + * guest.
>>> + */
>>> +static void __kvm_vgic_sync_to_cpu(struct kvm_vcpu *vcpu)
>>> +{
>>> + struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
>>> + struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
>>> + unsigned long *pending;
>>> + int i, c, vcpu_id;
>>> + int overflow = 0;
>>> +
>>> + vcpu_id = vcpu->vcpu_id;
>>> +
>>> + /*
>>> + * We may not have any pending interrupt, or the interrupts
>>> + * may have been serviced from another vcpu. In all cases,
>>> + * move along.
>>> + */
>>> + if (!kvm_vgic_vcpu_pending_irq(vcpu)) {
>>> + pr_debug("CPU%d has no pending interrupt\n", vcpu_id);
>>
>> nit: kvm_debug or should go away
>>
>>> + goto epilog;
>>> + }
>>> +
>>> + /* SGIs */
>>> + pending = vgic_bitmap_get_cpu_map(&dist->irq_state, vcpu_id);
>>> + for_each_set_bit(i, vgic_cpu->pending, 16) {
>>> + unsigned long sources;
>>> +
>>> + pr_debug("SGI%d on CPU%d\n", i, vcpu_id);
>>
>> nit: kvm_debug or should go away
>>
>>> + sources = dist->irq_sgi_sources[vcpu_id][i];
>>> + for_each_set_bit(c, &sources, 8) {
>>> + if (vgic_queue_irq(vcpu, c, i)) {
>>
>> nit: see to me the vgic_queue_irq return value should be flipped, so
>> it's if (!vgic_queue_irq
>>
>>> + overflow = 1;
>>
>> not necessarily an overflow, in the sense that a HCR_UIE may not be
>> relevant, but certainly we cannot clear the irq_pending_on_cpu bit.
>>
>>> + continue;
>>
>> isn't there an interesting case where we keep hammering even though
>> there are no more free lr registers? makes sense to split the case
>> with out of lr's versus already occupied lr for that irq?
>
> It could be an interesting optimization if you end up with multiple
> simultaneously pending interrupts.
>
> What you could do is once you've allocated all the LRs, only consider
> the interrupts that are part of the allocated set. Only these can progress.
>
> I'll think of it, but I don't think this is high priority.
>
no rush on my account.
>>> + }
>>> +
>>> + sources &= ~(1 << c);
>>
>> clear_bit() ?
>
> OK.
>
>>> + }
>>> +
>>> + if (!sources)
>>> + clear_bit(i, pending);
>>> +
>>> + dist->irq_sgi_sources[vcpu_id][i] = sources;
>>> + }
>>> +
>>> + /* PPIs */
>>> + for_each_set_bit_from(i, vgic_cpu->pending, 32) {
>>> + if (vgic_queue_irq(vcpu, 0, i)) {
>>> + overflow = 1;
>>> + continue;
>>> + }
>>> +
>>> + clear_bit(i, pending);
>>> + }
>>> +
>>> +
>>> + /* 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))
>>> + continue; /* level interrupt, already queued */
>>> +
>>> + if (vgic_queue_irq(vcpu, 0, i)) {
>>> + overflow = 1;
>>> + continue;
>>> + }
>>> +
>>> + /* Immediate clear on edge, set active on level */
>>> + if (vgic_irq_is_edge(dist, i))
>>> + clear_bit(i - 32, pending);
>>> + else
>>> + vgic_bitmap_set_irq_val(&dist->irq_active, 0, i, 1);
>>> + }
>>> +
>>> +epilog:
>>> + if (overflow)
>>> + vgic_cpu->vgic_hcr |= VGIC_HCR_UIE;
>>> + else {
>>> + vgic_cpu->vgic_hcr &= ~VGIC_HCR_UIE;
>>> + /*
>>> + * We're about to run this VCPU, and we've consumed
>>> + * everything the distributor had in store for
>>> + * us. Claim we don't have anything pending. We'll
>>> + * adjust that if needed while exiting.
>>> + */
>>> + clear_bit(1 << vcpu_id, &dist->irq_pending_on_cpu);
>>> + }
>>> +}
>>> +
>>> +/*
>>> + * Sync back the VGIC state after a guest run.
>>> + */
>>> +static void __kvm_vgic_sync_from_cpu(struct kvm_vcpu *vcpu)
>>> +{
>>> + struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
>>> + struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
>>> + int lr, pending;
>>> +
>>> + /* Clear mappings for empty LRs */
>>> + for_each_set_bit(lr, (unsigned long *)vgic_cpu->vgic_elrsr,
>>> + vgic_cpu->nr_lr) {
>>> + int irq;
>>> +
>>> + if (!test_and_clear_bit(lr, vgic_cpu->lr_used))
>>> + continue;
>>> +
>>> + irq = vgic_cpu->vgic_lr[lr] & VGIC_LR_VIRTUALID;
>>> +
>>> + BUG_ON(irq >= VGIC_NR_IRQS);
>>> + vgic_cpu->vgic_irq_lr_map[irq] = LR_EMPTY;
>>> + }
>>> +
>>> + /* Check if we still have something up our sleeve... */
>>> + pending = find_first_zero_bit((unsigned long *)vgic_cpu->vgic_elrsr,
>>> + vgic_cpu->nr_lr);
>>> + if (pending < vgic_cpu->nr_lr)
>>> + set_bit(1 << vcpu->vcpu_id, &dist->irq_pending_on_cpu);
>>> +}
>>> +
>>> +void kvm_vgic_sync_to_cpu(struct kvm_vcpu *vcpu)
>>> +{
>>> + struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
>>> +
>>> + if (!irqchip_in_kernel(vcpu->kvm))
>>> + return;
>>> +
>>> + spin_lock(&dist->lock);
>>> + __kvm_vgic_sync_to_cpu(vcpu);
>>> + spin_unlock(&dist->lock);
>>> +
>>> + *__this_cpu_ptr(vgic_vcpus) = vcpu;
>>> +}
>>> +
>>> +void kvm_vgic_sync_from_cpu(struct kvm_vcpu *vcpu)
>>> +{
>>> + struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
>>> +
>>> + if (!irqchip_in_kernel(vcpu->kvm))
>>> + return;
>>> +
>>> + spin_lock(&dist->lock);
>>> + __kvm_vgic_sync_from_cpu(vcpu);
>>> + spin_unlock(&dist->lock);
>>> +
>>> + *__this_cpu_ptr(vgic_vcpus) = NULL;
>>> +}
>>> +
>>> +int kvm_vgic_vcpu_pending_irq(struct kvm_vcpu *vcpu)
>>> +{
>>> + struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
>>> +
>>> + if (!irqchip_in_kernel(vcpu->kvm))
>>> + return 0;
>>> +
>>> + return test_bit(1 << vcpu->vcpu_id, &dist->irq_pending_on_cpu);
>>> +}
>>> --
>>> 1.7.10.3
>>>
>>>
>>
>
>
> --
> Jazz is not dead. It just smells funny...
>
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm
