[Qemu-devel] Re: [PATCH 00/20] PCI express clean up patches.

["Michael S. Tsirkin" <mst@redhat.com>]

On Thu, Nov 12, 2009 at 02:58:28PM +0900, Isaku Yamahata wrote:
> Here is the patch series to clean up PCI express patches.
> Although there remained some issues to address, the PCI express
> patches was commited while I wasn't responsive last week. (Sorry for that)
> This patch series addresses the remained issues.
> 
> They are mostly trivial fixes or cosmetics suggested by Michael.
> I think I've covered almost all the issues. If I missed anything,
> please be kind to point it out again.

One small comment: please keep patch subject to 50 chars max.
Otherwise it overflows with e.g. git log --oneline.

> Some random comments:
> 
> - PCI configuration space register constant.
>   Now they're defined in pci.h and their symbol name is same to Linux's
>   pci_regs.h.
>   So it would make sense to import Linux pci_regs.h and remove the
>   definitions in pci.h. If this is acceptable, I'll create the patch.

Yes, good idea.

> - PCI configuration space helper functions.
>   Now range checking helper functions are introduced.
>   range_overlaps() and so on.
>   So it's possible to clean up each PCI devices by using them.
>   If acceptable, I'll create the patch.

idem

> - endian swap related to PCI host bridge/guest endian/host endian:
>   I gave up to address this.
>   I'll leave it to someone who knows PPC spec well and has access to
>   a big endian host machine.
> 
> - PCI bridge clean up:
>   PCI bridge stuff needs more clean up. Possibly it would result
>   in a new file pci_bridge.c.
>   I'd like to address it later. Anyway I have to do it when
>   I implement PCI express hot plug.

+1 to pci_bridge.c

> - PCI express initialization:
>   Since it uses PCI initialization code, so it isn't separated
>   from PCI cleanly.
>   One possible way is to introduce PCI express specific qdev
>   register function (PCIEDeviceInfo, pcie_qdev_register() and
>   pcie_qdev_init() which calls pci_qdev_init()).
>   I'm not sure it's worth while at the moment so I'd like to
>   leave it untouched for now.

Right. It's pretty clean IMO, no need to touch just yet.

> thanks,
> 
> Isaku Yamahata (20):
>   pci: fix pci_info_device().
>   pci: move pci_data_{read, write}() declaration from pci.h to
>     pci_host.h
>   pci: simplify pci_data_read(), pcie_mmcfg_data_read().
>   pci: remove pci_addr_to_config() by open code
>   pci: rename pci_addr_to_dev(), pcie_mmcfg_addr_to_dev().
>   pci: shorten pci_host_{conf, data}_register_xxx function a bit.
>   pci: remove pci_sub_bus() by open coding.
>   pci: s/pci_find_host_bus/pci_find_root_bus/g
>   pci_host: remove unnecessary & 0xff.
>   pci: kill unnecessary included in pci.c
>   pci: clean up of pci_init_wmask().
>   pci: remove some unnecessary comment in pci.h
>   pci: move typedef, PCIHostState, PCIExpressHost to qemu-common.h.
>   pci: remove unused constants.
>   pci: clean up of pci_update_mappings()
>   pci: kill goto in pci_update_mappings()
>   pci: remove magic number, 256 in pci.c
>   pci: fix pci_config_get_io_base().
>   pci: pci bridge related clean up.
>   pci: remove goto in pci_bridge_filter().

I put these on top of my pci patches, and tweaked
the following ones:
   pci: simplify pci_data_read(), pcie_mmcfg_data_read().
   pci: kill goto in pci_update_mappings()
   pci: remove goto in pci_bridge_filter().
   pci: move pci_data_{read, write}() declaration from pci.h to
     pci_host.h

- in the way that I have specified in reply to individual patches.

The combined tree is here:
git://git.kernel.org/pub/scm/linux/kernel/git/mst/qemu.git pci

(or will be in a couple of minutes).

Please take a look, and if this all makes sense, please use them. I'll
be on vacation starting tomorrow, maybe just include this whole tree in
patch series you send out.

-- 
MST