[Qemu-devel] Re: PCI: Fix bus address conversion (was Re: commit rules for common git tree)

[Michael S. Tsirkin]

On Mon, Jan 04, 2010 at 07:49:10PM +0000, Blue Swirl wrote:
> On Mon, Jan 4, 2010 at 7:10 PM, Michael S. Tsirkin <address@hidden> wrote:
> > On Mon, Jan 04, 2010 at 07:04:38PM +0000, Blue Swirl wrote:
> >> On Mon, Jan 4, 2010 at 6:33 PM, Michael S. Tsirkin <address@hidden> wrote:
> >> >> On Sun, Dec 27, 2009 at 05:01:38PM -0600, Anthony Liguori wrote:
> >> >> > Likewise, if you see a patch go in that you think would have benefited
> >> >> > from being on the list, point it out.  People are reasonable and if 
> >> >> > you
> >> >> > have a good suggestion, they'll value your input and be likely to seek
> >> >> > it out in the future.
> >> >
> >> > Here is another patch that would have benefitted from review
> >> > before commit:
> >> >
> >> >> commit cf616802171905a9b6d087a69caa3b978b9cd741
> >> >> Author: Blue Swirl <address@hidden>
> >> >> Date:   Sun Dec 27 20:52:36 2009 +0000
> >> >>
> >> >>     PCI: Fix bus address conversion
> >> >>
> >> >>     Pass physical addresses to map functions instead of PCI bus 
> >> >> addresses.
> >> >>
> >> >>     Signed-off-by: Blue Swirl <address@hidden>
> >> >
> >> > and previous related patches.  The issues here that I see are:
> >> >
> >> > - IMO mem_base should really be pci_bus_t, as pci address might be
> >> >  64 bit mapped into 32 bit target
> >> >
> >> > - I think pci to pci bridges need mem_base copied from parent to child,
> >> >  this does not seem to be done?
> >> >
> >> > - map functions need to get pci_bus_t (for io), and now they get
> >> >  a cpu address there. The real fix IMO is moving the mapping
> >> >  to within pci.c. I think Avi had a patch to do this -
> >> >  any objections to refreshing it?
> >> >
> >> > Blue Swirl, could you comment please?
> >>
> >> The issues you point out (which may well be valid) are not related to
> >> the change made by the patch and should be addressed by new patches.
> >
> > Yes, there's no harm in fixing them separately.  The point I was making
> > is it is better to post patches on list so issues can be pointed out and
> > discussed without the need to dig through git history.
> 
> This may happen in any case, for example if you are busy and have no
> time to review the patch in time before it's committed. It has
> happened to me many times. Also patches that seem harmless can and
> will break stuff.

Yes, it may. But hey, give people a chance.

> >> IIRC Avi promised to refresh his patch but never delivered. I think
> >> Paul also wanted that the bus translation would be handled in a more
> >> generic way (eliminate map functions).
> >
> > I'd like to eliminate map functions as well. Do you have a link to the 
> > original patch
> > btw?
> 
> I couldn't find it from the archives, maybe I didn't remember
> correctly. I think the discussions were about generic DMA.

DMA? Sounds strange ... these are PCI memory/io/ROM mappings.

-- 
MST