Re: [Qemu-devel] Re: [PATCH 00/10] pci: pci_register_bar_simple

[Michael S. Tsirkin]

On Tue, Apr 05, 2011 at 09:02:45PM +0300, Blue Swirl wrote:
> On Tue, Apr 5, 2011 at 12:26 AM, Michael S. Tsirkin <address@hidden> wrote:
> > On Tue, Apr 05, 2011 at 12:05:08AM +0300, Blue Swirl wrote:
> >> On Mon, Apr 4, 2011 at 10:44 PM, Michael S. Tsirkin <address@hidden> wrote:
> >> > On Mon, Apr 04, 2011 at 08:02:23PM +0300, Blue Swirl wrote:
> >> >> On Mon, Apr 4, 2011 at 7:35 PM, Avi Kivity <address@hidden> wrote:
> >> >> > On 04/04/2011 07:22 PM, Anthony Liguori wrote:
> >> >> >>
> >> >> >> On 04/04/2011 10:59 AM, Michael S. Tsirkin wrote:
> >> >> >>>
> >> >> >>> On Mon, Apr 04, 2011 at 06:27:57PM +0300, Avi Kivity wrote:
> >> >> >>>>
> >> >> >>>> Many PCI BARs that use the memory address space map a single MMIO 
> >> >> >>>> region
> >> >> >>>> into
> >> >> >>>> the entire BAR range.  Introduce an API pci_register_bar_simple() 
> >> >> >>>> for
> >> >> >>>> that use
> >> >> >>>> case, and convert all users where this can be done trivially.
> >> >> >>>>
> >> >> >>>> This will reduce the work required to introduce a PCI memory API; 
> >> >> >>>> it's
> >> >> >>>> also
> >> >> >>>> a nice code reduction in its own right.
> >> >> >>>
> >> >> >>> This will save some code, so
> >> >> >>> Acked-by: Michael S. Tsirkin<address@hidden>
> >> >> >>>
> >> >> >>> I really hope the rest of devices will follow.
> >> >> >>
> >> >> >> How complete is this?
> >> >> >
> >> >> > I converted all devices which were easy to convert.  There may be one 
> >> >> > or two
> >> >> > more that can be converted with additional work (and perhaps with an
> >> >> > additional pic_bar_get_current_address() API, and a 
> >> >> > pci_bar_set_coalescing()
> >> >> > API).  The rest likely need to stick with the callback-based API.
> >> >>
> >> >> In my version which I sent earlier but didn't commit, also other BARs
> >> >> besides the first one and also tricky devices like VGA were handled.
> >> >
> >> > Yes, I liked that patchset too. What happened to it?
> >>
> >> Nothing, but I thought that there could be a "perfect" solution.
> >>
> >> I like in Avi's version that unnecessary API changes are avoided.
> >
> > Yes, it's nice that it's incremental.
> >
> >> >> But I didn't commit it because I felt it was not going to right
> >> >> direction. I think the BARs should be specified in PCIDeviceInfo
> >> >> instead of adding more function calls. The same applies to this patch
> >> >> set.
> >> >
> >> > Is that really that fundamental? What I do care about is
> >> > making pci.c track and register all device memory
> >> > so that we can finally implement pci bridge features
> >> > such as master abort handling and unmapped memory.
> >>
> >> The structure version can be done later. Right, pci.c should manage
> >> the device mappings.
> >
> > OK, so applying Avi's patchset and building on that is
> > your preferred approach too?
> 
> Avi's version is a bit too simple, at least multiple regions in a BAR
> should be handled (for example macio.c needs that). But also that can
> be added later (pci_register_bar_not_so_simple()?), so Avi's version
> could be used as the starting point.

OK I've put it on my branch so people have the chance to
comment, and they can be built upon.

-- 
MST