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

[David Gibson]

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?

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

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

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