From mboxrd@z Thu Jan 1 00:00:00 1970 From: andre.przywara@arm.com (Andre Przywara) Date: Tue, 25 Aug 2015 12:15:11 +0100 Subject: [PATCH v2 10/15] KVM: arm64: add data structures to model ITS interrupt translation In-Reply-To: <55CCBBBF.1080407@linaro.org> References: <1436538111-4294-1-git-send-email-andre.przywara@arm.com> <1436538111-4294-11-git-send-email-andre.przywara@arm.com> <55CCBBBF.1080407@linaro.org> Message-ID: <55DC4E3F.506@arm.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi Eric, On 13/08/15 16:46, Eric Auger wrote: > > On 07/10/2015 04:21 PM, Andre Przywara wrote: >> The GICv3 Interrupt Translation Service (ITS) uses tables in memory >> to allow a sophisticated interrupt routing. It features device tables, >> an interrupt table per device and a table connecting "collections" to >> actual CPUs (aka. redistributors in the GICv3 lingo). >> Since the interrupt numbers for the LPIs are allocated quite sparsely >> and the range can be quite huge (8192 LPIs being the minimum), using >> bitmaps or arrays for storing information is a waste of memory. >> We use linked lists instead, which we iterate linearily. This works >> very well with the actual number of LPIs/MSIs in the guest being >> quite low. Should the number of LPIs exceed the number where iterating >> through lists seems acceptable, we can later revisit this and use more >> efficient data structures. >> >> Signed-off-by: Andre Przywara >> --- >> include/kvm/arm_vgic.h | 3 +++ >> virt/kvm/arm/its-emul.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 51 insertions(+) >> >> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h >> index b432055..1648668 100644 >> --- a/include/kvm/arm_vgic.h >> +++ b/include/kvm/arm_vgic.h >> @@ -25,6 +25,7 @@ >> #include >> #include >> #include >> +#include >> >> #define VGIC_NR_IRQS_LEGACY 256 >> #define VGIC_NR_SGIS 16 >> @@ -162,6 +163,8 @@ struct vgic_its { >> u64 cbaser; >> int creadr; >> int cwriter; >> + struct list_head device_list; >> + struct list_head collection_list; >> }; >> >> struct vgic_dist { >> diff --git a/virt/kvm/arm/its-emul.c b/virt/kvm/arm/its-emul.c >> index b498f06..7f217fa 100644 >> --- a/virt/kvm/arm/its-emul.c >> +++ b/virt/kvm/arm/its-emul.c >> @@ -21,6 +21,7 @@ >> #include >> #include >> #include >> +#include >> >> #include >> #include >> @@ -32,6 +33,25 @@ >> #include "vgic.h" >> #include "its-emul.h" >> >> +struct its_device { >> + struct list_head dev_list; >> + struct list_head itt; >> + u32 device_id; >> +}; >> + >> +struct its_collection { >> + struct list_head coll_list; >> + u32 collection_id; >> + u32 target_addr; >> +}; >> + >> +struct its_itte { >> + struct list_head itte_list; >> + struct its_collection *collection; >> + u32 lpi; >> + u32 event_id; >> +}; >> + >> #define BASER_BASE_ADDRESS(x) ((x) & 0xfffffffff000ULL) >> >> /* The distributor lock is held by the VGIC MMIO handler. */ >> @@ -311,6 +331,9 @@ int vits_init(struct kvm *kvm) >> >> spin_lock_init(&its->lock); >> >> + INIT_LIST_HEAD(&its->device_list); >> + INIT_LIST_HEAD(&its->collection_list); >> + >> its->enabled = false; >> >> return -ENXIO; >> @@ -320,11 +343,36 @@ void vits_destroy(struct kvm *kvm) >> { >> struct vgic_dist *dist = &kvm->arch.vgic; >> struct vgic_its *its = &dist->its; >> + struct its_device *dev; >> + struct its_itte *itte; >> + struct list_head *dev_cur, *dev_temp; >> + struct list_head *cur, *temp; >> >> if (!vgic_has_its(kvm)) >> return; >> >> + if (!its->device_list.next) > Why not using list_empty? But I think I would simply remove this since > the empty case if handle below... list_empty() requires the list to be initialized before. This check here is to detect that map_resources was never called (this is only done on the first VCPU run) and thus device_list is basically still all zeroes. If we abort the guest without ever running a VCPU (for instance because some initialization failed), we call vits_destroy() anyway (because this is called when tearing down the VGIC device). So the check is here to detect early that vits_destroy() has been called without the ITS ever been fully initialized. This fixed a real bug when the guest start was aborted before the ITS was ever used. I will add a comment to make this clear. >> + return; >> + >> + spin_lock(&its->lock); >> + list_for_each_safe(dev_cur, dev_temp, &its->device_list) { >> + dev = container_of(dev_cur, struct its_device, dev_list); > isn't the usage of list_for_each_entry_safe more synthetic here? If I got this correctly, we need the _safe variant if we want to remove the list item within the loop. Or am I missing something here? Cheers, Andre. >> + list_for_each_safe(cur, temp, &dev->itt) { >> + itte = (container_of(cur, struct its_itte, itte_list)); > same > > Eric >> + list_del(cur); >> + kfree(itte); >> + } >> + list_del(dev_cur); >> + kfree(dev); >> + } >> + >> + list_for_each_safe(cur, temp, &its->collection_list) { >> + list_del(cur); >> + kfree(container_of(cur, struct its_collection, coll_list)); >> + } >> + >> kfree(dist->pendbaser); >> >> its->enabled = false; >> + spin_unlock(&its->lock); >> } >> >