Re: [PATCH 11/13] KVM: arm64: implement ITS command queue command handlers
On Fri, Jul 03, 2015 at 04:57:04PM +0100, Andre Przywara wrote:
> Hi Christoffer,
>
>
>
> >> +
> >> +static struct its_collection *vits_new_collection(struct kvm *kvm, u32
> >> coll_id)
> >> +{
> >> + struct its_collection *collection;
> >> +
> >> + collection = kmalloc(sizeof(struct its_collection), GFP_KERNEL);
> >
> > If I manage to understand the structure here, you're calling all these
> > handler functions with a spinlock held so any operation that may sleep
> > could cause your system to deadlock.
> >
> > I'll stop looking for these and recommend you go over the whole series
> > for these.
>
> Do you reckon it would be sufficient to just avoid the kmallocs inside
> the lock? For this case above we could go with some storage space
> preallocated outside of the lock (I hope).
>
Yes, you can preallocate or you need to grab a mutex instead of a
spinlock...
> >
> > Perhaps the right thing to do is to synchronize access to your data
> > structure (why you hold the spinlock right?) with RCU if you can...
>
> Well, the point is that there is not one ITS data structure, but it's a
> mesh of lists connected to each other. I'd like to avoid going down with
> RCU there, so I'd like to keep it all under one lock.
> I wonder if this could be mutex_lock_interruptible, though. From the top
> of your head, is there anything that would prevent that? I reckon ITS
> access contention is rather rare (though possible), so a sleeping VCPU
> wouldn't harm us so much in practice, would it?
>
We know from experience from x86 that one of the things they had to look
at was to get the run-loop lock-free, and we went through a lot of
effort to do that on ARM too. Along came the vgic and that was out the
window, and now it feels like we're just grabbing spinlocks all over the
place.
I'm fine with a mutex if other solutions are not easy/possible and it's
in a truly non-critical path, but I wouldn't to speculate about the
level of contention at this point without profiling something.
In any case, let's fix the potential host-kernel deadlock issues in the
most elegant way first and let's try to think about not grabbing too
many spinlocks in this code and take it from there.
Thanks,
-Christoffer
--
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
Re: [PATCH 11/13] KVM: arm64: implement ITS command queue command handlers
Hi Christoffer,
>> +
>> +static struct its_collection *vits_new_collection(struct kvm *kvm, u32
>> coll_id)
>> +{
>> +struct its_collection *collection;
>> +
>> +collection = kmalloc(sizeof(struct its_collection), GFP_KERNEL);
>
> If I manage to understand the structure here, you're calling all these
> handler functions with a spinlock held so any operation that may sleep
> could cause your system to deadlock.
>
> I'll stop looking for these and recommend you go over the whole series
> for these.
Do you reckon it would be sufficient to just avoid the kmallocs inside
the lock? For this case above we could go with some storage space
preallocated outside of the lock (I hope).
>
> Perhaps the right thing to do is to synchronize access to your data
> structure (why you hold the spinlock right?) with RCU if you can...
Well, the point is that there is not one ITS data structure, but it's a
mesh of lists connected to each other. I'd like to avoid going down with
RCU there, so I'd like to keep it all under one lock.
I wonder if this could be mutex_lock_interruptible, though. From the top
of your head, is there anything that would prevent that? I reckon ITS
access contention is rather rare (though possible), so a sleeping VCPU
wouldn't harm us so much in practice, would it?
Cheers,
Andre.
--
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
Re: [PATCH 11/13] KVM: arm64: implement ITS command queue command handlers
On Fri, May 29, 2015 at 10:53:27AM +0100, Andre Przywara wrote:
> The connection between a device, an event ID, the LPI number and the
> allocated CPU is stored in in-memory tables in a GICv3, but their
> format is not specified by the spec. Instead software uses a command
> queue to let the ITS implementation use their own format.
> Implement handlers for the various ITS commands and let them store
> the requested relation into our own data structures.
> Error handling is very basic at this point, as we don't have a good
> way of communicating errors to the guest (usually a SError).
>
> Signed-off-by: Andre Przywara
> ---
> include/linux/irqchip/arm-gic-v3.h | 1 +
> virt/kvm/arm/its-emul.c| 422
> -
> virt/kvm/arm/its-emul.h| 11 +
> 3 files changed, 433 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/irqchip/arm-gic-v3.h
> b/include/linux/irqchip/arm-gic-v3.h
> index 0b450c7..651aacc 100644
> --- a/include/linux/irqchip/arm-gic-v3.h
> +++ b/include/linux/irqchip/arm-gic-v3.h
> @@ -254,6 +254,7 @@
> #define GITS_CMD_MAPD0x08
> #define GITS_CMD_MAPC0x09
> #define GITS_CMD_MAPVI 0x0a
> +#define GITS_CMD_MAPI0x0b
> #define GITS_CMD_MOVI0x01
> #define GITS_CMD_DISCARD 0x0f
> #define GITS_CMD_INV 0x0c
> diff --git a/virt/kvm/arm/its-emul.c b/virt/kvm/arm/its-emul.c
> index afd440e..574cf05 100644
> --- a/virt/kvm/arm/its-emul.c
> +++ b/virt/kvm/arm/its-emul.c
> @@ -22,6 +22,7 @@
> #include
> #include
> #include
> +#include
>
> #include
> #include
> @@ -55,6 +56,34 @@ struct its_itte {
> unsigned long *pending;
> };
>
> +static struct its_device *find_its_device(struct kvm *kvm, u32 device_id)
> +{
> + struct vgic_its *its = &kvm->arch.vgic.its;
> + struct its_device *device;
> +
> + list_for_each_entry(device, &its->device_list, dev_list)
> + if (device_id == device->device_id)
> + return device;
> +
> + return NULL;
> +}
> +
> +static struct its_itte *find_itte(struct kvm *kvm, u32 device_id, u32
> event_id)
> +{
> + struct its_device *device;
> + struct its_itte *itte;
> +
> + device = find_its_device(kvm, device_id);
> + if (device == NULL)
> + return NULL;
> +
> + list_for_each_entry(itte, &device->itt, itte_list)
> + if (itte->event_id == event_id)
> + return itte;
> +
> + return NULL;
> +}
> +
> #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)
> @@ -71,6 +100,19 @@ static struct its_itte *find_itte_by_lpi(struct kvm *kvm,
> int lpi)
> return NULL;
> }
>
> +static struct its_collection *find_collection(struct kvm *kvm, int coll_id)
> +{
> + struct its_collection *collection;
> +
> + list_for_each_entry(collection, &kvm->arch.vgic.its.collection_list,
> + coll_list) {
> + if (coll_id == collection->collection_id)
> + return collection;
> + }
> +
> + return NULL;
> +}
> +
> #define LPI_PROP_ENABLE_BIT(p) ((p) & LPI_PROP_ENABLED)
> #define LPI_PROP_PRIORITY(p) ((p) & 0xfc)
>
> @@ -345,9 +387,386 @@ void vits_unqueue_lpi(struct kvm_vcpu *vcpu, int lpi)
> spin_unlock(&its->lock);
> }
>
> +static u64 its_cmd_mask_field(u64 *its_cmd, int word, int shift, int size)
> +{
> + return (le64_to_cpu(its_cmd[word]) >> shift) & (BIT_ULL(size) - 1);
> +}
> +
> +#define its_cmd_get_command(cmd) its_cmd_mask_field(cmd, 0, 0, 8)
> +#define its_cmd_get_deviceid(cmd)its_cmd_mask_field(cmd, 0, 32, 32)
> +#define its_cmd_get_id(cmd) its_cmd_mask_field(cmd, 1, 0, 32)
> +#define its_cmd_get_physical_id(cmd) its_cmd_mask_field(cmd, 1, 32, 32)
> +#define its_cmd_get_collection(cmd) its_cmd_mask_field(cmd, 2, 0, 16)
> +#define its_cmd_get_target_addr(cmd) its_cmd_mask_field(cmd, 2, 16, 32)
> +#define its_cmd_get_validbit(cmd)its_cmd_mask_field(cmd, 2, 63, 1)
> +
> +/*
> + * Handles the DISCARD command, which frees an ITTE.
> + * Must be called with the ITS lock held.
> + */
> +static int vits_cmd_handle_discard(struct kvm *kvm, u64 *its_cmd)
> +{
> + u32 device_id;
> + u32 event_id;
> + struct its_itte *itte;
> +
> + device_id = its_cmd_get_deviceid(its_cmd);
> + event_id = its_cmd_get_id(its_cmd);
> +
> + itte = find_itte(kvm, device_id, event_id);
> + if (!itte || !itte->collection)
> + return E_ITS_DISCARD_UNMAPPED_INTERRUPT;
> +
> + clear_bit(itte->collection->target_addr, itte->pending);
> +
> + list_del(&itte->itte_list);
> + kfree(itte);
> + return 0;
> +}
> +
> +/*
> + * Handles the MOVI command, which moves an ITTE to a different collection
Re: [PATCH 11/13] KVM: arm64: implement ITS command queue command handlers
Hi Andre
On 05/29/2015 11:53 AM, Andre Przywara wrote:
> The connection between a device, an event ID, the LPI number and the
> allocated CPU is stored in in-memory tables in a GICv3, but their
> format is not specified by the spec. Instead software uses a command
> queue to let the ITS implementation use their own format.
, implement
> Implement handlers for the various ITS commands and let them store
> the requested relation into our own data structures.
> Error handling is very basic at this point, as we don't have a good
> way of communicating errors to the guest (usually a SError).
>
> Signed-off-by: Andre Przywara
> ---
> include/linux/irqchip/arm-gic-v3.h | 1 +
> virt/kvm/arm/its-emul.c| 422
> -
> virt/kvm/arm/its-emul.h| 11 +
> 3 files changed, 433 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/irqchip/arm-gic-v3.h
> b/include/linux/irqchip/arm-gic-v3.h
> index 0b450c7..651aacc 100644
> --- a/include/linux/irqchip/arm-gic-v3.h
> +++ b/include/linux/irqchip/arm-gic-v3.h
> @@ -254,6 +254,7 @@
> #define GITS_CMD_MAPD0x08
> #define GITS_CMD_MAPC0x09
> #define GITS_CMD_MAPVI 0x0a
> +#define GITS_CMD_MAPI0x0b
> #define GITS_CMD_MOVI0x01
> #define GITS_CMD_DISCARD 0x0f
> #define GITS_CMD_INV 0x0c
> diff --git a/virt/kvm/arm/its-emul.c b/virt/kvm/arm/its-emul.c
> index afd440e..574cf05 100644
> --- a/virt/kvm/arm/its-emul.c
> +++ b/virt/kvm/arm/its-emul.c
> @@ -22,6 +22,7 @@
> #include
> #include
> #include
> +#include
>
> #include
> #include
> @@ -55,6 +56,34 @@ struct its_itte {
> unsigned long *pending;
> };
>
> +static struct its_device *find_its_device(struct kvm *kvm, u32 device_id)
> +{
> + struct vgic_its *its = &kvm->arch.vgic.its;
> + struct its_device *device;
> +
> + list_for_each_entry(device, &its->device_list, dev_list)
> + if (device_id == device->device_id)
> + return device;
> +
> + return NULL;
> +}
> +
> +static struct its_itte *find_itte(struct kvm *kvm, u32 device_id, u32
> event_id)
> +{
> + struct its_device *device;
> + struct its_itte *itte;
> +
> + device = find_its_device(kvm, device_id);
> + if (device == NULL)
> + return NULL;
> +
> + list_for_each_entry(itte, &device->itt, itte_list)
> + if (itte->event_id == event_id)
> + return itte;
> +
> + return NULL;
> +}
> +
> #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)
> @@ -71,6 +100,19 @@ static struct its_itte *find_itte_by_lpi(struct kvm *kvm,
> int lpi)
> return NULL;
> }
>
> +static struct its_collection *find_collection(struct kvm *kvm, int coll_id)
> +{
> + struct its_collection *collection;
> +
> + list_for_each_entry(collection, &kvm->arch.vgic.its.collection_list,
> + coll_list) {
> + if (coll_id == collection->collection_id)
> + return collection;
> + }
> +
> + return NULL;
> +}
> +
> #define LPI_PROP_ENABLE_BIT(p) ((p) & LPI_PROP_ENABLED)
> #define LPI_PROP_PRIORITY(p) ((p) & 0xfc)
>
> @@ -345,9 +387,386 @@ void vits_unqueue_lpi(struct kvm_vcpu *vcpu, int lpi)
> spin_unlock(&its->lock);
> }
>
> +static u64 its_cmd_mask_field(u64 *its_cmd, int word, int shift, int size)
> +{
> + return (le64_to_cpu(its_cmd[word]) >> shift) & (BIT_ULL(size) - 1);
> +}
> +
> +#define its_cmd_get_command(cmd) its_cmd_mask_field(cmd, 0, 0, 8)
> +#define its_cmd_get_deviceid(cmd)its_cmd_mask_field(cmd, 0, 32, 32)
> +#define its_cmd_get_id(cmd) its_cmd_mask_field(cmd, 1, 0, 32)
> +#define its_cmd_get_physical_id(cmd) its_cmd_mask_field(cmd, 1, 32, 32)
> +#define its_cmd_get_collection(cmd) its_cmd_mask_field(cmd, 2, 0, 16)
> +#define its_cmd_get_target_addr(cmd) its_cmd_mask_field(cmd, 2, 16, 32)
> +#define its_cmd_get_validbit(cmd)its_cmd_mask_field(cmd, 2, 63, 1)
> +
> +/*
> + * Handles the DISCARD command, which frees an ITTE.
> + * Must be called with the ITS lock held.
> + */
> +static int vits_cmd_handle_discard(struct kvm *kvm, u64 *its_cmd)
> +{
> + u32 device_id;
> + u32 event_id;
> + struct its_itte *itte;
> +
> + device_id = its_cmd_get_deviceid(its_cmd);
> + event_id = its_cmd_get_id(its_cmd);
> +
> + itte = find_itte(kvm, device_id, event_id);
> + if (!itte || !itte->collection)
> + return E_ITS_DISCARD_UNMAPPED_INTERRUPT;
> +
> + clear_bit(itte->collection->target_addr, itte->pending);
> +
> + list_del(&itte->itte_list);
> + kfree(itte);
> + return 0;
> +}
> +
> +/*
> + * Handles the MOVI command, which moves an ITTE to a different collect
[PATCH 11/13] KVM: arm64: implement ITS command queue command handlers
The connection between a device, an event ID, the LPI number and the
allocated CPU is stored in in-memory tables in a GICv3, but their
format is not specified by the spec. Instead software uses a command
queue to let the ITS implementation use their own format.
Implement handlers for the various ITS commands and let them store
the requested relation into our own data structures.
Error handling is very basic at this point, as we don't have a good
way of communicating errors to the guest (usually a SError).
Signed-off-by: Andre Przywara
---
include/linux/irqchip/arm-gic-v3.h | 1 +
virt/kvm/arm/its-emul.c| 422 -
virt/kvm/arm/its-emul.h| 11 +
3 files changed, 433 insertions(+), 1 deletion(-)
diff --git a/include/linux/irqchip/arm-gic-v3.h
b/include/linux/irqchip/arm-gic-v3.h
index 0b450c7..651aacc 100644
--- a/include/linux/irqchip/arm-gic-v3.h
+++ b/include/linux/irqchip/arm-gic-v3.h
@@ -254,6 +254,7 @@
#define GITS_CMD_MAPD 0x08
#define GITS_CMD_MAPC 0x09
#define GITS_CMD_MAPVI 0x0a
+#define GITS_CMD_MAPI 0x0b
#define GITS_CMD_MOVI 0x01
#define GITS_CMD_DISCARD 0x0f
#define GITS_CMD_INV 0x0c
diff --git a/virt/kvm/arm/its-emul.c b/virt/kvm/arm/its-emul.c
index afd440e..574cf05 100644
--- a/virt/kvm/arm/its-emul.c
+++ b/virt/kvm/arm/its-emul.c
@@ -22,6 +22,7 @@
#include
#include
#include
+#include
#include
#include
@@ -55,6 +56,34 @@ struct its_itte {
unsigned long *pending;
};
+static struct its_device *find_its_device(struct kvm *kvm, u32 device_id)
+{
+ struct vgic_its *its = &kvm->arch.vgic.its;
+ struct its_device *device;
+
+ list_for_each_entry(device, &its->device_list, dev_list)
+ if (device_id == device->device_id)
+ return device;
+
+ return NULL;
+}
+
+static struct its_itte *find_itte(struct kvm *kvm, u32 device_id, u32 event_id)
+{
+ struct its_device *device;
+ struct its_itte *itte;
+
+ device = find_its_device(kvm, device_id);
+ if (device == NULL)
+ return NULL;
+
+ list_for_each_entry(itte, &device->itt, itte_list)
+ if (itte->event_id == event_id)
+ return itte;
+
+ return NULL;
+}
+
#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)
@@ -71,6 +100,19 @@ static struct its_itte *find_itte_by_lpi(struct kvm *kvm,
int lpi)
return NULL;
}
+static struct its_collection *find_collection(struct kvm *kvm, int coll_id)
+{
+ struct its_collection *collection;
+
+ list_for_each_entry(collection, &kvm->arch.vgic.its.collection_list,
+ coll_list) {
+ if (coll_id == collection->collection_id)
+ return collection;
+ }
+
+ return NULL;
+}
+
#define LPI_PROP_ENABLE_BIT(p) ((p) & LPI_PROP_ENABLED)
#define LPI_PROP_PRIORITY(p) ((p) & 0xfc)
@@ -345,9 +387,386 @@ void vits_unqueue_lpi(struct kvm_vcpu *vcpu, int lpi)
spin_unlock(&its->lock);
}
+static u64 its_cmd_mask_field(u64 *its_cmd, int word, int shift, int size)
+{
+ return (le64_to_cpu(its_cmd[word]) >> shift) & (BIT_ULL(size) - 1);
+}
+
+#define its_cmd_get_command(cmd) its_cmd_mask_field(cmd, 0, 0, 8)
+#define its_cmd_get_deviceid(cmd) its_cmd_mask_field(cmd, 0, 32, 32)
+#define its_cmd_get_id(cmd)its_cmd_mask_field(cmd, 1, 0, 32)
+#define its_cmd_get_physical_id(cmd) its_cmd_mask_field(cmd, 1, 32, 32)
+#define its_cmd_get_collection(cmd)its_cmd_mask_field(cmd, 2, 0, 16)
+#define its_cmd_get_target_addr(cmd) its_cmd_mask_field(cmd, 2, 16, 32)
+#define its_cmd_get_validbit(cmd) its_cmd_mask_field(cmd, 2, 63, 1)
+
+/*
+ * Handles the DISCARD command, which frees an ITTE.
+ * Must be called with the ITS lock held.
+ */
+static int vits_cmd_handle_discard(struct kvm *kvm, u64 *its_cmd)
+{
+ u32 device_id;
+ u32 event_id;
+ struct its_itte *itte;
+
+ device_id = its_cmd_get_deviceid(its_cmd);
+ event_id = its_cmd_get_id(its_cmd);
+
+ itte = find_itte(kvm, device_id, event_id);
+ if (!itte || !itte->collection)
+ return E_ITS_DISCARD_UNMAPPED_INTERRUPT;
+
+ clear_bit(itte->collection->target_addr, itte->pending);
+
+ list_del(&itte->itte_list);
+ kfree(itte);
+ return 0;
+}
+
+/*
+ * Handles the MOVI command, which moves an ITTE to a different collection.
+ * Must be called with the ITS lock held.
+ */
+static int vits_cmd_handle_movi(struct kvm *kvm, u64 *its_cmd)
+{
+ u32 device_id = its_cmd_get_deviceid(its_cmd);
+ u32 event_id = its_cmd_get_id(its_cmd);
+ u32 coll_id = its_cmd_get_collection(its_cmd);
+ str
