Hi Pavel, On 12/10/15 08:40, Pavel Fedin wrote: > Hello! > >> -----Original Message----- >> From: kvm-owner@xxxxxxxxxxxxxxx [mailto:kvm-owner@xxxxxxxxxxxxxxx] On Behalf Of Andre Przywara >> Sent: Wednesday, October 07, 2015 5:55 PM >> To: marc.zyngier@xxxxxxx; christoffer.dall@xxxxxxxxxx >> Cc: eric.auger@xxxxxxxxxx; p.fedin@xxxxxxxxxxx; kvmarm@xxxxxxxxxxxxxxxxxxxxx; linux-arm- >> kernel@xxxxxxxxxxxxxxxxxxx; kvm@xxxxxxxxxxxxxxx >> Subject: [PATCH v3 12/16] KVM: arm64: handle pending bit for LPIs in ITS emulation >> >> As the actual LPI number in a guest can be quite high, but is mostly >> assigned using a very sparse allocation scheme, bitmaps and arrays >> for storing the virtual interrupt status are a waste of memory. >> We use our equivalent of the "Interrupt Translation Table Entry" >> (ITTE) to hold this extra status information for a virtual LPI. >> As the normal VGIC code cannot use its fancy bitmaps to manage >> pending interrupts, we provide a hook in the VGIC code to let the >> ITS emulation handle the list register queueing itself. >> LPIs are located in a separate number range (>=8192), so >> distinguishing them is easy. With LPIs being only edge-triggered, we >> get away with a less complex IRQ handling. >> We extend the number of bits for storing the IRQ number in our >> LR struct to 16 to cover the LPI numbers we support as well. >> >> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx> >> --- >> Changelog v2..v3: >> - extend LR data structure to hold 16-bit wide IRQ IDs >> - only clear pending bit if IRQ could be queued >> - adapt __kvm_vgic_sync_hwstate() to upstream changes >> >> include/kvm/arm_vgic.h | 4 +- >> virt/kvm/arm/its-emul.c | 75 ++++++++++++++++++++++++++++++++++++ >> virt/kvm/arm/its-emul.h | 3 ++ >> virt/kvm/arm/vgic-v3-emul.c | 2 + >> virt/kvm/arm/vgic.c | 93 +++++++++++++++++++++++++++++++-------------- >> 5 files changed, 148 insertions(+), 29 deletions(-) >> >> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h >> index c3eb414..035911f 100644 >> --- a/include/kvm/arm_vgic.h >> +++ b/include/kvm/arm_vgic.h >> @@ -95,7 +95,7 @@ enum vgic_type { >> #define LR_HW (1 << 3) >> >> struct vgic_lr { >> - unsigned irq:10; >> + unsigned irq:16; >> union { >> unsigned hwirq:10; >> unsigned source:3; >> @@ -147,6 +147,8 @@ struct vgic_vm_ops { >> int (*init_model)(struct kvm *); >> void (*destroy_model)(struct kvm *); >> int (*map_resources)(struct kvm *, const struct vgic_params *); >> + bool (*queue_lpis)(struct kvm_vcpu *); >> + void (*unqueue_lpi)(struct kvm_vcpu *, int irq); >> }; >> >> struct vgic_io_device { >> diff --git a/virt/kvm/arm/its-emul.c b/virt/kvm/arm/its-emul.c >> index bab8033..8349970 100644 >> --- a/virt/kvm/arm/its-emul.c >> +++ b/virt/kvm/arm/its-emul.c >> @@ -59,8 +59,27 @@ struct its_itte { >> struct its_collection *collection; >> u32 lpi; >> u32 event_id; >> + bool enabled; >> + unsigned long *pending; >> }; >> >> +/* To be used as an iterator this macro misses the enclosing parentheses */ >> +#define for_each_lpi(dev, itte, kvm) \ >> + list_for_each_entry(dev, &(kvm)->arch.vgic.its.device_list, dev_list) \ >> + list_for_each_entry(itte, &(dev)->itt, itte_list) >> + >> +static struct its_itte *find_itte_by_lpi(struct kvm *kvm, int lpi) >> +{ >> + struct its_device *device; >> + struct its_itte *itte; >> + >> + for_each_lpi(device, itte, kvm) { >> + if (itte->lpi == lpi) >> + return itte; >> + } >> + return NULL; >> +} >> + >> #define BASER_BASE_ADDRESS(x) ((x) & 0xfffffffff000ULL) >> >> /* The distributor lock is held by the VGIC MMIO handler. */ >> @@ -154,9 +173,65 @@ static bool handle_mmio_gits_idregs(struct kvm_vcpu *vcpu, >> return false; >> } >> >> +/* >> + * Find all enabled and pending LPIs and queue them into the list >> + * registers. >> + * The dist lock is held by the caller. >> + */ >> +bool vits_queue_lpis(struct kvm_vcpu *vcpu) >> +{ >> + struct vgic_its *its = &vcpu->kvm->arch.vgic.its; >> + struct its_device *device; >> + struct its_itte *itte; >> + bool ret = true; >> + >> + if (!vgic_has_its(vcpu->kvm)) >> + return true; >> + if (!its->enabled || !vcpu->kvm->arch.vgic.lpis_enabled) >> + return true; >> + >> + spin_lock(&its->lock); >> + for_each_lpi(device, itte, vcpu->kvm) { >> + if (!itte->enabled || !test_bit(vcpu->vcpu_id, itte->pending)) >> + continue; >> + >> + if (!itte->collection) >> + continue; >> + >> + if (itte->collection->target_addr != vcpu->vcpu_id) >> + continue; >> + >> + >> + if (vgic_queue_irq(vcpu, 0, itte->lpi)) >> + __clear_bit(vcpu->vcpu_id, itte->pending); >> + else >> + ret = false; > > Shouldn't we also have 'break' here? If vgic_queue_irq() returns false, this means we have no more > LRs to use, therefore it makes no sense to keep iterating. I consider this too much optimization at this point. vgic_queue_irq() just tells about the success for this interrupt, I'd rather not make assumptions about other IRQs (we could piggy-back those, for instance). Even if not, I'd prefer to not break abstraction here. Cheers, Andre. -- 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
