MSI-X mask bit support for KVM

Then we can support mask bit operation of assigned devices now.

arch/x86/kvm/x86.c +1 -0

@@ -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);
  	/*

include/linux/kvm.h +5 -0

+struct kvm_msix_mmio_user {
+	__u32 dev_id;
+	__u16 type;
+	__u16 max_entries_nr;
+	__u64 base_addr;

virt/kvm/assigned-dev.c +27 -0

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

virt/kvm/msix_mmio.c new file +6 -0

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

virt/kvm/msix_mmio.c new file +30 -0

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

virt/kvm/msix_mmio.c new file +4 -0

+
+	/* No allow writing to other fields when entry is unmasked */
+	if (!(old_ctrl&  PCI_MSIX_ENTRY_CTRL_MASKBIT)&&
+	    offset != PCI_MSIX_ENTRY_VECTOR_CTRL)

virt/kvm/msix_mmio.c new file +8 -0

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

virt/kvm/msix_mmio.c new file +25 -0

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

Quoted 21 lines · Sheng Yang

Then we can support mask bit operation of assigned devices now.

(quoted diff) +2 -1

@@ -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;

arch/x86/kvm/x86.c static int emulator_write_emulated_onepage(unsigned long addr, · line 3831

Quoted 5 lines

arch/x86/kvm/x86.c +2 -1

  	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.

include/linux/kvm.h struct kvm_assigned_msix_entry { · line 809

Quoted 8 lines

include/linux/kvm.h +7 -0

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

Quoted 6 lines

include/linux/kvm.h +5 -0

+struct kvm_msix_mmio_user {
+	__u32 dev_id;
+	__u16 type;
+	__u16 max_entries_nr;
+	__u64 base_addr;

include/linux/kvm.h struct kvm_assigned_msix_entry {

Quoted 10 lines

include/linux/kvm.h +8 -0

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

Quoted 36 lines

virt/kvm/assigned-dev.c +27 -0

+	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

virt/kvm/kvm_main.c static int kvm_dev_ioctl_create_vm(void) · line 2015

Quoted 5 lines

virt/kvm/kvm_main.c +5 -0

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

Quoted 11 lines

(quoted diff) +1 -0

+static int msix_table_mmio_read(struct kvm_io_device *this, gpa_t addr,

int len, + void *val)

virt/kvm/msix_mmio.c new file +9 -0

+{
+	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) {

virt/kvm/msix_mmio.c line 63

Quoted 5 lines

virt/kvm/msix_mmio.c new file +5 -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.

Quoted 33 lines

virt/kvm/msix_mmio.c new file +30 -0

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

virt/kvm/msix_mmio.c line 103

Quoted 10 lines

virt/kvm/msix_mmio.c new file +10 -0

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

Quoted 4 lines

virt/kvm/msix_mmio.c new file +4 -0

+
+	/* No allow writing to other fields when entry is unmasked */
+	if (!(old_ctrl&  PCI_MSIX_ENTRY_CTRL_MASKBIT)&&
+	    offset != PCI_MSIX_ENTRY_VECTOR_CTRL)

virt/kvm/msix_mmio.c line 116

Quoted 10 lines

virt/kvm/msix_mmio.c new file +9 -0

+		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()

Quoted 8 lines

virt/kvm/msix_mmio.c new file +8 -0

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

virt/kvm/msix_mmio.c line 134

Quoted 10 lines

virt/kvm/msix_mmio.c new file +10 -0

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

Quoted 25 lines

virt/kvm/msix_mmio.c new file +25 -0

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

virt/kvm/msix_mmio.c line 193

Quoted 10 lines

virt/kvm/msix_mmio.c new file +10 -0

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

virt/kvm/msix_mmio.c line 195

Quoted 2 lines

virt/kvm/msix_mmio.c new file +2 -0

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

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

virt/kvm/msix_mmio.c line 204

Quoted 9 lines

virt/kvm/msix_mmio.c new file +9 -0

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

virt/kvm/msix_mmio.c line 222

Quoted 12 lines

virt/kvm/msix_mmio.c new file +12 -0

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

Quoted 22 lines · On Tuesday 28 December 2010 20:26:13 Avi Kivity wrote:

Quoted 21 lines · Sheng Yang

Then we can support mask bit operation of assigned devices now.

(quoted diff) +2 -1

@@ -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;

Quoted 8 lines

arch/x86/kvm/x86.c static int emulator_write_emulated_onepage(unsigned long addr, · line 3831

Quoted 5 lines

arch/x86/kvm/x86.c +2 -1

  	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.

Sure.

Quoted 10 lines

include/linux/kvm.h struct kvm_assigned_msix_entry { · line 809

Quoted 8 lines

include/linux/kvm.h +7 -0

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

I chose to use assigned device id instead of one specific table id, because every device should got at most one MSI MMIO(the same should applied to vfio device as well), and if we use specific table ID, we need way to associate with the device anyway, to perform mask/unmask or other operation. So I think it's better to use device ID here directly.

And for the table and pba address, it's due to the mapping in userspace may know the guest MSI-X table address and PBA address at different time(due to different BAR, refer to the code in assigned_dev_iomem_map() of qemu). So I purposed this API to allow each of them can be passed to kernel space individually.

Quoted 55 lines

Quoted 6 lines

include/linux/kvm.h +5 -0

+struct kvm_msix_mmio_user {
+	__u32 dev_id;
+	__u16 type;
+	__u16 max_entries_nr;
+	__u64 base_addr;

include/linux/kvm.h struct kvm_assigned_msix_entry {

Quoted 10 lines

include/linux/kvm.h +8 -0

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

Quoted 36 lines

virt/kvm/assigned-dev.c +27 -0

+	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

Quoted 7 lines

virt/kvm/kvm_main.c static int kvm_dev_ioctl_create_vm(void) · line 2015

Quoted 5 lines

virt/kvm/kvm_main.c +5 -0

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

In fact this MMIO device is more like global one for the VM, not for every devices. It should handle all MMIO from all MSI-X enabled devices, so I put it in the VM init/destroy process.

Quoted 11 lines

Quoted 11 lines

(quoted diff) +1 -0

+static int msix_table_mmio_read(struct kvm_io_device *this, gpa_t addr,

int len, + void *val)

virt/kvm/msix_mmio.c new file +9 -0

+{
+	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) {

Quoted 8 lines

virt/kvm/msix_mmio.c line 63

Quoted 5 lines

virt/kvm/msix_mmio.c new file +5 -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.

Should not supported. But I haven't found words on the PCI spec for it. So I didn't add this check.

Quoted 139 lines

Quoted 33 lines

virt/kvm/msix_mmio.c new file +30 -0

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

virt/kvm/msix_mmio.c line 103

Quoted 10 lines

virt/kvm/msix_mmio.c new file +10 -0

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

Quoted 4 lines

virt/kvm/msix_mmio.c new file +4 -0

+
+	/* No allow writing to other fields when entry is unmasked */
+	if (!(old_ctrl&  PCI_MSIX_ENTRY_CTRL_MASKBIT)&&
+	    offset != PCI_MSIX_ENTRY_VECTOR_CTRL)

virt/kvm/msix_mmio.c line 116

Quoted 10 lines

virt/kvm/msix_mmio.c new file +9 -0

+		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()

Quoted 8 lines

virt/kvm/msix_mmio.c new file +8 -0

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

virt/kvm/msix_mmio.c line 134

Quoted 10 lines

virt/kvm/msix_mmio.c new file +10 -0

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

Quoted 25 lines

virt/kvm/msix_mmio.c new file +25 -0

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

virt/kvm/msix_mmio.c line 193

Quoted 10 lines

virt/kvm/msix_mmio.c new file +10 -0

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

virt/kvm/msix_mmio.c line 195

Quoted 2 lines

virt/kvm/msix_mmio.c new file +2 -0

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

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

virt/kvm/msix_mmio.c line 204

Quoted 9 lines

virt/kvm/msix_mmio.c new file +9 -0

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

Quoted 7 lines

Quoted 7 lines

virt/kvm/msix_mmio.c new file +7 -0

+	} 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:

Quoted 8 lines

virt/kvm/msix_mmio.c line 222

Quoted 5 lines

virt/kvm/msix_mmio.c new file +5 -0

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

Would add the API document soon.

--
regards
Yang, Sheng

Quoted 44 lines · Sheng Yang

Quoted 22 lines · On Tuesday 28 December 2010 20:26:13 Avi Kivity wrote:

Quoted 21 lines · Sheng Yang

Then we can support mask bit operation of assigned devices now.

(quoted diff) +2 -1

@@ -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;

Quoted 8 lines

arch/x86/kvm/x86.c static int emulator_write_emulated_onepage(unsigned long addr, · line 3831

Quoted 5 lines

arch/x86/kvm/x86.c +2 -1

  	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.

Sure.

Quoted 9 lines

include/linux/kvm.h struct kvm_assigned_msix_entry { · line 809

Quoted 7 lines

include/linux/kvm.h +7 -0

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

Quoted 5 lines

I chose to use assigned device id instead of one specific table id, because every device should got at most one MSI MMIO(the same should applied to vfio device as well), and if we use specific table ID, we need way to associate with the device anyway, to perform mask/unmask or other operation. So I think it's better to use device ID here directly.

Table id will be needed to make things work for emulated devices.

My idea was this: we have the device id in kvm_assigned_msix_entry already. Just put table id and entry number in kvm_irq_routing_entry (create a new gsi type for this).
The result will also work for irqfd because these are mapped to gsi.

Quoted 92 lines

And for the table and pba address, it's due to the mapping in userspace may know the guest MSI-X table address and PBA address at different time(due to different BAR, refer to the code in assigned_dev_iomem_map() of qemu). So I purposed this API to allow each of them can be passed to kernel space individually.

Quoted 54 lines

Quoted 5 lines

include/linux/kvm.h +5 -0

+struct kvm_msix_mmio_user {
+	__u32 dev_id;
+	__u16 type;
+	__u16 max_entries_nr;
+	__u64 base_addr;

include/linux/kvm.h struct kvm_assigned_msix_entry {

Quoted 10 lines

include/linux/kvm.h +8 -0

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

Quoted 36 lines

virt/kvm/assigned-dev.c +27 -0

+	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

Quoted 7 lines

virt/kvm/kvm_main.c static int kvm_dev_ioctl_create_vm(void) · line 2015

Quoted 5 lines

virt/kvm/kvm_main.c +5 -0

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

In fact this MMIO device is more like global one for the VM, not for every devices. It should handle all MMIO from all MSI-X enabled devices, so I put it in the VM init/destroy process.

Quoted 11 lines

Quoted 11 lines

(quoted diff) +1 -0

+static int msix_table_mmio_read(struct kvm_io_device *this, gpa_t

addr, int len, + void *val)

virt/kvm/msix_mmio.c new file +9 -0

+{
+	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) {

Quoted 8 lines

virt/kvm/msix_mmio.c line 63

Quoted 5 lines

virt/kvm/msix_mmio.c new file +5 -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.

Quoted 2 lines

Should not supported. But I haven't found words on the PCI spec for it. So I didn't add this check.

IMPLEMENTATION NOTE
MSI-X Memory Space Structures in Read/Write Memory

....

For all accesses to MSI-X Table and MSI-X PBA fields, software must use aligned full
DWORD or aligned full QWORD transactions; otherwise, the result is undefined.

Quoted 139 lines

Quoted 139 lines

Quoted 33 lines

virt/kvm/msix_mmio.c new file +14 -0

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

virt/kvm/msix_mmio.c new file +15 -0

+{
+	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))

virt/kvm/msix_mmio.c line 103

Quoted 10 lines

virt/kvm/msix_mmio.c new file +10 -0

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

Quoted 4 lines

virt/kvm/msix_mmio.c new file +4 -0

+
+	/* No allow writing to other fields when entry is unmasked */
+	if (!(old_ctrl&  PCI_MSIX_ENTRY_CTRL_MASKBIT)&&
+	    offset != PCI_MSIX_ENTRY_VECTOR_CTRL)

virt/kvm/msix_mmio.c line 116

Quoted 10 lines

virt/kvm/msix_mmio.c new file +9 -0

+		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()

Quoted 8 lines

virt/kvm/msix_mmio.c new file +8 -0

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

virt/kvm/msix_mmio.c line 134

Quoted 10 lines

virt/kvm/msix_mmio.c new file +10 -0

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

Quoted 25 lines

virt/kvm/msix_mmio.c new file +25 -0

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

virt/kvm/msix_mmio.c line 193

Quoted 10 lines

virt/kvm/msix_mmio.c new file +10 -0

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

virt/kvm/msix_mmio.c line 195

Quoted 2 lines

virt/kvm/msix_mmio.c new file +2 -0

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

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

virt/kvm/msix_mmio.c line 204

Quoted 9 lines

virt/kvm/msix_mmio.c new file +9 -0

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

Quoted 15 lines

Quoted 7 lines

Quoted 7 lines

virt/kvm/msix_mmio.c new file +7 -0

+	} 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:

Quoted 8 lines

virt/kvm/msix_mmio.c line 222

Quoted 5 lines

virt/kvm/msix_mmio.c new file +5 -0

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

Some general comments:
PBA isn't supported in this version, which is OK, but let's not add a capability until it is, and let's not try to guess what
the interface will look like. I think keeping PBA in userspace will be hard because it needs to be modified from interrupt context. Removing the PBA stub will make the interface simpler.

Quoted 5 lines

Would add the API document soon.

--
regards
Yang, Sheng

Quoted 51 lines · On Wednesday 29 December 2010 16:31:35 Michael S. Tsirkin wrote:

Quoted 49 lines · Sheng Yang

Quoted 22 lines · On Tuesday 28 December 2010 20:26:13 Avi Kivity wrote:

Quoted 21 lines · Sheng Yang

Then we can support mask bit operation of assigned devices now.

(quoted diff) +2 -1

@@ -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;

Quoted 8 lines

arch/x86/kvm/x86.c static int emulator_write_emulated_onepage(unsigned long addr, · line 3831

Quoted 5 lines

arch/x86/kvm/x86.c +2 -1

  	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.

Sure.

Quoted 9 lines

include/linux/kvm.h struct kvm_assigned_msix_entry { · line 809

Quoted 7 lines

include/linux/kvm.h +7 -0

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

I chose to use assigned device id instead of one specific table id, because every device should got at most one MSI MMIO(the same should applied to vfio device as well), and if we use specific table ID, we need way to associate with the device anyway, to perform mask/unmask or other operation. So I think it's better to use device ID here directly.

Quoted 1 line

Table id will be needed to make things work for emulated devices.

I suppose even emulated device should got some kind of id(BDF)?

I think that is
enough for identification, which is already there, so we don't need to allocate another ID for the device - because one device would got at most one MSI-X MMIO, then use BDF or other device specific ID should be quite straightforward.

Quoted 104 lines

My idea was this: we have the device id in kvm_assigned_msix_entry already. Just put table id and entry number in kvm_irq_routing_entry (create a new gsi type for this).
The result will also work for irqfd because these are mapped to gsi.

Quoted 92 lines

And for the table and pba address, it's due to the mapping in userspace may know the guest MSI-X table address and PBA address at different time(due to different BAR, refer to the code in assigned_dev_iomem_map() of qemu). So I purposed this API to allow each of them can be passed to kernel space individually.

Quoted 54 lines

Quoted 5 lines

include/linux/kvm.h +5 -0

+struct kvm_msix_mmio_user {
+	__u32 dev_id;
+	__u16 type;
+	__u16 max_entries_nr;
+	__u64 base_addr;

include/linux/kvm.h struct kvm_assigned_msix_entry {

Quoted 10 lines

include/linux/kvm.h +8 -0

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

Quoted 36 lines

virt/kvm/assigned-dev.c +27 -0

+	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

Quoted 7 lines

virt/kvm/kvm_main.c static int kvm_dev_ioctl_create_vm(void) · line 2015

Quoted 5 lines

virt/kvm/kvm_main.c +5 -0

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

In fact this MMIO device is more like global one for the VM, not for every devices. It should handle all MMIO from all MSI-X enabled devices, so I put it in the VM init/destroy process.

Quoted 11 lines

Quoted 11 lines

(quoted diff) +1 -0

+static int msix_table_mmio_read(struct kvm_io_device *this, gpa_t

addr, int len, + void *val)

virt/kvm/msix_mmio.c new file +9 -0

+{
+	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) {

Quoted 8 lines

virt/kvm/msix_mmio.c line 63

Quoted 5 lines

virt/kvm/msix_mmio.c new file +5 -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.

Quoted 2 lines

Should not supported. But I haven't found words on the PCI spec for it. So I didn't add this check.

IMPLEMENTATION NOTE
MSI-X Memory Space Structures in Read/Write Memory

Quoted 6 lines

....

For all accesses to MSI-X Table and MSI-X PBA fields, software must use aligned full
DWORD or aligned full QWORD transactions; otherwise, the result is undefined.

Yes, this one is enough, I would add the checking.

Quoted 155 lines

Quoted 139 lines

Quoted 139 lines

Quoted 33 lines

virt/kvm/msix_mmio.c new file +14 -0

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

virt/kvm/msix_mmio.c new file +15 -0

+{
+	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))

virt/kvm/msix_mmio.c line 103

Quoted 10 lines

virt/kvm/msix_mmio.c new file +10 -0

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

Quoted 4 lines

virt/kvm/msix_mmio.c new file +4 -0

+
+	/* No allow writing to other fields when entry is unmasked */
+	if (!(old_ctrl&  PCI_MSIX_ENTRY_CTRL_MASKBIT)&&
+	    offset != PCI_MSIX_ENTRY_VECTOR_CTRL)

virt/kvm/msix_mmio.c line 116

Quoted 10 lines

virt/kvm/msix_mmio.c new file +9 -0

+		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()

Quoted 8 lines

virt/kvm/msix_mmio.c new file +8 -0

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

virt/kvm/msix_mmio.c line 134

Quoted 10 lines

virt/kvm/msix_mmio.c new file +10 -0

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

Quoted 25 lines

virt/kvm/msix_mmio.c new file +25 -0

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

virt/kvm/msix_mmio.c line 193

Quoted 10 lines

virt/kvm/msix_mmio.c new file +10 -0

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

virt/kvm/msix_mmio.c line 195

Quoted 2 lines

virt/kvm/msix_mmio.c new file +2 -0

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

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

virt/kvm/msix_mmio.c line 204

Quoted 9 lines

virt/kvm/msix_mmio.c new file +9 -0

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

Quoted 15 lines

Quoted 7 lines

Quoted 7 lines

virt/kvm/msix_mmio.c new file +7 -0

+	} 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:

Quoted 8 lines

virt/kvm/msix_mmio.c line 222

Quoted 5 lines

virt/kvm/msix_mmio.c new file +5 -0

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

Quoted 6 lines

Some general comments:
PBA isn't supported in this version, which is OK, but let's not add a capability until it is, and let's not try to guess what
the interface will look like. I think keeping PBA in userspace will be hard because it needs to be modified from interrupt context. Removing the PBA stub will make the interface simpler.

The API only get the PBA address now which should be fine. And we still have threaded irq and tasklet for accessing the userspace for interrupt handler...

--
regards
Yang, Sheng

Quoted 5 lines

Quoted 5 lines

Would add the API document soon.

--
regards
Yang, Sheng

Quoted 53 lines · Sheng Yang

Quoted 52 lines · On Wednesday 29 December 2010 16:31:35 Michael S. Tsirkin wrote:

Quoted 50 lines · Sheng Yang

Quoted 22 lines · On Tuesday 28 December 2010 20:26:13 Avi Kivity wrote:

Quoted 21 lines · Sheng Yang

Then we can support mask bit operation of assigned devices now.

(quoted diff) +2 -1

@@ -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;

Quoted 8 lines

arch/x86/kvm/x86.c static int emulator_write_emulated_onepage(unsigned long addr, · line 3831

Quoted 5 lines

arch/x86/kvm/x86.c +2 -1

  	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.

Sure.

Quoted 9 lines

include/linux/kvm.h struct kvm_assigned_msix_entry { · line 809

Quoted 7 lines

include/linux/kvm.h +7 -0

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

I chose to use assigned device id instead of one specific table id, because every device should got at most one MSI MMIO(the same should applied to vfio device as well), and if we use specific table ID, we need way to associate with the device anyway, to perform mask/unmask or other operation. So I think it's better to use device ID here directly.

Quoted 3 lines

Quoted 1 line

Table id will be needed to make things work for emulated devices.

I suppose even emulated device should got some kind of id(BDF)?

Not that I know. Look at how irqfd is defined for example, or how interrupts are sent through a gsi.
I would like to make the interface be able to support that.

Quoted 5 lines

I think that is
enough for identification, which is already there, so we don't need to allocate another ID for the device - because one device would got at most one MSI-X MMIO, then use BDF or other device specific ID should be quite straightforward.

So you propose allocating ids for emulated devices? OK. How will we map e.g. irqfds to these?

Quoted 205 lines

Quoted 104 lines

My idea was this: we have the device id in kvm_assigned_msix_entry already. Just put table id and entry number in kvm_irq_routing_entry (create a new gsi type for this).
The result will also work for irqfd because these are mapped to gsi.

Quoted 92 lines

And for the table and pba address, it's due to the mapping in userspace may know the guest MSI-X table address and PBA address at different time(due to different BAR, refer to the code in assigned_dev_iomem_map() of qemu). So I purposed this API to allow each of them can be passed to kernel space individually.

Quoted 54 lines

Quoted 5 lines

include/linux/kvm.h +5 -0

+struct kvm_msix_mmio_user {
+	__u32 dev_id;
+	__u16 type;
+	__u16 max_entries_nr;
+	__u64 base_addr;

include/linux/kvm.h struct kvm_assigned_msix_entry {

Quoted 10 lines

include/linux/kvm.h +8 -0

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

Quoted 36 lines

virt/kvm/assigned-dev.c +27 -0

+	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

Quoted 7 lines

virt/kvm/kvm_main.c static int kvm_dev_ioctl_create_vm(void) · line 2015

Quoted 5 lines

virt/kvm/kvm_main.c +5 -0

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

In fact this MMIO device is more like global one for the VM, not for every devices. It should handle all MMIO from all MSI-X enabled devices, so I put it in the VM init/destroy process.

Quoted 11 lines

Quoted 11 lines

(quoted diff) +1 -0

+static int msix_table_mmio_read(struct kvm_io_device *this,

gpa_t addr, int len, + void *val)

virt/kvm/msix_mmio.c new file +9 -0

+{
+	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) {

Quoted 8 lines

virt/kvm/msix_mmio.c line 63

Quoted 5 lines

virt/kvm/msix_mmio.c new file +5 -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.

Quoted 2 lines

Should not supported. But I haven't found words on the PCI spec for it. So I didn't add this check.

IMPLEMENTATION NOTE
MSI-X Memory Space Structures in Read/Write Memory

Quoted 6 lines

....

For all accesses to MSI-X Table and MSI-X PBA fields, software must use aligned full
DWORD or aligned full QWORD transactions; otherwise, the result is undefined.

Yes, this one is enough, I would add the checking.

Quoted 92 lines

Quoted 92 lines

Quoted 92 lines

Quoted 32 lines

virt/kvm/msix_mmio.c new file +14 -0

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

virt/kvm/msix_mmio.c new file +15 -0

+{
+	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))

virt/kvm/msix_mmio.c line 103

Quoted 10 lines

virt/kvm/msix_mmio.c new file +10 -0

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

Quoted 4 lines

virt/kvm/msix_mmio.c new file +4 -0

+
+	/* No allow writing to other fields when entry is unmasked */
+	if (!(old_ctrl&  PCI_MSIX_ENTRY_CTRL_MASKBIT)&&
+	    offset != PCI_MSIX_ENTRY_VECTOR_CTRL)

virt/kvm/msix_mmio.c line 116

Quoted 10 lines

virt/kvm/msix_mmio.c new file +9 -0

+		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()

Quoted 8 lines

virt/kvm/msix_mmio.c new file +8 -0

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

virt/kvm/msix_mmio.c line 134

Quoted 10 lines

virt/kvm/msix_mmio.c new file +10 -0

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

Quoted 9 lines

virt/kvm/msix_mmio.c new file +9 -0

+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)
+{

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

virt/kvm/msix_mmio.c

Quoted 8 lines

virt/kvm/msix_mmio.c new file +8 -0

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

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Review

Quoted 3 lines

(quoted diff) +3 -0

+		    (mmio_user->type&  KVM_MSIX_MMIO_TYPE_DEV_MASK)) {
+			mmio =&mmio_dev->mmio[i];
+			if (mmio->max_entries_nr != mmio_user->max_entries_nr) 

Quoted 58 lines

Quoted 51 lines

Quoted 35 lines

Quoted 35 lines

Quoted 5 lines

virt/kvm/msix_mmio.c new file +5 -0

+				r = -EINVAL;
+				goto out;
+			}
+			break;
+		}

virt/kvm/msix_mmio.c line 193

Quoted 10 lines

virt/kvm/msix_mmio.c new file +10 -0

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

virt/kvm/msix_mmio.c line 195

Quoted 2 lines

virt/kvm/msix_mmio.c new file +2 -0

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

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

virt/kvm/msix_mmio.c line 204

Quoted 9 lines

virt/kvm/msix_mmio.c new file +9 -0

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

Quoted 15 lines

Quoted 7 lines

Quoted 7 lines

virt/kvm/msix_mmio.c new file +7 -0

+	} 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:

Quoted 8 lines

virt/kvm/msix_mmio.c line 222

Quoted 5 lines

virt/kvm/msix_mmio.c new file +5 -0

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

Quoted 6 lines

Some general comments:
PBA isn't supported in this version, which is OK, but let's not add a capability until it is, and let's not try to guess what
the interface will look like. I think keeping PBA in userspace will be hard because it needs to be modified from interrupt context. Removing the PBA stub will make the interface simpler.

Quoted 3 lines

The API only get the PBA address now which should be fine. And we still have threaded irq and tasklet for accessing the userspace for interrupt handler...

I don't think it's going to work: we are not
in the context of the right process. Further
I think we should keep the option of
reading the PBA status from the device or host kernel open. And generally having an interface
for functionality we don't implement is not a good idea:
you don't know whether you really can support the interface you promised.

Quoted 10 lines

--
regards
Yang, Sheng

Quoted 5 lines

Quoted 5 lines

Would add the API document soon.

--
regards
Yang, Sheng

Quoted 62 lines · On Wednesday 29 December 2010 17:28:24 Michael S. Tsirkin wrote:

Quoted 53 lines · Sheng Yang

Quoted 52 lines · On Wednesday 29 December 2010 16:31:35 Michael S. Tsirkin wrote:

Quoted 50 lines · Sheng Yang

Quoted 22 lines · On Tuesday 28 December 2010 20:26:13 Avi Kivity wrote:

Quoted 21 lines · Sheng Yang

Then we can support mask bit operation of assigned devices now.

(quoted diff) +2 -1

@@ -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;

Quoted 8 lines

Review static int

Quoted 5 lines

(quoted diff) +2 -1

  	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.

Sure.

Quoted 9 lines

Review

Quoted 7 lines

(quoted diff) +7 -0

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

I chose to use assigned device id instead of one specific table id, because every device should got at most one MSI MMIO(the same should applied to vfio device as well), and if we use specific table ID, we need way to associate with the device anyway, to perform mask/unmask or other operation. So I think it's better to use device ID here directly.

Quoted 3 lines

Quoted 1 line

Table id will be needed to make things work for emulated devices.

I suppose even emulated device should got some kind of id(BDF)?

Not that I know. Look at how irqfd is defined for example, or how interrupts are sent through a gsi.
I would like to make the interface be able to support that.

Quoted 8 lines

Quoted 5 lines

I think that is
enough for identification, which is already there, so we don't need to allocate another ID for the device - because one device would got at most one MSI-X MMIO, then use BDF or other device specific ID should be quite straightforward.

So you propose allocating ids for emulated devices? OK. How will we map e.g. irqfds to these?

I don't understand. I've checked virtio-pci.c which is using irqfd, and it's still a PCI device, and still have BDF, right?

Also, what we want is a way to identify the MSI-X MMIO. For assigned device, we use BDF, then we can easily identify the MMIO as well as the device. For others, even they don't have BDF(I don't think so, because MSI-X is a part of PCI, and every PCI device has BDF), what you need is an ID, no matter what it is and how it defined. QEmu can get the allocation done, and the type field in this API can still tell which kind of ID/devices they are, then determine how to deal with them.

Quoted 205 lines

Quoted 205 lines

Quoted 104 lines

My idea was this: we have the device id in kvm_assigned_msix_entry already. Just put table id and entry number in kvm_irq_routing_entry (create a new gsi type for this).
The result will also work for irqfd because these are mapped to gsi.

Quoted 92 lines

And for the table and pba address, it's due to the mapping in userspace may know the guest MSI-X table address and PBA address at different time(due to different BAR, refer to the code in assigned_dev_iomem_map() of qemu). So I purposed this API to allow each of them can be passed to kernel space individually.

Quoted 54 lines

Quoted 5 lines

(quoted diff) +5 -0

+struct kvm_msix_mmio_user {
+	__u32 dev_id;
+	__u16 type;
+	__u16 max_entries_nr;
+	__u64 base_addr;

Review

Quoted 10 lines

(quoted diff) +8 -0

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

Quoted 36 lines

(quoted diff) +27 -0

+	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

Quoted 7 lines

Review static int kvm_dev_ioctl_create_vm(void) · line 2018

Quoted 5 lines

(quoted diff) +5 -0

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

In fact this MMIO device is more like global one for the VM, not for every devices. It should handle all MMIO from all MSI-X enabled devices, so I put it in the VM init/destroy process.

Quoted 11 lines

Quoted 11 lines

(quoted diff) +1 -0

+static int msix_table_mmio_read(struct kvm_io_device *this,

gpa_t addr, int len, + void *val)

(quoted diff) +9 -0

+{
+	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) {

Quoted 8 lines

Review

Quoted 5 lines

(quoted diff) +5 -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.

Quoted 2 lines

Should not supported. But I haven't found words on the PCI spec for it. So I didn't add this check.

IMPLEMENTATION NOTE
MSI-X Memory Space Structures in Read/Write Memory

Quoted 6 lines

....

For all accesses to MSI-X Table and MSI-X PBA fields, software must use aligned full
DWORD or aligned full QWORD transactions; otherwise, the result is undefined.

Yes, this one is enough, I would add the checking.

Quoted 92 lines

Quoted 92 lines

Quoted 92 lines

Quoted 32 lines

(quoted diff) +14 -0

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

(quoted diff) +15 -0

+{
+	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))

Review

Quoted 10 lines

(quoted diff) +10 -0

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

Quoted 4 lines

(quoted diff) +4 -0

+
+	/* No allow writing to other fields when entry is unmasked */
+	if (!(old_ctrl&  PCI_MSIX_ENTRY_CTRL_MASKBIT)&&
+	    offset != PCI_MSIX_ENTRY_VECTOR_CTRL)

Review

Quoted 10 lines

(quoted diff) +9 -0

+		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()

Quoted 8 lines

(quoted diff) +8 -0

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

Review

Quoted 10 lines

(quoted diff) +10 -0

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

Quoted 9 lines

(quoted diff) +9 -0

+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)
+{

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Review

Quoted 8 lines

(quoted diff) +8 -0

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

==

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Review

Quoted 3 lines

(quoted diff) +3 -0

+		    (mmio_user->type&  KVM_MSIX_MMIO_TYPE_DEV_MASK)) {
+			mmio =&mmio_dev->mmio[i];
+			if (mmio->max_entries_nr != mmio_user->max_entries_nr) 

{

Quoted 62 lines

Quoted 58 lines

Quoted 51 lines

Quoted 35 lines

Quoted 35 lines

Quoted 5 lines

(quoted diff) +5 -0

+				r = -EINVAL;
+				goto out;
+			}
+			break;
+		}

Review

Quoted 10 lines

(quoted diff) +10 -0

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

Review

Quoted 2 lines

(quoted diff) +2 -0

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

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

Review

Quoted 9 lines

(quoted diff) +9 -0

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

Quoted 15 lines

Quoted 7 lines

Quoted 7 lines

(quoted diff) +7 -0

+	} 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:

Quoted 8 lines

Review

Quoted 5 lines

(quoted diff) +5 -0

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

Quoted 6 lines

Some general comments:
PBA isn't supported in this version, which is OK, but let's not add a capability until it is, and let's not try to guess what
the interface will look like. I think keeping PBA in userspace will be hard because it needs to be modified from interrupt context. Removing the PBA stub will make the interface simpler.

Quoted 3 lines

The API only get the PBA address now which should be fine. And we still have threaded irq and tasklet for accessing the userspace for interrupt handler...

Quoted 7 lines

I don't think it's going to work: we are not
in the context of the right process. Further
I think we should keep the option of
reading the PBA status from the device or host kernel open. And generally having an interface
for functionality we don't implement is not a good idea:
you don't know whether you really can support the interface you promised.

Well, I don't know if we want to read PBA from device directly. To me it's not a good idea because the real device has nothing to do with the one we show to the guest. At least direct accessing the mask bits of real device would be very dangerous. Avi?

--
regards
Yang, Sheng

Quoted 10 lines

Quoted 10 lines

--
regards
Yang, Sheng

Quoted 5 lines

Quoted 5 lines

Would add the API document soon.

--
regards
Yang, Sheng

Quoted 77 lines · Sheng Yang

Quoted 64 lines · On Wednesday 29 December 2010 17:28:24 Michael S. Tsirkin wrote:

Quoted 55 lines · Sheng Yang

Quoted 54 lines · On Wednesday 29 December 2010 16:31:35 Michael S. Tsirkin wrote:

Quoted 52 lines · Sheng Yang

Quoted 24 lines · On Tuesday 28 December 2010 20:26:13 Avi Kivity wrote:

Quoted 23 lines · Sheng Yang

Then we can support mask bit operation of assigned devices now.

(quoted diff) +2 -1

@@ -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;

Quoted 8 lines

Review

Quoted 5 lines

  	vcpu->mmio_needed = 1;

(quoted diff) +2 -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.

Sure.

Quoted 9 lines

Review

Quoted 7 lines

(quoted diff) +7 -0

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

I chose to use assigned device id instead of one specific table id, because every device should got at most one MSI MMIO(the same should applied to vfio device as well), and if we use specific table ID, we need way to associate with the device anyway, to perform mask/unmask or other operation. So I think it's better to use device ID here directly.

Quoted 3 lines

Quoted 1 line

Table id will be needed to make things work for emulated devices.

I suppose even emulated device should got some kind of id(BDF)?

Not that I know. Look at how irqfd is defined for example, or how interrupts are sent through a gsi.
I would like to make the interface be able to support that.

Quoted 8 lines

Quoted 5 lines

I think that is
enough for identification, which is already there, so we don't need to allocate another ID for the device - because one device would got at most one MSI-X MMIO, then use BDF or other device specific ID should be quite straightforward.

So you propose allocating ids for emulated devices? OK. How will we map e.g. irqfds to these?

I don't understand. I've checked virtio-pci.c which is using irqfd, and it's still a PCI device, and still have BDF, right?

Quoted 7 lines

Also, what we want is a way to identify the MSI-X MMIO. For assigned device, we use BDF, then we can easily identify the MMIO as well as the device. For others, even they don't have BDF(I don't think so, because MSI-X is a part of PCI, and every PCI device has BDF), what you need is an ID, no matter what it is and how it defined. QEmu can get the allocation done, and the type field in this API can still tell which kind of ID/devices they are, then determine how to deal with them.

Yes, the PCI device can be identified with e.g. BFD
(won't work for multi-domain but then we can write an allocator maybe). But how will we inject these interrupts?
We can do this now with GSI ioctl or map GSI to irqfd
and inject with irqfd write.

Quoted 207 lines

Quoted 207 lines

Quoted 207 lines

Quoted 106 lines

My idea was this: we have the device id in kvm_assigned_msix_entry already. Just put table id and entry number in kvm_irq_routing_entry (create a new gsi type for this). The result will also work for irqfd because these are mapped to gsi.

Quoted 94 lines

And for the table and pba address, it's due to the mapping in userspace may know the guest MSI-X table address and PBA address at different time(due to different BAR, refer to the code in assigned_dev_iomem_map() of qemu). So I purposed this API to allow each of them can be passed to kernel space individually.

Quoted 55 lines

Quoted 13 lines

(quoted diff) +11 -0

+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

Quoted 2 lines

*kvm, + int assigned_dev_id, int entry, u32 flag)
+{

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

Quoted 37 lines

(quoted diff) +27 -0

+	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

Quoted 8 lines

Review

Quoted 5 lines

(quoted diff) +5 -0

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

In fact this MMIO device is more like global one for the VM, not for every devices. It should handle all MMIO from all MSI-X enabled devices, so I put it in the VM init/destroy process.

Quoted 11 lines

Quoted 11 lines

+static int msix_table_mmio_read(struct kvm_io_device *this,
gpa_t addr, int len, + void *val)

(quoted diff) +9 -0

+{
+	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) {

Quoted 8 lines

Review

Quoted 5 lines

(quoted diff) +5 -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.

Quoted 2 lines

Should not supported. But I haven't found words on the PCI spec for it. So I didn't add this check.

IMPLEMENTATION NOTE
MSI-X Memory Space Structures in Read/Write Memory

Quoted 6 lines

....

For all accesses to MSI-X Table and MSI-X PBA fields, software must use aligned full
DWORD or aligned full QWORD transactions; otherwise, the result is undefined.

Yes, this one is enough, I would add the checking.

Quoted 92 lines

Quoted 92 lines

Quoted 92 lines

Quoted 37 lines

(quoted diff) +3 -0

+	mmio =&mmio_dev->mmio[idx];
+
+	entry = (addr - mmio->table_base_addr) /

PCI_MSIX_ENTRY_SIZE; + offset = addr& 0xf;

(quoted diff) +10 -0

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

(quoted diff) +18 -0

+{
+	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;

Review

Quoted 5 lines

(quoted diff) +5 -0

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

get_user() is easier.

Quoted 4 lines

(quoted diff) +2 -0

+
+	/* No allow writing to other fields when entry is unmasked

*/ + if (!(old_ctrl& PCI_MSIX_ENTRY_CTRL_MASKBIT)&&

(quoted diff) +1 -0

+	    offset != PCI_MSIX_ENTRY_VECTOR_CTRL)

Review

Quoted 10 lines

(quoted diff) +9 -0

+		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()

Quoted 8 lines

(quoted diff) +8 -0

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

Review

Quoted 10 lines

(quoted diff) +10 -0

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

Quoted 9 lines

+static const struct kvm_io_device_ops msix_mmio_table_ops =
{ + .read = msix_table_mmio_read,

(quoted diff) +7 -0

+	.write    = msix_table_mmio_write,
+};
+
++
+int kvm_vm_ioctl_register_msix_mmio(struct kvm *kvm,
+				    struct kvm_msix_mmio_user *mmio_user)
+{

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Review

Quoted 8 lines

(quoted diff) +8 -0

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

Quoted 6 lines

==

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Review

Quoted 3 lines

(quoted diff) +3 -0

+		    (mmio_user->type&  KVM_MSIX_MMIO_TYPE_DEV_MASK)) {
+			mmio =&mmio_dev->mmio[i];
+			if (mmio->max_entries_nr != mmio_user-

max_entries_nr)

Quoted 74 lines

{

Quoted 62 lines

Quoted 58 lines

Quoted 51 lines

Quoted 35 lines

Quoted 35 lines

Quoted 5 lines

(quoted diff) +5 -0

+				r = -EINVAL;
+				goto out;
+			}
+			break;
+		}

Review

Quoted 10 lines

(quoted diff) +10 -0

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

Review

Quoted 2 lines

(quoted diff) +2 -0

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

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

Review

Quoted 9 lines

(quoted diff) +9 -0

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

Quoted 15 lines

Quoted 7 lines

Quoted 7 lines

(quoted diff) +1 -0

+	} else if ((mmio_user->type&  KVM_MSIX_MMIO_TYPE_BASE_MASK)

== + KVM_MSIX_MMIO_TYPE_BASE_PBA) {

(quoted diff) +5 -0

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

Quoted 8 lines

Review

Quoted 5 lines

(quoted diff) +5 -0

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

Quoted 6 lines

Some general comments:
PBA isn't supported in this version, which is OK, but let's not add a capability until it is, and let's not try to guess what the interface will look like. I think keeping PBA in userspace will be hard because it needs to be modified from interrupt context. Removing the PBA stub will make the interface simpler.

Quoted 3 lines

The API only get the PBA address now which should be fine. And we still have threaded irq and tasklet for accessing the userspace for interrupt handler...

Quoted 8 lines

I don't think it's going to work: we are not
in the context of the right process. Further
I think we should keep the option of
reading the PBA status from the device or host kernel open. And generally having an interface
for functionality we don't implement is not a good idea:
you don't know whether you really can support the interface you promised.

Quoted 8 lines

Well, I don't know if we want to read PBA from device directly. To me it's not a good idea because the real device has nothing to do with the one we show to the guest. At least direct accessing the mask bits of real device would be very dangerous. Avi?

--
regards
Yang, Sheng

I am not really suggesting this. What I say is PBA is unimplemented let us not commit to an interface yet.

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

--
regards
Yang, Sheng

Quoted 5 lines

Quoted 5 lines

Would add the API document soon.

--
regards
Yang, Sheng

Quoted 86 lines · On Thursday 30 December 2010 15:47:48 Michael S. Tsirkin wrote:

Quoted 84 lines · Sheng Yang

Quoted 64 lines · On Wednesday 29 December 2010 17:28:24 Michael S. Tsirkin wrote:

Quoted 55 lines · Sheng Yang

Quoted 54 lines · On Wednesday 29 December 2010 16:31:35 Michael S. Tsirkin wrote:

Quoted 52 lines · Sheng Yang

Quoted 24 lines · On Tuesday 28 December 2010 20:26:13 Avi Kivity wrote:

Quoted 23 lines · Sheng Yang

Then we can support mask bit operation of assigned devices now.

(quoted diff) +2 -1

@@ -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;

Quoted 8 lines

Review

Quoted 5 lines

  	vcpu->mmio_needed = 1;

(quoted diff) +2 -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.

Sure.

Quoted 9 lines

Review

Quoted 7 lines

(quoted diff) +7 -0

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

I chose to use assigned device id instead of one specific table id, because every device should got at most one MSI MMIO(the same should applied to vfio device as well), and if we use specific table ID, we need way to associate with the device anyway, to perform mask/unmask or other operation. So I think it's better to use device ID here directly.

Quoted 3 lines

Quoted 1 line

Table id will be needed to make things work for emulated devices.

I suppose even emulated device should got some kind of id(BDF)?

Not that I know. Look at how irqfd is defined for example, or how interrupts are sent through a gsi.
I would like to make the interface be able to support that.

Quoted 8 lines

Quoted 5 lines

I think that is
enough for identification, which is already there, so we don't need to allocate another ID for the device - because one device would got at most one MSI-X MMIO, then use BDF or other device specific ID should be quite straightforward.

So you propose allocating ids for emulated devices? OK. How will we map e.g. irqfds to these?

I don't understand. I've checked virtio-pci.c which is using irqfd, and it's still a PCI device, and still have BDF, right?

Also, what we want is a way to identify the MSI-X MMIO. For assigned device, we use BDF, then we can easily identify the MMIO as well as the device. For others, even they don't have BDF(I don't think so, because MSI-X is a part of PCI, and every PCI device has BDF), what you need is an ID, no matter what it is and how it defined. QEmu can get the allocation done, and the type field in this API can still tell which kind of ID/devices they are, then determine how to deal with them.

Quoted 5 lines

Yes, the PCI device can be identified with e.g. BFD
(won't work for multi-domain but then we can write an allocator maybe). But how will we inject these interrupts?
We can do this now with GSI ioctl or map GSI to irqfd
and inject with irqfd write.

I suppose it's not in the scope of this patch...

But I think you can still do
this, everything is the same as before. QEmu can read from table to get data/address pair, then program the routing table, etc.

--
regards
Yang, Sheng

Quoted 207 lines

Quoted 207 lines

Quoted 207 lines

Quoted 207 lines

Quoted 106 lines

My idea was this: we have the device id in kvm_assigned_msix_entry already. Just put table id and entry number in kvm_irq_routing_entry (create a new gsi type for this). The result will also work for irqfd because these are mapped to gsi.

Quoted 94 lines

And for the table and pba address, it's due to the mapping in userspace may know the guest MSI-X table address and PBA address at different time(due to different BAR, refer to the code in assigned_dev_iomem_map() of qemu). So I purposed this API to allow each of them can be passed to kernel space individually.

Quoted 55 lines

Quoted 13 lines

(quoted diff) +11 -0

+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

Quoted 2 lines

*kvm, + int assigned_dev_id, int entry, u32 flag)
+{

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

Quoted 37 lines

(quoted diff) +27 -0

+	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

Quoted 8 lines

Review

Quoted 5 lines

(quoted diff) +5 -0

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

In fact this MMIO device is more like global one for the VM, not for every devices. It should handle all MMIO from all MSI-X enabled devices, so I put it in the VM init/destroy process.

Quoted 11 lines

Quoted 11 lines

+static int msix_table_mmio_read(struct kvm_io_device *this,
gpa_t addr, int len, + void *val)

(quoted diff) +9 -0

+{
+	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) {

Quoted 8 lines

Review

Quoted 5 lines

(quoted diff) +5 -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.

Quoted 2 lines

Should not supported. But I haven't found words on the PCI spec for it. So I didn't add this check.

IMPLEMENTATION NOTE
MSI-X Memory Space Structures in Read/Write Memory

Quoted 6 lines

....

For all accesses to MSI-X Table and MSI-X PBA fields, software must use aligned full
DWORD or aligned full QWORD transactions; otherwise, the result is undefined.

Yes, this one is enough, I would add the checking.

Quoted 92 lines

Quoted 92 lines

Quoted 92 lines

Quoted 37 lines

(quoted diff) +3 -0

+	mmio =&mmio_dev->mmio[idx];
+
+	entry = (addr - mmio->table_base_addr) /

PCI_MSIX_ENTRY_SIZE; + offset = addr& 0xf;

(quoted diff) +10 -0

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

(quoted diff) +18 -0

+{
+	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;

Review

Quoted 5 lines

(quoted diff) +5 -0

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

get_user() is easier.

Quoted 4 lines

(quoted diff) +2 -0

+
+	/* No allow writing to other fields when entry is unmasked

*/ + if (!(old_ctrl& PCI_MSIX_ENTRY_CTRL_MASKBIT)&&

(quoted diff) +1 -0

+	    offset != PCI_MSIX_ENTRY_VECTOR_CTRL)

Review

Quoted 10 lines

(quoted diff) +9 -0

+		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()

Quoted 8 lines

(quoted diff) +8 -0

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

Review

Quoted 10 lines

(quoted diff) +10 -0

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

Quoted 9 lines

+static const struct kvm_io_device_ops msix_mmio_table_ops =
{ + .read = msix_table_mmio_read,

(quoted diff) +7 -0

+	.write    = msix_table_mmio_write,
+};
+
++
+int kvm_vm_ioctl_register_msix_mmio(struct kvm *kvm,
+				    struct kvm_msix_mmio_user *mmio_user)
+{

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Review

Quoted 8 lines

(quoted diff) +8 -0

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

Quoted 102 lines

Quoted 6 lines

==

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Review

Quoted 3 lines

(quoted diff) +3 -0

+		    (mmio_user->type&  KVM_MSIX_MMIO_TYPE_DEV_MASK)) {
+			mmio =&mmio_dev->mmio[i];
+			if (mmio->max_entries_nr != mmio_user-

max_entries_nr)

Quoted 74 lines

{

Quoted 62 lines

Quoted 58 lines

Quoted 51 lines

Quoted 35 lines

Quoted 35 lines

Quoted 5 lines

(quoted diff) +5 -0

+				r = -EINVAL;
+				goto out;
+			}
+			break;
+		}

Review

Quoted 10 lines

(quoted diff) +10 -0

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

Review

Quoted 2 lines

(quoted diff) +2 -0

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

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

Review

Quoted 9 lines

(quoted diff) +9 -0

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

Quoted 15 lines

Quoted 7 lines

Quoted 7 lines

(quoted diff) +1 -0

+	} else if ((mmio_user->type&  KVM_MSIX_MMIO_TYPE_BASE_MASK)

== + KVM_MSIX_MMIO_TYPE_BASE_PBA) {

(quoted diff) +5 -0

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

Quoted 8 lines

Review

Quoted 5 lines

(quoted diff) +5 -0

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

Quoted 6 lines

Some general comments:
PBA isn't supported in this version, which is OK, but let's not add a capability until it is, and let's not try to guess what the interface will look like. I think keeping PBA in userspace will be hard because it needs to be modified from interrupt context. Removing the PBA stub will make the interface simpler.

Quoted 3 lines

The API only get the PBA address now which should be fine. And we still have threaded irq and tasklet for accessing the userspace for interrupt handler...

Quoted 8 lines

I don't think it's going to work: we are not
in the context of the right process. Further
I think we should keep the option of
reading the PBA status from the device or host kernel open. And generally having an interface
for functionality we don't implement is not a good idea:
you don't know whether you really can support the interface you promised.

Quoted 8 lines

Well, I don't know if we want to read PBA from device directly. To me it's not a good idea because the real device has nothing to do with the one we show to the guest. At least direct accessing the mask bits of real device would be very dangerous. Avi?

--
regards
Yang, Sheng

I am not really suggesting this. What I say is PBA is unimplemented let us not commit to an interface yet.

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

--
regards
Yang, Sheng

Quoted 5 lines

Quoted 5 lines

Would add the API document soon.

--
regards
Yang, Sheng