From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sheng Yang Subject: Re: [PATCH 1/7] KVM: Add a route layer to convert MSI message to GSI Date: Fri, 9 Jan 2009 10:50:59 +0800 Message-ID: <200901091050.59817.sheng@linux.intel.com> References: <1231411535-2461-1-git-send-email-sheng@linux.intel.com> <1231411535-2461-2-git-send-email-sheng@linux.intel.com> <496616E9.5040007@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Cc: Marcelo Tosatti , kvm@vger.kernel.org To: Avi Kivity Return-path: Received: from mga07.intel.com ([143.182.124.22]:51535 "EHLO azsmga101.ch.intel.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1752510AbZAICvO (ORCPT ); Thu, 8 Jan 2009 21:51:14 -0500 In-Reply-To: <496616E9.5040007@redhat.com> Content-Disposition: inline Sender: kvm-owner@vger.kernel.org List-ID: On Thursday 08 January 2009 23:08:25 Avi Kivity wrote: > Sheng Yang wrote: > > 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. > > > > @@ -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. OK. > > > + struct kvm_gsi_route_entry_guest *entries; > > Like Marcelo says, zero sized array is better here. > > > +}; > > + > > +#define KVM_GSI_ROUTE_MSI (1 << 0) > > This looks like a flag. Shouldn't it be a type? Oh, custom... Would update. > > +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? :) > > + __u32 gsi; > > + __u32 type; > > + __u32 flags; > > + __u32 reserved; > > + 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? > > /* 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]; > > No need for reserved fields in kernel data. Yeah > > + }; > > + 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; > > + } > > + *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? 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. > 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.. -- regards Yang, Sheng