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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
