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

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

[Michael S. Tsirkin]

> 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 <blauwirbel@gmail.com>
> 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 <blauwirbel@gmail.com>

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?

-- 
MST

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

[Aurelien Jarno]

On Mon, Jan 04, 2010 at 08:33:56PM +0200, Michael S. Tsirkin 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:
> 

This is the kind of patch I send to the mailing first first.

I am still waiting for you to list patches from Anthony or me, that is 
the only two persons that have been pointed into your original mail.

-- 
Aurelien Jarno	                        GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [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:47:00PM +0100, Aurelien Jarno wrote:
> I am still waiting for you to list patches from Anthony or me,

Why should I go look for them?  I thought you basically agreed it's a
good idea to post patches publically either before or - for trivial
patches - after commit?

-- 
MST

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

[Blue Swirl]

On Mon, Jan 4, 2010 at 6:33 PM, Michael S. Tsirkin <mst@redhat.com> 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 <blauwirbel@gmail.com>
>> 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 <blauwirbel@gmail.com>
>
> 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.

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

[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:04:38PM +0000, Blue Swirl wrote:
> On Mon, Jan 4, 2010 at 6:33 PM, Michael S. Tsirkin <mst@redhat.com> 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 <blauwirbel@gmail.com>
> >> 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 <blauwirbel@gmail.com>
> >
> > 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.

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

-- 
MST

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

[Blue Swirl]

On Mon, Jan 4, 2010 at 7:10 PM, Michael S. Tsirkin <mst@redhat.com> 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 <mst@redhat.com> 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 <blauwirbel@gmail.com>
>> >> 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 <blauwirbel@gmail.com>
>> >
>> > 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.

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

[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 <mst@redhat.com> 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 <mst@redhat.com> 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 <blauwirbel@gmail.com>
> >> >> 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 <blauwirbel@gmail.com>
> >> >
> >> > 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

Re: [Qemu-devel] Re: PCI: Fix bus address conversion

[Andreas Färber]

Am 04.01.2010 um 19:33 schrieb Michael S. Tsirkin:

>> 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 <blauwirbel@gmail.com>
>> 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 <blauwirbel@gmail.com>

While irrelevant for this discussion, for the record, this one was  
posted to and tested on OpenBIOS list instead.

Andreas