Re: [PATCH 2/3] KVM: Emulate MSI-X table in kernel

[Avi Kivity <avi@redhat.com>]

On 01/30/2011 06:38 AM, Sheng Yang wrote:
> (Sorry, missed this mail...)
>
> On Mon, Jan 17, 2011 at 02:29:44PM +0200, Avi Kivity wrote:
> >  On 01/06/2011 12:19 PM, Sheng Yang wrote:
> >  >Then we can support mask bit operation of assigned devices now.
> >  >
> >  >
> >  >
> >  >+int kvm_assigned_device_update_msix_mask_bit(struct kvm *kvm,
> >  >+				int assigned_dev_id, int entry, bool mask)
> >  >+{
> >  >+	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 (mask)
> >  >+				disable_irq_nosync(
> >  >+					adev->host_msix_entries[i].vector);
> >
> >  Is it okay to call disable_irq_nosync() here?  IIRC we don't check
> >  the mask bit on irq delivery, so we may forward an interrupt to the
> >  guest after the mask bit was set.
> >
> >  What does pci say about the mask bit?  when does it take effect?
> >
> >  Another question is whether disable_irq_nosync() actually programs
> >  the device mask bit, or not.  If it does, then it's slow, and it may
> >  be better to leave interrupts enabled but have an internal pending
> >  bit.  If it doesn't program the mask bit, it's fine.
>
> I think Michael and Jan had explained this.
> >
> >  >+			else
> >  >+				enable_irq(adev->host_msix_entries[i].vector);
> >  >+			r = 0;
> >  >+			break;
> >  >+		}
> >  >+out:
> >  >+	mutex_unlock(&kvm->lock);
> >  >+	return r;
> >  >+}
> >  >
> >  >+
> >  >+static int msix_table_mmio_read(struct kvm_io_device *this, gpa_t addr, int len,
> >  >+				void *val)
> >  >+{
> >  >+	struct kvm_msix_mmio_dev *mmio_dev =
> >  >+		container_of(this, struct kvm_msix_mmio_dev, table_dev);
> >  >+	struct kvm_msix_mmio *mmio;
> >  >+	int idx, ret = 0, entry, offset, r;
> >  >+
> >  >+	mutex_lock(&mmio_dev->lock);
> >  >+	idx = get_mmio_table_index(mmio_dev, addr, len);
> >  >+	if (idx<   0) {
> >  >+		ret = -EOPNOTSUPP;
> >  >+		goto out;
> >  >+	}
> >  >+	if ((addr&   0x3) || (len != 4&&   len != 8))
> >  >+		goto out;
> >  >+
> >  >+	offset = addr&   0xf;
> >  >+	if (offset == PCI_MSIX_ENTRY_VECTOR_CTRL&&   len == 8)
> >  >+		goto out;
> >  >+
> >  >+	mmio =&mmio_dev->mmio[idx];
> >  >+	entry = (addr - mmio->table_base_addr) / PCI_MSIX_ENTRY_SIZE;
> >  >+	r = copy_from_user(val, (void __user *)(mmio->table_base_va +
> >  >+			entry * PCI_MSIX_ENTRY_SIZE + offset), len);
> >  >+	if (r)
> >  >+		goto out;
> >
> >  and return ret == 0?
>
> Yes. This operation should be handled by in-kernel MSI-X MMIO. So we return 0
> in order to omit this action. We can add warning to it later.

But it failed.  We need to return -EFAULT.

> The same as above.
> >
> >  >+
> >  >+	if ((offset<   PCI_MSIX_ENTRY_VECTOR_CTRL&&   len == 4) ||
> >  >+	    (offset<   PCI_MSIX_ENTRY_DATA&&   len == 8))
> >  >+		ret = -ENOTSYNC;
> >
> >  goto out?
>
> No. This judgement only check if MSI data/address was touched. And the line
> below would check if we need to operate mask bit. Because in theory guest can
> use len=8 to modify MSI-X data and ctrl at the same time.
>

Ok, makes sense.

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