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

[Alex Williamson <alex.williamson@redhat.com>]

On Wed, 2011-02-23 at 20:39 +0200, Michael S. Tsirkin wrote:
> 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.

That's just silly.  We're writing code to match a hardware
specification.  The registers always land at the same place, are always
the same size and always have the alignment requirements dictated in the
spec.  I see no problem depending on that.  They both do the same thing,
I prefer my version.

Alex