Re: [PATCH 2/4] KVM: pci device assignment

[Avi Kivity <avi@qumranet.com>]

Ben-Ami Yassour wrote:
> Based on a patch from: Amit Shah <amit.shah@qumranet.com>
>
> This patch adds support for handling PCI devices that are assigned to
> the guest.
>
> The device to be assigned to the guest is registered in the host kernel
> and interrupt delivery is handled. If a device is already assigned, or
> the device driver for it is still loaded on the host, the device
> assignment
> is failed by conveying a -EBUSY reply to the userspace.
>
> Devices that share their interrupt line are not supported at the moment.
>
> By itself, this patch will not make devices work within the guest.
> The VT-d extension is required to enable the device to perform DMA.
> Another alternative is PVDMA.
>
>  
> +struct kvm_assigned_dev_kernel
> +*kvm_find_assigned_dev(struct list_head *head,
>   

Keep these two on the same line.

> +
> +static int
> +kvm_vm_ioctl_device_assignment(struct kvm *kvm,
>   

Ditto.

> +			       struct kvm_assigned_dev *assigned_dev)
> +{
> +	int r = 0;
> +	struct kvm_assigned_dev_kernel *match;
> +	struct pci_dev *dev;
> +
> +	if (assigned_dev->host.num_valid_irqs != 1) {
> +		printk(KERN_INFO "%s: Unsupported number of irqs %d\n",
> +		       __func__, assigned_dev->host.num_valid_irqs);
> +		return -EINVAL;
> +	}
>   

We also support zero irqs.  While this patch doesn't do much with the 
information (only claim the device), this is still useful.


> diff --git a/include/asm-x86/kvm.h b/include/asm-x86/kvm.h
> index 8f13749..12b4b25 100644
> --- a/include/asm-x86/kvm.h
> +++ b/include/asm-x86/kvm.h
> @@ -208,4 +208,5 @@ struct kvm_pit_channel_state {
>  struct kvm_pit_state {
>  	struct kvm_pit_channel_state channels[3];
>  };
> +
>  #endif
>   

Unrelated.

> diff --git a/include/asm-x86/kvm_host.h b/include/asm-x86/kvm_host.h
> index e2864e6..34eb3e7 100644
> --- a/include/asm-x86/kvm_host.h
> +++ b/include/asm-x86/kvm_host.h
> @@ -325,6 +325,25 @@ struct kvm_irq_ack_notifier {
>  	void (*irq_acked)(struct kvm_irq_ack_notifier *kian);
>  };
>  
> +/* For assigned devices, we schedule work in the system workqueue to
> + * inject interrupts into the guest when an interrupt occurs on the
> + * physical device and also when the guest acks the interrupt.
> + */
> +struct kvm_assigned_dev_work {
> +	struct work_struct work;
> +	struct kvm_assigned_dev_kernel *assigned_dev;
> +};
> +
> +struct kvm_assigned_dev_kernel {
> +	struct kvm_irq_ack_notifier ack_notifier;
> +	struct list_head list;
> +	struct kvm_assigned_dev_info guest;
> +	struct kvm_assigned_dev_info host;
> +	struct kvm_assigned_dev_work int_work;
>   

Just put the work_struct here, and use container_of() instead of 
->assigned_dev.

>  struct kvm_arch{
>  	int naliases;
>  	struct kvm_mem_alias aliases[KVM_ALIAS_SLOTS];
> @@ -337,6 +356,7 @@ struct kvm_arch{
>  	 * Hash table of struct kvm_mmu_page.
>  	 */
>  	struct list_head active_mmu_pages;
> +	struct list_head assigned_dev_head;
>   

This is useful for non-x86 (except s390).  It's okay to leave it to the 
ia64/ppc maintainers, though.

>  	struct kvm_pic *vpic;
>  	struct kvm_ioapic *vioapic;
>  	struct kvm_pit *vpit;
> diff --git a/include/asm-x86/kvm_para.h b/include/asm-x86/kvm_para.h
> index 76f3921..3aa1731 100644
> --- a/include/asm-x86/kvm_para.h
> +++ b/include/asm-x86/kvm_para.h
> @@ -143,5 +143,4 @@ static inline unsigned int kvm_arch_para_features(void)
>  }
>  
>  #endif
> -
>   

Spurious.

>  #endif
> diff --git a/include/linux/kvm.h b/include/linux/kvm.h
> index 6edba45..c436c08 100644
> --- a/include/linux/kvm.h
> +++ b/include/linux/kvm.h
> @@ -382,6 +382,7 @@ struct kvm_trace_rec {
>  #define KVM_CAP_PV_MMU 13
>  #define KVM_CAP_MP_STATE 14
>  #define KVM_CAP_COALESCED_MMIO 15
> +#define KVM_CAP_DEVICE_ASSIGNMENT 16
>  
>  /*
>   * ioctls for VM fds
> @@ -411,6 +412,8 @@ struct kvm_trace_rec {
>  			_IOW(KVMIO,  0x67, struct kvm_coalesced_mmio_zone)
>  #define KVM_UNREGISTER_COALESCED_MMIO \
>  			_IOW(KVMIO,  0x68, struct kvm_coalesced_mmio_zone)
> +#define KVM_UPDATE_ASSIGNED_DEVICE _IOR(KVMIO, 0x69,		\
> +					struct kvm_assigned_dev)
>  
>  /*
>   * ioctls for vcpu fds
> @@ -475,4 +478,22 @@ struct kvm_trace_rec {
>  #define KVM_TRC_STLB_INVAL       (KVM_TRC_HANDLER + 0x18)
>  #define KVM_TRC_PPC_INSTR        (KVM_TRC_HANDLER + 0x19)
>  
> +#define ASSIGNED_DEV_MAX_IRQ 16
> +
> +/* Stores information for identifying host PCI devices assigned to the
> + * guest: this is used in the host kernel and in the userspace.
> + */
> +struct kvm_assigned_dev_info {
> +	__u32 busnr;
> +	__u32 devfn;
> +	__u32 irq[ASSIGNED_DEV_MAX_IRQ];
> +	__u32 num_valid_irqs; /* currently only 1 is supported */
> +};
>   

Move num_valid_irqs before the array.  Add padding so the structure is a 
multiple of 64 bits (needed so that the i386 and x86_64 ABIs are identical).


> +
> +/* Mapping between host and guest PCI device */
> +struct kvm_assigned_dev {
> +	struct kvm_assigned_dev_info guest;
> +	struct kvm_assigned_dev_info host;
> +};
> +
>   


guest.busnr and guest.devfn are meaningless; that's a wart.

What do you think of this API:

  struct kvm_assigned_pci_dev {
      __u32 assigned_dev_id;
      __u32 busnr;
      __u32 devfn;
      __u32 flags;
  };

  struct kvm_assigned_irq {
      __u32 assigned_dev_id;
      __u32 host_irq;
      __u32 guest_irq;
      __u32 flags;
  };

This automatically handles zero, one, or many irqs, and also allows for 
non-pci devices.


-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.