Re: [Qemu-devel] [PATCH] pci: Factor out bounds checking on config space accesses

[David Gibson]

On Thu, Mar 29, 2012 at 11:28:19AM +0200, Michael S. Tsirkin wrote:
> On Thu, Mar 29, 2012 at 02:53:52PM +1100, David Gibson wrote:
> > On Wed, Mar 28, 2012 at 11:30:56AM +0200, Michael S. Tsirkin wrote:
> > > On Wed, Mar 28, 2012 at 12:11:52PM +1100, David Gibson wrote:
> > > > Michael,
> > > > 
> > > > Any chance of an ack or nack on this one?
> > > > 
> > > > On Mon, Mar 19, 2012 at 03:58:11PM +1100, David Gibson wrote:
> > > > > There are several paths into the code to emulate PCI config space 
> > > > > accesses:
> > > > > one for MMIO to a plain old PCI bridge one for MMIO to a PCIe bridge 
> > > > > and
> > > > > one for the pseries machine which provides para-virtualized access to 
> > > > > PCI
> > > > > config space.  Each of these functions does their own bounds checking
> > > > > against the size of config space to check for addresses outside the
> > > > > size of config space.  The pci_host_config_{read,write}_common() (sort
> > > > > of) checks for partial overruns, that is where the address is within
> > > > > the size of config space, but address + length is not, it takes a
> > > > > limit parameter for this purpose.
> > > > > 
> > > > > As well as being a small code duplication, and it being weird to
> > > > > separate the checks for partial and total overruns, this checking
> > > > > currently has a few buglets:
> > > > > 
> > > > >     * For non PCI-Express we assume that the size of config space is
> > > > >       PCI_CONFIG_SPACE_SIZE.  That's true for everything we emulate
> > > > >       now, but is not necessarily true (e.g. PCI-X devices can have
> > > > >       extended config space)
> > > > > 
> > > > >     * The limit parameter is not necessary, since the size of config
> > > > >       space can be obtained using pci_config_size()
> > > > > 
> > > > >     * Partial overruns could only occur with a misaligned access,
> > > > >       which should have already been dealt with by this point
> > > > > 
> > > > >     * Partial overruns are handled as a partial read or write, which
> > > > >       is very unlikely behaviour for real hardware
> > > > > 
> > > > >     * Furthermore, partial reads are 0x0 padded, whereas returning
> > > > >       0xff for unimplemented addresses us much more common.
> > > > > 
> > > > >     * The partial reads/writes only work correctly by assuming
> > > > >       little-endian byte layout.  While that is always true for PCI
> > > > >       config space, it's an awfully subtle thing to rely on without
> > > > >       comment.
> > > 
> > > This last point can be addressed by adding a comment?
> > > Patch welcome.
> > 
> > Well, it could.  But why comment on the subtle assumptions of code
> > which implements a totally bizarre behaviour, rather than just
> > removing the bizarre behaviour.
> > 
> > > 
> > > > > This patch, therefore, moves the bounds checking wholly into
> > > > > pci_host_config_{read,write}_common().  No partial reads or writes are
> > > > > performed, instead any out-of-bounds write is simply ignored and an
> > > > > out-of-bounds read returns 0xff.
> > > > > 
> > > > > This simplifies all the callers, and makes the overall semantics saner
> > > > > for edge cases.
> > > > > 
> > > > > Cc: Michael S. Tsirkin <address@hidden>
> > > > > 
> > > > > Signed-off-by: David Gibson <address@hidden>
> > > 
> > > Sorry, I didn't reply because I have no idea whether this patch is
> > > correct.
> > 
> > Well, what do you need to decide one way or the other?
> > 
> > Would it help to split this up into micro-patches which address single
> > aspects of the points covered in the patch description?
> 
> That's always good.
> But most importantly, I'd like to get the motivation straight.

Ok, I'll split the patch up.

> > > Couldn't figure out from the description whether there's a test case
> > > where we differ from real hardware in our behaviour.
> > 
> > Not sure what you mean by a testcase here.  Do you mean a formal
> > automated testcase somewhere, or just are there cases in which the
> > existing behaviour differs from hardware.
> 
> No, not formal. Simply
> - what these cases are
> - what is the actual versus expected behaviour
> - how to observe the difference from the guest

Ok, I'll try to be clearer about that in the split patch comments.

However, in this case doing exactly the same thing as real hardware
isn't particularly important - the guest has already done something
probably fatally wrong.  But the point is that it's silly to have
extra code, with subtle and non-obvious assumptions just to implement
a behaviour, in an error case, that will appear on no hardware.

> >  If the latter, then yes,
> > absolutely.  In fact I'd be astonished if there is *any* hardware
> > which implements partial writes (or reads) the way the existing code
> > does.
> 
> We could classify such a difference as a minor bug.
> The fix might turn out to be different for different platforms.

The point isn't really that what we do is different from hardware, but
that it's more complex than necessary, useless and without even the

> > > The change affects lots of platforms and there's no mention of which
> > > ones were tested.
> > 
> > Only pseries, I'm afraid, because that's all I've really got guest
> > setups available to try.
> 
> Then it would be better to find a way to limit the change to that
> platform.

That would completely defeat the purpose, since the result would be
more complex rather than less.

> Alternatively need to get others interested enough to spend cycles
> on testing your patches.

Well, I was trying to get the PCI maintainer interested, but he seems
to be in unbelievably pointless nitpicking mode.

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson