Skip to contentSkip to conversation

GSI route layer for MSI/MSI-X

PATCH Superseded Sheng Yang 4 participants 22 messages Jan 8, 2009 – Jan 12, 2009 KVM

v5 of 6 Superseded Latest is v6 →

lore
Share & tools
lore

Sheng Yang

author
#0 lore raw

v5 cover GSI route layer for MSI/MSI-X

Update from v4:
Addressed Marcelo's comments on the first patch, as well as separate bitmap optimize patchset, which would be posted after this one is checked in.

--
regards
Yang, Sheng

author
#1 lore raw

PATCH v5 1/7 KVM: Add a route layer to convert MSI message to GSI

Avi's purpose, to use single kvm_set_irq() to deal with all interrupt, including MSI. So here is it.

struct gsi_route_entry is a mapping from a special gsi(with KVM_GSI_MSG_MASK) to MSI/MSI-X message address/data. And the struct can also be extended for other purpose.

Now we support up to 256 gsi_route_entry mapping, and gsi is allocated by kernel and provide two ioctls to userspace, which is more flexiable.

Signed-off-by Sheng Yang <[email protected]>
---
 include/linux/kvm.h      |   26 +++++++++++
 include/linux/kvm_host.h |   20 +++++++++
 virt/kvm/irq_comm.c      |   70 ++++++++++++++++++++++++++++++
 virt/kvm/kvm_main.c      |  105 ++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 221 insertions(+), 0 deletions(-)
include/linux/kvm.h +26 -0
index 71c150f..bbefce6 100644
@@ -399,6 +399,9 @@ struct kvm_trace_rec {
 #if defined(CONFIG_X86)
 #define KVM_CAP_REINJECT_CONTROL 24
 #endif
+#if defined(CONFIG_X86)
+#define KVM_CAP_GSI_ROUTE 25
+#endif
 
 /*
  * ioctls for VM fds

@@ -433,6 +436,8 @@ struct kvm_trace_rec {
 #define KVM_ASSIGN_IRQ _IOR(KVMIO, 0x70, \
 			    struct kvm_assigned_irq)
 #define KVM_REINJECT_CONTROL      _IO(KVMIO, 0x71)
+#define KVM_REQUEST_GSI_ROUTE	  _IOWR(KVMIO, 0x72, void *)
+#define KVM_FREE_GSI_ROUTE	  _IOR(KVMIO, 0x73, void *)
 
 /*
  * ioctls for vcpu fds

@@ -553,4 +558,25 @@ struct kvm_assigned_irq {
 #define KVM_DEV_IRQ_ASSIGN_MSI_ACTION	KVM_DEV_IRQ_ASSIGN_ENABLE_MSI
 #define KVM_DEV_IRQ_ASSIGN_ENABLE_MSI	(1 << 0)
 
+struct kvm_gsi_route_guest {
+	__u32 entries_nr;
+	struct kvm_gsi_route_entry_guest *entries;
+};
+
+#define KVM_GSI_ROUTE_MSI	(1 << 0)
+struct kvm_gsi_route_entry_guest {
+	__u32 gsi;
+	__u32 type;
+	__u32 flags;
+	__u32 reserved;
+	union {
+		struct {
+			__u32 addr_lo;
+			__u32 addr_hi;
+			__u32 data;
+		} msi;
+		__u32 padding[8];
+	};
+};
+
 #endif
include/linux/kvm_host.h +20 -0
index a8bcad0..6a00201 100644
@@ -136,6 +136,9 @@ struct kvm {
 	unsigned long mmu_notifier_seq;
 	long mmu_notifier_count;
 #endif
+	struct hlist_head gsi_route_list;
+#define KVM_NR_GSI_ROUTE_ENTRIES    256
+	DECLARE_BITMAP(gsi_route_bitmap, KVM_NR_GSI_ROUTE_ENTRIES);
 };
 
 /* The guest did something we don't support. */

@@ -336,6 +339,19 @@ void kvm_unregister_irq_mask_notifier(struct kvm *kvm, int irq,
 				      struct kvm_irq_mask_notifier *kimn);
 void kvm_fire_mask_notifiers(struct kvm *kvm, int irq, bool mask);
 
+#define KVM_GSI_ROUTE_MASK    0x1000000ull
+struct kvm_gsi_route_entry {
+	u32 gsi;
+	u32 type;
+	u32 flags;
+	u32 reserved;
+	union {
+		struct msi_msg msi;
+		u32 reserved[8];
+	};
+	struct hlist_node link;
+};
+
 void kvm_set_irq(struct kvm *kvm, int irq_source_id, int irq, int level);
 void kvm_notify_acked_irq(struct kvm *kvm, unsigned gsi);
 void kvm_register_irq_ack_notifier(struct kvm *kvm,

@@ -343,6 +359,10 @@ void kvm_register_irq_ack_notifier(struct kvm *kvm,
 void kvm_unregister_irq_ack_notifier(struct kvm_irq_ack_notifier *kian);
 int kvm_request_irq_source_id(struct kvm *kvm);
 void kvm_free_irq_source_id(struct kvm *kvm, int irq_source_id);
+int kvm_update_gsi_route(struct kvm *kvm, struct kvm_gsi_route_entry *entry);
+struct kvm_gsi_route_entry *kvm_find_gsi_route_entry(struct kvm *kvm, u32 gsi);
+void kvm_free_gsi_route(struct kvm *kvm, struct kvm_gsi_route_entry *entry);
+void kvm_free_gsi_route_list(struct kvm *kvm);
 
 #ifdef CONFIG_DMAR
 int kvm_iommu_map_pages(struct kvm *kvm, gfn_t base_gfn,
virt/kvm/irq_comm.c +70 -0

73 lines not on this page · raw message

virt/kvm/kvm_main.c +105 -0

139 lines not on this page · raw message

Signature · 1 line
1.5.4.5
#2 lore raw

Re: [PATCH 1/7] KVM: Add a route layer to convert MSI message to GSI

include/linux/kvm.h struct kvm_trace_rec { · line 440

Quoted 7 lines · Sheng Yang

* ioctls for VM fds

include/linux/kvm.h +2 -0
@@ -433,6 +436,8 @@ struct kvm_trace_rec {
 #define KVM_ASSIGN_IRQ _IOR(KVMIO, 0x70, \
 			    struct kvm_assigned_irq)
 #define KVM_REINJECT_CONTROL      _IO(KVMIO, 0x71)
+#define KVM_REQUEST_GSI_ROUTE	  _IOWR(KVMIO, 0x72, void *)
+#define KVM_FREE_GSI_ROUTE	  _IOR(KVMIO, 0x73, void *)

Oh this slipped: should be struct kvm_gsi_route_guest.

include/linux/kvm.h struct kvm_assigned_irq { · line 564

Quoted 10 lines
include/linux/kvm.h +4 -0
 /*
  * ioctls for vcpu fds

@@ -553,4 +558,25 @@ struct kvm_assigned_irq {
 #define KVM_DEV_IRQ_ASSIGN_MSI_ACTION	KVM_DEV_IRQ_ASSIGN_ENABLE_MSI
 #define KVM_DEV_IRQ_ASSIGN_ENABLE_MSI	(1 << 0)
 
+struct kvm_gsi_route_guest {
+	__u32 entries_nr;
+	struct kvm_gsi_route_entry_guest *entries;
+};

And you can use a zero sized array here.

Sorry :(

author
#3 lore raw

Re: [PATCH 1/7] KVM: Add a route layer to convert MSI message to GSI

Quoted 10 lines · On Thursday 08 January 2009 22:20:22 Marcelo Tosatti wrote:

include/linux/kvm.h struct kvm_trace_rec { · line 440

Quoted 7 lines · Sheng Yang

* ioctls for VM fds

include/linux/kvm.h +2 -0
@@ -433,6 +436,8 @@ struct kvm_trace_rec {
 #define KVM_ASSIGN_IRQ _IOR(KVMIO, 0x70, \
 			    struct kvm_assigned_irq)
 #define KVM_REINJECT_CONTROL      _IO(KVMIO, 0x71)
+#define KVM_REQUEST_GSI_ROUTE	  _IOWR(KVMIO, 0x72, void *)
+#define KVM_FREE_GSI_ROUTE	  _IOR(KVMIO, 0x73, void *)

Oh this slipped: should be struct kvm_gsi_route_guest.

Oh, yeah, forgot to change it back(I original purpose a array here...)

Quoted 7 lines
Quoted 7 lines
include/linux/kvm.h +0 -0
 /*
  * ioctls for vcpu fds

@@ -553,4 +558,25 @@ struct kvm_assigned_irq {
 #define KVM_DEV_IRQ_ASSIGN_MSI_ACTION	KVM_DEV_IRQ_ASSIGN_ENABLE_MSI
 #define KVM_DEV_IRQ_ASSIGN_ENABLE_MSI	(1 << 0)
Quoted 6 lines

include/linux/kvm.h struct kvm_assigned_irq { · line 564

Quoted 4 lines
include/linux/kvm.h +4 -0
+struct kvm_gsi_route_guest {
+	__u32 entries_nr;
+	struct kvm_gsi_route_entry_guest *entries;
+};

And you can use a zero sized array here.

OK.

Quoted 2 lines

Sorry :(

Oh, that's not necessary. :)

Signature · 2 lines
regards
Yang, Sheng
#4 lore raw

Re: [PATCH 1/7] KVM: Add a route layer to convert MSI message to GSI

In reply to Sheng Yang
Quoted 11 lines · Sheng Yang

Avi's purpose, to use single kvm_set_irq() to deal with all interrupt, including MSI. So here is it.

struct gsi_route_entry is a mapping from a special gsi(with KVM_GSI_MSG_MASK) to MSI/MSI-X message address/data. And the struct can also be extended for other purpose.

Now we support up to 256 gsi_route_entry mapping, and gsi is allocated by kernel and provide two ioctls to userspace, which is more flexiable.

include/linux/kvm.h struct kvm_assigned_irq { · line 563

Quoted 6 lines
include/linux/kvm.h +2 -0
@@ -553,4 +558,25 @@ struct kvm_assigned_irq {
 #define KVM_DEV_IRQ_ASSIGN_MSI_ACTION	KVM_DEV_IRQ_ASSIGN_ENABLE_MSI
 #define KVM_DEV_IRQ_ASSIGN_ENABLE_MSI	(1 << 0)
 
+struct kvm_gsi_route_guest {
+	__u32 entries_nr;

Need padding here otherwise offsetof(entries) will differ on 32-bit and 64-bit kernels.

include/linux/kvm.h struct kvm_assigned_irq { · line 563

Quoted 2 lines
include/linux/kvm.h +1 -0
+	struct kvm_gsi_route_entry_guest *entries;

Like Marcelo says, zero sized array is better here.

include/linux/kvm.h struct kvm_assigned_irq { · line 566

Quoted 4 lines
include/linux/kvm.h +3 -0
+};
+
+#define KVM_GSI_ROUTE_MSI	(1 << 0)

This looks like a flag. Shouldn't it be a type?

include/linux/kvm.h struct kvm_assigned_irq { · line 567

Quoted 2 lines
include/linux/kvm.h +1 -0
+struct kvm_gsi_route_entry_guest {

what does _guest mean here? almost all kvm stuff is _guest related.

Quoted 5 lines
include/linux/kvm.h +4 -0
+	__u32 gsi;
+	__u32 type;
+	__u32 flags;
+	__u32 reserved;

include/linux/kvm.h struct kvm_assigned_irq { · line 581

Quoted 10 lines
include/linux/kvm.h +10 -0
+	union {
+		struct {
+			__u32 addr_lo;
+			__u32 addr_hi;
+			__u32 data;
+		} msi;
+		__u32 padding[8];
+	};
+};
+

Since we replace the entire table every time, how do ioapic/pic gsis work?

Quoted 6 lines

/* The guest did something we don't support. */

include/linux/kvm_host.h +0 -0
@@ -336,6 +339,19 @@ void kvm_unregister_irq_mask_notifier(struct kvm *kvm, int irq,
 				      struct kvm_irq_mask_notifier *kimn);
 void kvm_fire_mask_notifiers(struct kvm *kvm, int irq, bool mask);

include/linux/kvm_host.h void kvm_unregister_irq_mask_notifier(struct kvm *kvm, int irq, · line 351

Quoted 10 lines
include/linux/kvm_host.h +9 -0
+#define KVM_GSI_ROUTE_MASK    0x1000000ull
+struct kvm_gsi_route_entry {
+	u32 gsi;
+	u32 type;
+	u32 flags;
+	u32 reserved;
+	union {
+		struct msi_msg msi;
+		u32 reserved[8];

No need for reserved fields in kernel data.

Quoted 30 lines
include/linux/kvm_host.h +25 -0
+	};
+	struct hlist_node link;
+};

@@ -123,3 +123,73 @@ void kvm_fire_mask_notifiers(struct kvm *kvm, int irq, bool mask)
 			kimn->func(kimn, mask);
 }
 
+int kvm_update_gsi_route(struct kvm *kvm, struct kvm_gsi_route_entry *entry)
+{
+	struct kvm_gsi_route_entry *found_entry, *new_entry;
+	int r, gsi;
+
+	mutex_lock(&kvm->lock);
+	/* Find whether we need a update or a new entry */
+	found_entry = kvm_find_gsi_route_entry(kvm, entry->gsi);
+	if (found_entry)
+		*found_entry = *entry;
+	else {
+		gsi = find_first_zero_bit(kvm->gsi_route_bitmap,
+					  KVM_NR_GSI_ROUTE_ENTRIES);
+		if (gsi >= KVM_NR_GSI_ROUTE_ENTRIES) {
+			r = -ENOSPC;
+			goto out;
+		}
+		new_entry = kzalloc(sizeof(*new_entry), GFP_KERNEL);
+		if (!new_entry) {
+			r = -ENOMEM;
+			goto out;
+		}

include/linux/kvm_host.h void kvm_fire_mask_notifiers(struct kvm *kvm, int irq, bool mask) · line 157

Quoted 10 lines
include/linux/kvm_host.h +10 -0
+		*new_entry = *entry;
+		entry->gsi = gsi | KVM_GSI_ROUTE_MASK;
+		__set_bit(gsi, kvm->gsi_route_bitmap);
+		hlist_add_head(&new_entry->link, &kvm->gsi_route_list);
+	}
+	r = 0;
+out:
+	mutex_unlock(&kvm->lock);
+	return r;
+}

Why not throw everything and set the new table?

I didn't see where you respond the new KVM_CAP. It looks like a good place to return the maximum size of the table.

Signature · 1 line
error compiling committee.c: too many arguments to function
author
#5 lore raw

Re: [PATCH 1/7] KVM: Add a route layer to convert MSI message to GSI

Quoted 15 lines · On Thursday 08 January 2009 23:08:25 Avi Kivity wrote:
Quoted 14 lines · Sheng Yang

Avi's purpose, to use single kvm_set_irq() to deal with all interrupt, including MSI. So here is it.

struct gsi_route_entry is a mapping from a special gsi(with KVM_GSI_MSG_MASK) to MSI/MSI-X message address/data. And the struct can also be extended for other purpose.

Now we support up to 256 gsi_route_entry mapping, and gsi is allocated by kernel and provide two ioctls to userspace, which is more flexiable.

include/linux/kvm.h +0 -0
@@ -553,4 +558,25 @@ struct kvm_assigned_irq {
 #define KVM_DEV_IRQ_ASSIGN_MSI_ACTION	KVM_DEV_IRQ_ASSIGN_ENABLE_MSI
 #define KVM_DEV_IRQ_ASSIGN_ENABLE_MSI	(1 << 0)
Quoted 5 lines

include/linux/kvm.h struct kvm_assigned_irq { · line 562

Quoted 2 lines
include/linux/kvm.h +2 -0
+struct kvm_gsi_route_guest {
+	__u32 entries_nr;

Need padding here otherwise offsetof(entries) will differ on 32-bit and 64-bit kernels.

OK.

Quoted 10 lines

include/linux/kvm.h struct kvm_assigned_irq { · line 563

Quoted 2 lines
include/linux/kvm.h +1 -0
+	struct kvm_gsi_route_entry_guest *entries;

Like Marcelo says, zero sized array is better here.

include/linux/kvm.h struct kvm_assigned_irq { · line 566

Quoted 3 lines
include/linux/kvm.h +3 -0
+};
+
+#define KVM_GSI_ROUTE_MSI	(1 << 0)

This looks like a flag. Shouldn't it be a type?

Oh, custom... Would update.

Quoted 3 lines

include/linux/kvm.h struct kvm_assigned_irq { · line 567

Quoted 1 line
include/linux/kvm.h +1 -0
+struct kvm_gsi_route_entry_guest {

what does _guest mean here? almost all kvm stuff is _guest related.

Because I can't think of a good name... kvm_gsi_route_entry_guest? kvm_gsi_kernel_route_entry? What's your favorite? :)

Quoted 21 lines
Quoted 4 lines
include/linux/kvm.h +4 -0
+	__u32 gsi;
+	__u32 type;
+	__u32 flags;
+	__u32 reserved;

include/linux/kvm.h struct kvm_assigned_irq { · line 581

Quoted 10 lines
include/linux/kvm.h +10 -0
+	union {
+		struct {
+			__u32 addr_lo;
+			__u32 addr_hi;
+			__u32 data;
+		} msi;
+		__u32 padding[8];
+	};
+};
+

Since we replace the entire table every time, how do ioapic/pic gsis work?

Quoted 5 lines
include/linux/kvm_host.h +0 -0
 /* The guest did something we don't support. */

@@ -336,6 +339,19 @@ void kvm_unregister_irq_mask_notifier(struct kvm
 *kvm, int irq, struct kvm_irq_mask_notifier *kimn);
 void kvm_fire_mask_notifiers(struct kvm *kvm, int irq, bool mask);
Quoted 11 lines

include/linux/kvm_host.h void kvm_unregister_irq_mask_notifier(struct kvm *kvm, int irq, · line 350

Quoted 9 lines
include/linux/kvm_host.h +9 -0
+#define KVM_GSI_ROUTE_MASK    0x1000000ull
+struct kvm_gsi_route_entry {
+	u32 gsi;
+	u32 type;
+	u32 flags;
+	u32 reserved;
+	union {
+		struct msi_msg msi;
+		u32 reserved[8];

No need for reserved fields in kernel data.

Yeah

Quoted 33 lines
Quoted 33 lines
include/linux/kvm_host.h +4 -0
+	};
+	struct hlist_node link;
+};

@@ -123,3 +123,73 @@ void kvm_fire_mask_notifiers(struct kvm *kvm, int
 irq, bool mask) kimn->func(kimn, mask);
 }
 
+int kvm_update_gsi_route(struct kvm *kvm, struct kvm_gsi_route_entry

*entry) +{

virt/kvm/irq_comm.c +24 -0
+	struct kvm_gsi_route_entry *found_entry, *new_entry;
+	int r, gsi;
+
+	mutex_lock(&kvm->lock);
+	/* Find whether we need a update or a new entry */
+	found_entry = kvm_find_gsi_route_entry(kvm, entry->gsi);
+	if (found_entry)
+		*found_entry = *entry;
+	else {
+		gsi = find_first_zero_bit(kvm->gsi_route_bitmap,
+					  KVM_NR_GSI_ROUTE_ENTRIES);
+		if (gsi >= KVM_NR_GSI_ROUTE_ENTRIES) {
+			r = -ENOSPC;
+			goto out;
+		}
+		new_entry = kzalloc(sizeof(*new_entry), GFP_KERNEL);
+		if (!new_entry) {
+			r = -ENOMEM;
+			goto out;
+		}
+		*new_entry = *entry;
+		entry->gsi = gsi | KVM_GSI_ROUTE_MASK;
+		__set_bit(gsi, kvm->gsi_route_bitmap);
+		hlist_add_head(&new_entry->link, &kvm->gsi_route_list);
Quoted 8 lines

virt/kvm/irq_comm.c void kvm_fire_mask_notifiers(struct kvm *kvm, int irq, bool mask) · line 157

Quoted 6 lines
virt/kvm/irq_comm.c +6 -0
+	}
+	r = 0;
+out:
+	mutex_unlock(&kvm->lock);
+	return r;
+}

Why not throw everything and set the new table?

Userspace to maintain a big route table? Just for MSI/MSI-X it's easy, but for others, a global one is needed, and need follow up more maintain functions. For kernel, a little more effect can archive this, like this. So I do it in this way.

Quoted 2 lines

I didn't see where you respond the new KVM_CAP. It looks like a good place to return the maximum size of the table.

I just use it as #ifdef in userspace now, for no user other than MSI/MSI-X now. And if we keep maintaining it in kernel, we would return free size instead of maximum size..

Signature · 2 lines
regards
Yang, Sheng
#6 lore raw

Re: [PATCH 1/7] KVM: Add a route layer to convert MSI message to GSI

Quoted 8 lines · Sheng Yang
Quoted 4 lines
Quoted 2 lines

+struct kvm_gsi_route_entry_guest {

what does _guest mean here? almost all kvm stuff is _guest related.

Because I can't think of a good name... kvm_gsi_route_entry_guest? kvm_gsi_kernel_route_entry? What's your favorite? :)

kvm_gsi_route_entry?

Quoted 3 lines
Quoted 3 lines

Since we replace the entire table every time, how do ioapic/pic gsis work?

Missed this question...

Quoted 5 lines
Quoted 2 lines

Why not throw everything and set the new table?

Userspace to maintain a big route table? Just for MSI/MSI-X it's easy, but for others, a global one is needed, and need follow up more maintain functions. For kernel, a little more effect can archive this, like this. So I do it in this way.

Sorry, I don't understand the reply.

Quoted 6 lines
Quoted 3 lines

I didn't see where you respond the new KVM_CAP. It looks like a good place to return the maximum size of the table.

I just use it as #ifdef in userspace now, for no user other than MSI/MSI-X now. And if we keep maintaining it in kernel, we would return free size instead of maximum size..

We need to allow userspace to change pic/ioapic routing for the HPET.

There are two styles of maintaining the table:

1. add/remove ioctls

The advantage is that very little work needs to be done when something changes, but the code size (and bug count) doubles.

2. replace everything ioctl

Smaller code size, but slower if there are high frequency changes

I don't think we'll see high frequency interrupt routing changes; we'll probably have one on setup (for HPET), another when switching from ACPI PIC mode to ACPI APIC mode, and one for every msi initialized.

Signature · 2 lines
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.
#7 lore raw

Re: [PATCH 1/7] KVM: Add a route layer to convert MSI message to GSI

Quoted 6 lines · Avi Kivity
Quoted 5 lines · Sheng Yang
Quoted 4 lines
Quoted 2 lines

+struct kvm_gsi_route_entry_guest {

what does _guest mean here? almost all kvm stuff is _guest related.

Quoted 6 lines
Quoted 4 lines

Because I can't think of a good name... kvm_gsi_route_entry_guest? kvm_gsi_kernel_route_entry? What's your favorite? :)

kvm_gsi_route_entry?

Which is used for kernel one now...

Quoted 6 lines
Quoted 4 lines
Quoted 4 lines

Since we replace the entire table every time, how do ioapic/pic gsis work?

Missed this question...

My comments below...

Quoted 30 lines
Quoted 9 lines
Quoted 3 lines

Why not throw everything and set the new table?

Userspace to maintain a big route table? Just for MSI/MSI-X it's easy, but for others, a global one is needed, and need follow up more maintain functions. For kernel, a little more effect can archive this, like this. So I do it in this way.

Sorry, I don't understand the reply.

Quoted 8 lines
Quoted 3 lines

I didn't see where you respond the new KVM_CAP. It looks like a good place to return the maximum size of the table.

I just use it as #ifdef in userspace now, for no user other than MSI/MSI-X now. And if we keep maintaining it in kernel, we would return free size instead of maximum size..

We need to allow userspace to change pic/ioapic routing for the HPET.

There are two styles of maintaining the table:

1. add/remove ioctls

The advantage is that very little work needs to be done when something changes, but the code size (and bug count) doubles.

Quoted 7 lines

2. replace everything ioctl

Smaller code size, but slower if there are high frequency changes

I don't think we'll see high frequency interrupt routing changes; we'll probably have one on setup (for HPET), another when switching from ACPI PIC mode to ACPI APIC mode, and one for every msi initialized.

I come to option 2 mix with option 1 now. What I meant is, MSI part is in device-assignment part, and HPET and others are in some other place, so a global table should be maintained for these three across the parts of userspace. I don't like the global gsi route table, and especially we already got one in kernel space. We have to do some maintain work in the kernel space, e.g. looking up a entry, so I think a little add-on can take the job.

And now I think you original purpose is three different tables for MSI, HPET and ACPI APIC mode? This also avoid global big table in userspace. But it seems like a waste, and also too specific...

So how about this? One ioctl to replace everything, another(maybe two, transfer entry number then table, or one with maximum entries number in userspace) can export current gsi route table? This can avoid the global route table, as well as reduce the complexity.

How do you think?

Signature · 2 lines
regards
Yang, Sheng	|Intel Opensource Technology Center
#8 lore raw

Re: [PATCH 1/7] KVM: Add a route layer to convert MSI message to GSI

Quoted 10 lines · Sheng Yang
Quoted 8 lines · Avi Kivity
Quoted 6 lines · Sheng Yang
Quoted 6 lines
Quoted 3 lines

+struct kvm_gsi_route_entry_guest {

what does _guest mean here? almost all kvm stuff is _guest related.

Quoted 9 lines
Quoted 6 lines
Quoted 4 lines

Because I can't think of a good name... kvm_gsi_route_entry_guest? kvm_gsi_kernel_route_entry? What's your favorite? :)

kvm_gsi_route_entry?

Which is used for kernel one now...

So add _kernel to that...

It's more important to keep the userspace interface clean.

Quoted 23 lines
Quoted 6 lines

We need to allow userspace to change pic/ioapic routing for the HPET.

There are two styles of maintaining the table:

1. add/remove ioctls

Quoted 8 lines

The advantage is that very little work needs to be done when something changes, but the code size (and bug count) doubles.

2. replace everything ioctl

Smaller code size, but slower if there are high frequency changes

I don't think we'll see high frequency interrupt routing changes; we'll probably have one on setup (for HPET), another when switching from ACPI PIC mode to ACPI APIC mode, and one for every msi initialized.

I come to option 2 mix with option 1 now. What I meant is, MSI part is in device-assignment part, and HPET and others are in some other place, so a global table should be maintained for these three across the parts of userspace. I don't like the global gsi route table, and especially we already got one in kernel space. We have to do some maintain work in the kernel space, e.g. looking up a entry, so I think a little add-on can take the job.

Quoted 9 lines

And now I think you original purpose is three different tables for MSI, HPET and ACPI APIC mode? This also avoid global big table in userspace. But it seems like a waste, and also too specific...

So how about this? One ioctl to replace everything, another(maybe two, transfer entry number then table, or one with maximum entries number in userspace) can export current gsi route table? This can avoid the global route table, as well as reduce the complexity.

I still don't understand.

We already have all of the information needed in userspace. We know whether HPET is in use or not, whether PIC mode or APIC mode is enabled, and what MSIs are enabled. We need all this at least for live migration.

In the kernel, we'll need a function to free the table (for VM shutdown), so adding a function to populate a new table seems to be the least amount of work.

But I think we're miscommunicating, maybe I'm misunderstanding what you're suggesting.

Signature · 1 line
error compiling committee.c: too many arguments to function
#9 lore raw

Re: [PATCH 1/7] KVM: Add a route layer to convert MSI message to GSI

In reply to Avi Kivity
Quoted 15 lines · Avi Kivity
Quoted 4 lines · Sheng Yang

I just use it as #ifdef in userspace now, for no user other than MSI/MSI-X now. And if we keep maintaining it in kernel, we would return free size instead of maximum size..

We need to allow userspace to change pic/ioapic routing for the HPET.

There are two styles of maintaining the table:

1. add/remove ioctls

The advantage is that very little work needs to be done when something changes, but the code size (and bug count) doubles.

Quoted 7 lines

2. replace everything ioctl

Smaller code size, but slower if there are high frequency changes

I don't think we'll see high frequency interrupt routing changes; we'll probably have one on setup (for HPET), another when switching from ACPI PIC mode to ACPI APIC mode, and one for every msi initialized.

Hi, Avi

After reconsidering, I must say I prefer add/remove ioctls.

About the code size, I don't think it would increase much. I've rewritten the code twice, I think I know the difference is little.

For the option 2 route table ioctl, we got a array from userspace, and would convert it to linked list and keep it in kernel. That's a kind of must(I don't think you would prefer use a array in kernel), and it's very direct.

So, we have to insert/delete route entry for both. What's the real difference we do it one by one or do it all at once. I don't think it is much different on the code size. And it's indeed very clear and direct.

Beside this, option 2 seems strange. Why should we handle this table in this way when it won't result in significant code reduce. Insert/delete entry it there, look up entry is also there, not many things changed. And it's not that direct as option 1, which also can be a source of bugs.

How do you think?

Signature · 2 lines
regards
Yang, Sheng	|Intel Opensource Technology Center
#10 lore raw

Re: [PATCH 1/7] KVM: Add a route layer to convert MSI message to GSI

Quoted 5 lines · Sheng Yang

After reconsidering, I must say I prefer add/remove ioctls.

About the code size, I don't think it would increase much. I've rewritten the code twice, I think I know the difference is little.

:( sorry about that.

Quoted 4 lines

For the option 2 route table ioctl, we got a array from userspace, and would convert it to linked list and keep it in kernel. That's a kind of must(I don't think you would prefer use a array in kernel), and it's very direct.

Actually, eventually we'd want an array indexed by gsi. Each element would be a pointer to another array (one or two routing entries).

Certainly we don't want to iterate a list which could hold several hundred interrupts for a large guest.

It's okay to start with a linked list, but eventually we'll want something faster.

Quoted 11 lines

So, we have to insert/delete route entry for both. What's the real difference we do it one by one or do it all at once. I don't think it is much different on the code size. And it's indeed very clear and direct.

Beside this, option 2 seems strange. Why should we handle this table in this way when it won't result in significant code reduce. Insert/delete entry it there, look up entry is also there, not many things changed. And it's not that direct as option 1, which also can be a source of bugs.

How do you think?

I'm not convinced. Please post your latest, and I will post a counter-proposal.

Signature · 1 line
error compiling committee.c: too many arguments to function
author
#11 lore raw

Re: [PATCH 1/7] KVM: Add a route layer to convert MSI message to GSI

Quoted 13 lines · On Sunday 11 January 2009 17:38:22 Avi Kivity wrote:
Quoted 5 lines · Sheng Yang

After reconsidering, I must say I prefer add/remove ioctls.

About the code size, I don't think it would increase much. I've rewritten the code twice, I think I know the difference is little.

:( sorry about that.
:

Quoted 4 lines

For the option 2 route table ioctl, we got a array from userspace, and would convert it to linked list and keep it in kernel. That's a kind of must(I don't think you would prefer use a array in kernel), and it's very direct.

Quoted 8 lines

Actually, eventually we'd want an array indexed by gsi. Each element would be a pointer to another array (one or two routing entries).

Certainly we don't want to iterate a list which could hold several hundred interrupts for a large guest.

It's okay to start with a linked list, but eventually we'll want something faster.

Oh, I see. What I means here is allocate/deallocate would cause some memory fragments, but seems not that critical here.

Quoted 10 lines
Quoted 10 lines

So, we have to insert/delete route entry for both. What's the real difference we do it one by one or do it all at once. I don't think it is much different on the code size. And it's indeed very clear and direct.

Beside this, option 2 seems strange. Why should we handle this table in this way when it won't result in significant code reduce. Insert/delete entry it there, look up entry is also there, not many things changed. And it's not that direct as option 1, which also can be a source of bugs.

Quoted 4 lines
Quoted 1 line

How do you think?

I'm not convinced. Please post your latest, and I will post a counter-proposal.

OK.

Signature · 2 lines
regards
Yang, Sheng
#12 lore raw

Re: [PATCH 1/7] KVM: Add a route layer to convert MSI message to GSI

In reply to Avi Kivity
Quoted 5 lines · Avi Kivity

1. add/remove ioctls

The advantage is that very little work needs to be done when something changes, but the code size (and bug count) doubles.

One disadvantage of add/remove is that we cannot effect a change atomically. Probably not a big deal, but something to keep in mind.

Signature · 1 line
error compiling committee.c: too many arguments to function
author
#13 lore raw

PATCH v5 2/7 KVM: Using gsi route for MSI device assignment

In reply to Sheng Yang

Convert MSI userspace interface to support gsi_msg mapping(and nobody should be the user of the old interface...).

Signed-off-by Sheng Yang <[email protected]>
---
 include/linux/kvm_host.h |    1 -
 virt/kvm/kvm_main.c      |   79 ++++++++++++++++++++++++++--------------------
 2 files changed, 45 insertions(+), 35 deletions(-)
include/linux/kvm_host.h +0 -1
index 6a00201..eab9588 100644
@@ -316,7 +316,6 @@ struct kvm_assigned_dev_kernel {
 	int host_irq;
 	bool host_irq_disabled;
 	int guest_irq;
-	struct msi_msg guest_msi;
 #define KVM_ASSIGNED_DEV_GUEST_INTX	(1 << 0)
 #define KVM_ASSIGNED_DEV_GUEST_MSI	(1 << 1)
 #define KVM_ASSIGNED_DEV_HOST_INTX	(1 << 8)
virt/kvm/kvm_main.c +45 -34

106 lines not on this page · raw message

Signature · 1 line
1.5.4.5
#14 lore raw

Re: [PATCH 2/7] KVM: Using gsi route for MSI device assignment

Quoted 3 lines · Sheng Yang

Convert MSI userspace interface to support gsi_msg mapping(and nobody should be the user of the old interface...).

This affects device assignment only, right?

Signature · 1 line
error compiling committee.c: too many arguments to function
author
#15 lore raw

PATCH v5 3/7 KVM: Improve MSI dispatch function

In reply to Sheng Yang

Prepare to merge with kvm_set_irq().

Signed-off-by Sheng Yang <[email protected]>
---
 virt/kvm/kvm_main.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)
virt/kvm/kvm_main.c +4 -4
index 3bbb59f..fb2d639 100644
@@ -87,7 +87,7 @@ static bool kvm_rebooting;
 #ifdef KVM_CAP_DEVICE_ASSIGNMENT
 
 #ifdef CONFIG_X86
-static void assigned_device_msi_dispatch(struct kvm_assigned_dev_kernel *dev)
+static void assigned_device_msi_dispatch(struct kvm_assigned_dev_kernel *dev, u32 gsi)
 {
 	int vcpu_id;
 	struct kvm_vcpu *vcpu;

@@ -98,7 +98,7 @@ static void assigned_device_msi_dispatch(struct kvm_assigned_dev_kernel *dev)
 
 	BUG_ON(!ioapic);
 
-	gsi_entry = kvm_find_gsi_route_entry(dev->kvm, dev->guest_irq);
+	gsi_entry = kvm_find_gsi_route_entry(dev->kvm, gsi);
 	if (!gsi_entry) {
 		printk(KERN_WARNING "kvm: fail to find correlated gsi entry\n");
 		return;

@@ -145,7 +145,7 @@ static void assigned_device_msi_dispatch(struct kvm_assigned_dev_kernel *dev)
 	}
 }
 #else
-static void assigned_device_msi_dispatch(struct kvm_assigned_dev_kernel *dev) {}
+static void assigned_device_msi_dispatch(struct kvm_assigned_dev_kernel *dev, u32 gsi) {}
 #endif
 
 static struct kvm_assigned_dev_kernel *kvm_find_assigned_dev(struct list_head *head,

@@ -180,7 +180,7 @@ static void kvm_assigned_dev_interrupt_work_handler(struct work_struct *work)
 			    assigned_dev->guest_irq, 1);
 	else if (assigned_dev->irq_requested_type &
 				KVM_ASSIGNED_DEV_GUEST_MSI) {
-		assigned_device_msi_dispatch(assigned_dev);
+		assigned_device_msi_dispatch(assigned_dev, assigned_dev->guest_irq);
 		enable_irq(assigned_dev->host_irq);
 		assigned_dev->host_irq_disabled = false;
 	}
Signature · 1 line
1.5.4.5
author
#16 lore raw

PATCH v5 4/7 KVM: Using ioapic_irqchip() macro for kvm_set_irq

In reply to Sheng Yang
Signed-off-by Sheng Yang <[email protected]>
---
 virt/kvm/irq_comm.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
virt/kvm/irq_comm.c +1 -1
index 64ca4cf..f8b22a1 100644
@@ -39,7 +39,7 @@ void kvm_set_irq(struct kvm *kvm, int irq_source_id, int irq, int level)
 	 * IOAPIC.  So set the bit in both. The guest will ignore
 	 * writes to the unused one.
 	 */
-	kvm_ioapic_set_irq(kvm->arch.vioapic, irq, !!(*irq_state));
+	kvm_ioapic_set_irq(ioapic_irqchip(kvm), irq, !!(*irq_state));
 #ifdef CONFIG_X86
 	kvm_pic_set_irq(pic_irqchip(kvm), irq, !!(*irq_state));
 #endif
Signature · 1 line
1.5.4.5
author
#17 lore raw

PATCH v5 5/7 KVM: Merge MSI handling to kvm_set_irq

In reply to Sheng Yang

Using kvm_set_irq to handle all interrupt injection.

Signed-off-by Sheng Yang <[email protected]>
---
 include/linux/kvm_host.h |    2 +-
 virt/kvm/irq_comm.c      |   79 +++++++++++++++++++++++++++++++++++++++++++--
 virt/kvm/kvm_main.c      |   79 +++-------------------------------------------
 3 files changed, 81 insertions(+), 79 deletions(-)
include/linux/kvm_host.h +1 -1
index eab9588..bfdaab9 100644
@@ -351,7 +351,7 @@ struct kvm_gsi_route_entry {
 	struct hlist_node link;
 };
 
-void kvm_set_irq(struct kvm *kvm, int irq_source_id, int irq, int level);
+void kvm_set_irq(struct kvm *kvm, int irq_source_id, u32 gsi, int level);
 void kvm_notify_acked_irq(struct kvm *kvm, unsigned gsi);
 void kvm_register_irq_ack_notifier(struct kvm *kvm,
 				   struct kvm_irq_ack_notifier *kian);
virt/kvm/irq_comm.c +75 -4

94 lines not on this page · raw message

virt/kvm/kvm_main.c +5 -74

97 lines not on this page · raw message

Signature · 1 line
1.5.4.5
author
#18 lore raw

PATCH v5 6/7 KVM: Split IOAPIC structure

In reply to Sheng Yang

Prepared for reuse ioapic_redir_entry for MSI.

Signed-off-by Sheng Yang <[email protected]>
---
 include/linux/kvm_types.h |   17 +++++++++++++++++
 virt/kvm/ioapic.c         |    6 +++---
 virt/kvm/ioapic.h         |   17 +----------------
 3 files changed, 21 insertions(+), 19 deletions(-)
include/linux/kvm_types.h +17 -0
index 5f4a18c..46e3d8d 100644
@@ -52,4 +52,21 @@ struct kvm_pio_request {
 	int rep;
 };
 
+union kvm_ioapic_redirect_entry {
+	u64 bits;
+	struct {
+		u8 vector;
+		u8 delivery_mode:3;
+		u8 dest_mode:1;
+		u8 delivery_status:1;
+		u8 polarity:1;
+		u8 remote_irr:1;
+		u8 trig_mode:1;
+		u8 mask:1;
+		u8 reserve:7;
+		u8 reserved[4];
+		u8 dest_id;
+	} fields;
+};
+
 #endif /* __KVM_TYPES_H__ */
virt/kvm/ioapic.c +3 -3
index e85a2bc..b6530e9 100644
@@ -85,7 +85,7 @@ static unsigned long ioapic_read_indirect(struct kvm_ioapic *ioapic,
 
 static void ioapic_service(struct kvm_ioapic *ioapic, unsigned int idx)
 {
-	union ioapic_redir_entry *pent;
+	union kvm_ioapic_redirect_entry *pent;
 
 	pent = &ioapic->redirtbl[idx];
 

@@ -277,7 +277,7 @@ void kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int level)
 {
 	u32 old_irr = ioapic->irr;
 	u32 mask = 1 << irq;
-	union ioapic_redir_entry entry;
+	union kvm_ioapic_redirect_entry entry;
 
 	if (irq >= 0 && irq < IOAPIC_NUM_PINS) {
 		entry = ioapic->redirtbl[irq];

@@ -296,7 +296,7 @@ void kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int level)
 static void __kvm_ioapic_update_eoi(struct kvm_ioapic *ioapic, int gsi,
 				    int trigger_mode)
 {
-	union ioapic_redir_entry *ent;
+	union kvm_ioapic_redirect_entry *ent;
 
 	ent = &ioapic->redirtbl[gsi];
virt/kvm/ioapic.h +1 -16
index 49c9581..ee5b0bd 100644
@@ -40,22 +40,7 @@ struct kvm_ioapic {
 	u32 id;
 	u32 irr;
 	u32 pad;
-	union ioapic_redir_entry {
-		u64 bits;
-		struct {
-			u8 vector;
-			u8 delivery_mode:3;
-			u8 dest_mode:1;
-			u8 delivery_status:1;
-			u8 polarity:1;
-			u8 remote_irr:1;
-			u8 trig_mode:1;
-			u8 mask:1;
-			u8 reserve:7;
-			u8 reserved[4];
-			u8 dest_id;
-		} fields;
-	} redirtbl[IOAPIC_NUM_PINS];
+	union kvm_ioapic_redirect_entry redirtbl[IOAPIC_NUM_PINS];
 	struct kvm_io_device dev;
 	struct kvm *kvm;
 	void (*ack_notifier)(void *opaque, int irq);
Signature · 1 line
1.5.4.5
#19 lore raw

Re: [PATCH 6/7] KVM: Split IOAPIC structure

Quoted 1 line · Sheng Yang

Prepared for reuse ioapic_redir_entry for MSI.

What is the disadvantage of dispatching the MSI interrupts to the vcpus via the IOAPIC? Pin shortage I can think of, but adding more IOAPIC's is possible (and wanted anyway for systems with insane amounts of net/block devices).

That would avoid code duplication (might need to handle a few msi specific bits).

author
#20 lore raw

Re: [PATCH 6/7] KVM: Split IOAPIC structure

Quoted 10 lines · On Thursday 08 January 2009 23:27:21 Marcelo Tosatti wrote:
Quoted 1 line · Sheng Yang

Prepared for reuse ioapic_redir_entry for MSI.

What is the disadvantage of dispatching the MSI interrupts to the vcpus via the IOAPIC? Pin shortage I can think of, but adding more IOAPIC's is possible (and wanted anyway for systems with insane amounts of net/block devices).

That would avoid code duplication (might need to handle a few msi specific bits).

I think for current RH=1 implement, independence of IOAPIC is acceptable...

Indicated by "Redicection hint indication"(RH) bit, there are two mode that MSI would deliver: either use IOAPIC redirect table entry(RH=0), or redirect directly(RH=1). I implemented the later for now, and it's the most common way. Of course, delivery would go through IOAPIC when RH=0.

Signature · 2 lines
regards
Yang, Sheng
author
#21 lore raw

PATCH v5 7/7 KVM: Unified the delivery of IOAPIC and MSI

In reply to Sheng Yang
Signed-off-by Sheng Yang <[email protected]>
---
 include/linux/kvm_host.h |    3 ++
 virt/kvm/ioapic.c        |   84 ++++++++++++++++----------------------------
 virt/kvm/irq_comm.c      |   86 ++++++++++++++++++++++++++++------------------
 3 files changed, 86 insertions(+), 87 deletions(-)
include/linux/kvm_host.h +3 -0
index bfdaab9..2736dbf 100644
@@ -351,6 +351,9 @@ struct kvm_gsi_route_entry {
 	struct hlist_node link;
 };
 
+void kvm_get_intr_delivery_bitmask(struct kvm_ioapic *ioapic,
+				   union kvm_ioapic_redirect_entry *entry,
+				   u32 *deliver_bitmask);
 void kvm_set_irq(struct kvm *kvm, int irq_source_id, u32 gsi, int level);
 void kvm_notify_acked_irq(struct kvm *kvm, unsigned gsi);
 void kvm_register_irq_ack_notifier(struct kvm *kvm,
virt/kvm/ioapic.c +31 -53

106 lines not on this page · raw message

virt/kvm/irq_comm.c +52 -34

107 lines not on this page · raw message

Signature · 1 line
1.5.4.5