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: Wed, 23 Feb 2011 20:39:55 +0200 Message-ID: <20110223183955.GA28649@redhat.com> References: <1296364276-2351-1-git-send-email-sheng@linux.intel.com> <1296364276-2351-3-git-send-email-sheng@linux.intel.com> <1298420362.1668.21.camel@x201> <201102231459.04782.sheng@linux.intel.com> <1298478859.1668.52.camel@x201> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Sheng Yang , Avi Kivity , Marcelo Tosatti , kvm@vger.kernel.org To: Alex Williamson Return-path: Received: from mx1.redhat.com ([209.132.183.28]:44340 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754318Ab1BWSkG (ORCPT ); Wed, 23 Feb 2011 13:40:06 -0500 Content-Disposition: inline In-Reply-To: <1298478859.1668.52.camel@x201> Sender: kvm-owner@vger.kernel.org List-ID: On Wed, Feb 23, 2011 at 09:34:19AM -0700, Alex Williamson wrote: > On Wed, 2011-02-23 at 14:59 +0800, Sheng Yang wrote: > > On Wednesday 23 February 2011 08:19:21 Alex Williamson wrote: > > > On Sun, 2011-01-30 at 13:11 +0800, Sheng Yang wrote: > > > > +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; > > > > +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->kvm->lock); > > > > + 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; > > > > > > nit, this 0xf above. > > > > So you like another mask? > > I'm just noting that above you used 0xf and here you used 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; > > > > + > > > > + if ((offset < PCI_MSIX_ENTRY_VECTOR_CTRL && len == 4) || > > > > + (offset < PCI_MSIX_ENTRY_DATA && len == 8)) > > > > + ret = -ENOTSYNC; > > > > > > Couldn't we avoid the above get_user(new_ctrl,...) if we tested this > > > first? I'd also prefer to see a direct goto out here since it's not > > > entirely obvious that this first 'if' always falls into the below 'if'. > > > I'm not a fan of using an error code as a special non-error return > > > either. > > > > In fact when offset==PCI_MSIX_ENTRY_DATA and len ==8, we need to check new_ctrl to > > see if they indeed modified ctrl bits. > > > > I would try to make the logic here more clear. > > Isn't that what you're testing for though? > > (offset < PCI_MSIX_ENTRY_VECTOR_CTRL && len == 4) > > If this is true, we can only be writing address, upper address, or data, > not control. > > (offset < PCI_MSIX_ENTRY_DATA && len == 8) > > If this is true, we're writing address and upper address, not the > control vector. > > Additionally, I think the size an alignment test should be more > restrictive. We really only want to allow naturally aligned 4 or 8 byte > accesses. Maybe instead of: > > if ((addr & 0x3) || (len != 4 && len != 8)) > > we should do this: > > if (!(len == 4 || len == 8) || addr & (len - 1)) > > Then the test could become: > > if (offset + len <= PCI_MSIX_ENTRY_VECTOR_CTRL) > > Thanks, > > Alex Or rather if (offset + len != PCI_MSIX_ENTRY_VECTOR_CTRL + 4) we are matching offset + length to control offset + control length, and avoid depending on the fact control is the last register. -- MST