From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Michael S. Tsirkin" Subject: Re: [PATCH 2/3] KVM: Emulate MSI-X table in kernel Date: Mon, 17 Jan 2011 15:31:40 +0200 Message-ID: <20110117133140.GA25295@redhat.com> References: <1294309185-21417-1-git-send-email-sheng@linux.intel.com> <1294309185-21417-3-git-send-email-sheng@linux.intel.com> <4D343638.30004@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Sheng Yang , Marcelo Tosatti , kvm@vger.kernel.org To: Avi Kivity Return-path: Received: from mx1.redhat.com ([209.132.183.28]:14685 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752026Ab1AQNb6 (ORCPT ); Mon, 17 Jan 2011 08:31:58 -0500 Content-Disposition: inline In-Reply-To: <4D343638.30004@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: 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? It is only guaranteed to take effect on the next read. In practice, it takes effect earlier and guests rely on this as an optimization. > 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. > > >+ 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? > > >+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; > >+ u32 *ctrl_pos; > >+ > >+ 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; > >+ entry_base = mmio->table_base_va + entry * PCI_MSIX_ENTRY_SIZE; > >+ ctrl_pos = (u32 *)(entry_base + PCI_MSIX_ENTRY_VECTOR_CTRL); > >+ > >+ if (get_user(old_ctrl, ctrl_pos)) > >+ goto out; > >+ > >+ /* No allow writing to other fields when entry is unmasked */ > >+ if (!(old_ctrl& PCI_MSIX_ENTRY_CTRL_MASKBIT)&& > >+ offset != PCI_MSIX_ENTRY_VECTOR_CTRL) > >+ goto out; > >+ > >+ if (copy_to_user((void __user *)(entry_base + offset), val, len)) > >+ goto out; > >+ > >+ if (get_user(new_ctrl, ctrl_pos)) > >+ goto out; > > here, too. > > >+ > >+ if ((offset< PCI_MSIX_ENTRY_VECTOR_CTRL&& len == 4) || > >+ (offset< PCI_MSIX_ENTRY_DATA&& len == 8)) > >+ ret = -ENOTSYNC; > > goto out? > > >+ if (old_ctrl == new_ctrl) > >+ goto out; > >+ if (!(old_ctrl& PCI_MSIX_ENTRY_CTRL_MASKBIT)&& > >+ (new_ctrl& PCI_MSIX_ENTRY_CTRL_MASKBIT)) > >+ r = update_msix_mask_bit(mmio_dev->kvm, mmio, entry, 1); > >+ else if ((old_ctrl& PCI_MSIX_ENTRY_CTRL_MASKBIT)&& > >+ !(new_ctrl& PCI_MSIX_ENTRY_CTRL_MASKBIT)) > >+ r = update_msix_mask_bit(mmio_dev->kvm, mmio, entry, 0); > >+ if (r || ret) > >+ ret = -ENOTSYNC; > >+out: > >+ mutex_unlock(&mmio_dev->lock); > >+ return ret; > >+} > >+ > > -- > error compiling committee.c: too many arguments to function