Re: [Qemu-devel] [PATCH] pci: Error on PCI capability collisions

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

On Tue, 2011-08-23 at 22:30 +0300, Michael S. Tsirkin wrote:
> On Tue, Aug 23, 2011 at 01:12:19PM -0600, Alex Williamson wrote:
> > On Tue, 2011-08-23 at 21:26 +0300, Michael S. Tsirkin wrote:
> > > On Tue, Aug 23, 2011 at 12:21:47PM -0600, Alex Williamson wrote:
> > > > On Tue, 2011-08-23 at 21:17 +0300, Michael S. Tsirkin wrote:
> > > > > On Tue, Aug 23, 2011 at 07:28:08PM +0200, Jan Kiszka wrote:
> > > > > > From: Alex Williamson <alex.williamson@redhat.com>
> > > > > > 
> > > > > > Nothing good can happen when we overlap capabilities
> > > > > > 
> > > > > > [ Jan: rebased over qemu, minor formatting ]
> > > > > > 
> > > > > > Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> > > > > 
> > > > > I'll stick an assert there instead. Normal devices
> > > > > don't generate overlapping caps unless there's a bug,
> > > > > and device assignment should do it's own checks.
> > > > > 
> > > > > I really have a mind to rip out the used array too.
> > > > 
> > > > So you'd rather kill qemu rather than have a reasonable error return
> > > > path... great :(
> > > > 
> > > > Alex
> > > 
> > > Well that will make it possible to make pci_add_capability return void,
> > > less work for callers :) Dev assignment is really the only place where
> > > capability offsets need to be verified.
> > 
> > A few issues with that... Since when is error handling so difficult that
> > we need to pretend that nothing ever fails just to make it easy for the
> > caller?
> 
> It isn't but no need to introduce error codes just for fun.

Likewise no need to remove error handling code because you dislike it.

> >  Why is device assignment such a special case?
> 
> Assigned devices are under the guest control so should be assumed
> untrusted, and we must verify anything we get from them.

The device is under guest control, but setting up access to the device
is entirely under qemu control.

> For example, I think it's generally a mistake to read a device
> register and use that as an array index, we must check it's in range
> first. It's best to do these range checks in the dev assignment code
> so that it's easy to verify that all values are used safely.

Except you're assuming that emulated drivers always get it right and
don't need such range checks, which is completely bogus.  We can either
bloat the device assignment code with wrappers that do range checks for
every pci callback or have pci check for everybody and maybe we'll catch
a bug or two...

> > It's actually
> > rather ironic that we're trying to add error checking to catch bugs that
> > real hardware is exposing, but assuming that emulated drivers always get
> > it right.  How will a return void help the emulated driver that has a
> > coding error?
> 
> Drivers use fixed offsets so they will always fail or always work.

Drivers "often" used fixed offsets.  I don't see any requirement for
this.

> If we return an error they might seem to work but behave incrrectly
> without the right capability.

And if we return void and blast capabilities over top of each other,
that's ok?

Alex