Re: [PATCH v2 10/15] KVM: arm64: add data structures to model ITS interrupt translation
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_SGIS16 >> @@ -162,6 +163,8 @@ struct vgic_its { >> u64 cbaser; >> int creadr; >> int cwriter; >> +struct list_headdevice_list; >> +struct list_headcollection_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) & 0xf000ULL) >> >> /* 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); >>
Re: [PATCH v2 10/15] KVM: arm64: add data structures to model ITS interrupt translation
Hi Andre, On 08/25/2015 01:15 PM, Andre Przywara wrote: > 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_LEGACY256 >>> #define VGIC_NR_SGIS 16 >>> @@ -162,6 +163,8 @@ struct vgic_its { >>> u64 cbaser; >>> int creadr; >>> int cwriter; >>> + struct list_headdevice_list; >>> + struct list_headcollection_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) & 0xf000ULL) >>> >>> /* 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. OK. My next question is why don't we call vits_init in vgic_init after dist->nr_cpus? I had in mind map_resources was linked to the setting of vgic_dist_base & vgic_cpu_base. Here you do not depend on those addresses? Do I miss smthg? > >>> + 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? Sorry my point was about the _ENTRY_ variant that should avoid to use container_of, isn't it? Eric > > 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_
Re: [PATCH v2 10/15] KVM: arm64: add data structures to model ITS interrupt translation
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_headdevice_list; > + struct list_headcollection_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) & 0xf000ULL) > > /* 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... > + 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? > + 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); > } > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html