From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sheng Yang Subject: Re: [PATCH 2/3] KVM: Emulate MSI-X table in kernel Date: Tue, 1 Feb 2011 12:21:03 +0800 Message-ID: <20110201042103.GA9593@syang10-desktop> 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> <20110130043821.GA1812@syang10-desktop> <4D46B475.7010209@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Marcelo Tosatti , "Michael S. Tsirkin" , kvm@vger.kernel.org To: Avi Kivity Return-path: Received: from mga09.intel.com ([134.134.136.24]:22692 "EHLO mga09.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751789Ab1BAEUO (ORCPT ); Mon, 31 Jan 2011 23:20:14 -0500 Content-Disposition: inline In-Reply-To: <4D46B475.7010209@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On Mon, Jan 31, 2011 at 03:09:09PM +0200, Avi Kivity wrote: > 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. So it would return to QEmu. OK, let QEmu prints warning about it. -- regards Yang, Sheng > > >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 >