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 <m...@redhat.com>
> > > >
> > > > Signed-off-by: David Gibson <da...@gibson.dropbear.id.au>
> >
> > 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
