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

[Michael S. Tsirkin]

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.

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

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

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

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