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

[Avi Kivity <avi@redhat.com>]

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.

> +	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?

> +struct kvm_gsi_route_entry_guest {
>   
what does _guest mean here? almost all kvm stuff is _guest related.

> +	__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.

> +	};
> +	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?

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.

-- 
error compiling committee.c: too many arguments to function