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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
