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
--
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
