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