Re: [PATCH 2/2][RFC] KVM: Emulate MSI-X table and PBA in kernel

[Avi Kivity <avi@redhat.com>]

On 12/22/2010 10:44 AM, Sheng Yang wrote:
> Then we can support mask bit operation of assigned devices now.
>
>
> @@ -3817,14 +3819,16 @@ static int emulator_write_emulated_onepage(unsigned long addr,
>
>   mmio:
>   	trace_kvm_mmio(KVM_TRACE_MMIO_WRITE, bytes, gpa, *(u64 *)val);
> +	r = vcpu_mmio_write(vcpu, gpa, bytes, val);
>   	/*
>   	 * Is this MMIO handled locally?
>   	 */
> -	if (!vcpu_mmio_write(vcpu, gpa, bytes, val))
> +	if (!r)
>   		return X86EMUL_CONTINUE;
>
>   	vcpu->mmio_needed = 1;
> -	vcpu->run->exit_reason = KVM_EXIT_MMIO;
> +	vcpu->run->exit_reason = (r == -ENOTSYNC) ?
> +		KVM_EXIT_MSIX_ROUTING_UPDATE : KVM_EXIT_MMIO;

This isn't very pretty, exit_reason should be written in 
vcpu_mmio_write().  I guess we can refactor it later.

>
> +#define KVM_MSIX_MMIO_TYPE_ASSIGNED_DEV	    (1<<  0)
> +
> +#define KVM_MSIX_MMIO_TYPE_BASE_TABLE	    (1<<  8)
> +#define KVM_MSIX_MMIO_TYPE_BASE_PBA	    (1<<  9)
> +
> +#define KVM_MSIX_MMIO_TYPE_DEV_MASK	    0x00ff
> +#define KVM_MSIX_MMIO_TYPE_BASE_MASK	    0xff00

Any explanation of these?

> +struct kvm_msix_mmio_user {
> +	__u32 dev_id;
> +	__u16 type;
> +	__u16 max_entries_nr;
> +	__u64 base_addr;
> +	__u64 base_va;
> +	__u64 flags;
> +	__u64 reserved[4];
> +};
> +
>
>
> +int kvm_assigned_device_update_msix_mask_bit(struct kvm *kvm,
> +				int assigned_dev_id, int entry, u32 flag)
> +{

Need a better name for 'flag' (and make it a bool).

> +	int r = -EFAULT;
> +	struct kvm_assigned_dev_kernel *adev;
> +	int i;
> +
> +	if (!irqchip_in_kernel(kvm))
> +		return r;
> +
> +	mutex_lock(&kvm->lock);
> +	adev = kvm_find_assigned_dev(&kvm->arch.assigned_dev_head,
> +				      assigned_dev_id);
> +	if (!adev)
> +		goto out;
> +
> +	for (i = 0; i<  adev->entries_nr; i++)
> +		if (adev->host_msix_entries[i].entry == entry) {
> +			if (flag)
> +				disable_irq_nosync(
> +					adev->host_msix_entries[i].vector);
> +			else
> +				enable_irq(adev->host_msix_entries[i].vector);
> +			r = 0;
> +			break;
> +		}
> +out:
> +	mutex_unlock(&kvm->lock);
> +	return r;
> +}
>
> @@ -1988,6 +2008,12 @@ static int kvm_dev_ioctl_create_vm(void)
>   		return r;
>   	}
>   #endif
> +	r = kvm_register_msix_mmio_dev(kvm);
> +	if (r<  0) {
> +		kvm_put_kvm(kvm);
> +		return r;
> +	}

Shouldn't this be part of individual KVM_REGISTER_MSIX_MMIO calls?

> +static int msix_table_mmio_read(struct kvm_io_device *this, gpa_t addr, int len,
> +				void *val)
> +{
> +	struct kvm_msix_mmio_dev *mmio_dev =
> +		container_of(this, struct kvm_msix_mmio_dev, table_dev);
> +	struct kvm_msix_mmio *mmio;
> +	int idx, ret = 0, entry, offset, r;
> +
> +	mutex_lock(&mmio_dev->lock);
> +	idx = get_mmio_table_index(mmio_dev, addr, len);
> +	if (idx<  0) {
> +		ret = -EOPNOTSUPP;
> +		goto out;
> +	}
> +	if ((addr&  0x3) || (len != 4&&  len != 8))
> +		goto out;

What about (addr & 4) && (len == 8)? Is it supported? It may cross entry 
boundaries.

> +	mmio =&mmio_dev->mmio[idx];
> +
> +	entry = (addr - mmio->table_base_addr) / PCI_MSIX_ENTRY_SIZE;
> +	offset = addr&  0xf;
> +	r = copy_from_user(val, (void *)(mmio->table_base_va +
> +			entry * PCI_MSIX_ENTRY_SIZE + offset), len);


> +	if (r)
> +		goto out;
> +out:
> +	mutex_unlock(&mmio_dev->lock);
> +	return ret;
> +}
> +
> +static int msix_table_mmio_write(struct kvm_io_device *this, gpa_t addr,
> +				int len, const void *val)
> +{
> +	struct kvm_msix_mmio_dev *mmio_dev =
> +		container_of(this, struct kvm_msix_mmio_dev, table_dev);
> +	struct kvm_msix_mmio *mmio;
> +	int idx, entry, offset, ret = 0, r = 0;
> +	gpa_t entry_base;
> +	u32 old_ctrl, new_ctrl;
> +
> +	mutex_lock(&mmio_dev->lock);
> +	idx = get_mmio_table_index(mmio_dev, addr, len);
> +	if (idx<  0) {
> +		ret = -EOPNOTSUPP;
> +		goto out;
> +	}
> +	if ((addr&  0x3) || (len != 4&&  len != 8))
> +		goto out;
> +	mmio =&mmio_dev->mmio[idx];
> +	entry = (addr - mmio->table_base_addr) / PCI_MSIX_ENTRY_SIZE;
> +	entry_base = mmio->table_base_va + entry * PCI_MSIX_ENTRY_SIZE;
> +	offset = addr&  0xF;
> +
> +	if (copy_from_user(&old_ctrl,
> +			entry_base + PCI_MSIX_ENTRY_VECTOR_CTRL,
> +			sizeof old_ctrl))
> +		goto out;

get_user() is easier.

> +
> +	/* No allow writing to other fields when entry is unmasked */
> +	if (!(old_ctrl&  PCI_MSIX_ENTRY_CTRL_MASKBIT)&&
> +	    offset != PCI_MSIX_ENTRY_VECTOR_CTRL)
> +		goto out;
> +
> +	if (copy_to_user(entry_base + offset, val, len))
> +		goto out;

> +
> +	if (copy_from_user(&new_ctrl,
> +			entry_base + PCI_MSIX_ENTRY_VECTOR_CTRL,
> +			sizeof new_ctrl))
> +		goto out;

put_user()

> +
> +	if ((offset<  PCI_MSIX_ENTRY_VECTOR_CTRL&&  len == 4) ||
> +	    (offset<  PCI_MSIX_ENTRY_DATA&&  len == 8))
> +		ret = -ENOTSYNC;
> +	if (old_ctrl == new_ctrl)
> +		goto out;
> +	if (!(old_ctrl&  PCI_MSIX_ENTRY_CTRL_MASKBIT)&&
> +			(new_ctrl&  PCI_MSIX_ENTRY_CTRL_MASKBIT))
> +		r = update_msix_mask_bit(mmio_dev->kvm, mmio, entry, 1);
> +	else if ((old_ctrl&  PCI_MSIX_ENTRY_CTRL_MASKBIT)&&
> +			!(new_ctrl&  PCI_MSIX_ENTRY_CTRL_MASKBIT))
> +		r = update_msix_mask_bit(mmio_dev->kvm, mmio, entry, 0);
> +	if (r || ret)
> +		ret = -ENOTSYNC;
> +out:
> +	mutex_unlock(&mmio_dev->lock);
> +	return ret;
> +}

blank line...

> +static const struct kvm_io_device_ops msix_mmio_table_ops = {
> +	.read     = msix_table_mmio_read,
> +	.write    = msix_table_mmio_write,
> +};
> +
> ++
> +int kvm_vm_ioctl_register_msix_mmio(struct kvm *kvm,
> +				    struct kvm_msix_mmio_user *mmio_user)
> +{
> +	struct kvm_msix_mmio_dev *mmio_dev =&kvm->msix_mmio_dev;
> +	struct kvm_msix_mmio *mmio = NULL;
> +	int r = 0, i;
> +
> +	mutex_lock(&mmio_dev->lock);
> +	for (i = 0; i<  mmio_dev->mmio_nr; i++) {
> +		if (mmio_dev->mmio[i].dev_id == mmio_user->dev_id&&
> +		    (mmio_dev->mmio[i].type&  KVM_MSIX_MMIO_TYPE_DEV_MASK) ==
> +		    (mmio_user->type&  KVM_MSIX_MMIO_TYPE_DEV_MASK)) {
> +			mmio =&mmio_dev->mmio[i];
> +			if (mmio->max_entries_nr != mmio_user->max_entries_nr) {
> +				r = -EINVAL;
> +				goto out;
> +			}
> +			break;
> +		}
> +	}
> +	if (!mmio) {
> +		if (mmio_dev->mmio_nr == KVM_MSIX_MMIO_MAX) {
> +			r = -ENOSPC;
> +			goto out;
> +		}
> +		mmio =&mmio_dev->mmio[mmio_dev->mmio_nr];
> +		mmio_dev->mmio_nr++;
> +	}
> +	mmio->max_entries_nr = mmio_user->max_entries_nr;

Sanity check to avoid overflow.

> +	mmio->dev_id = mmio_user->dev_id;
> +	mmio->flags = mmio_user->flags;

Check for unsupported bits (all of them at present?)

> +	if ((mmio_user->type&  KVM_MSIX_MMIO_TYPE_DEV_MASK) ==
> +			KVM_MSIX_MMIO_TYPE_ASSIGNED_DEV)
> +		mmio->type = KVM_MSIX_MMIO_TYPE_ASSIGNED_DEV;
> +
> +	if ((mmio_user->type&  KVM_MSIX_MMIO_TYPE_BASE_MASK) ==
> +			KVM_MSIX_MMIO_TYPE_BASE_TABLE) {
> +		mmio->type |= KVM_MSIX_MMIO_TYPE_BASE_TABLE;
> +		mmio->table_base_addr = mmio_user->base_addr;
> +		mmio->table_base_va = mmio_user->base_va;

Check for va in kernel space.

> +	} else if ((mmio_user->type&  KVM_MSIX_MMIO_TYPE_BASE_MASK) ==
> +			KVM_MSIX_MMIO_TYPE_BASE_PBA) {
> +		mmio->type |= KVM_MSIX_MMIO_TYPE_BASE_PBA;
> +		mmio->pba_base_addr = mmio_user->base_addr;
> +		mmio->pba_base_va = mmio_user->base_va;
> +	}
> +out:
> +	mutex_unlock(&mmio_dev->lock);
> +	return r;
> +}
> +
> +

In all, looks reasonable.  I'd like to see documentation for it, and 
review from the pci people.  Alex, mst?

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