Re: [Qemu-devel] Re: [PATCH 07/26] pci/p2pbr: generic pci p2p bridge

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

On Thu, Mar 17, 2011 at 11:08:51AM +0900, Isaku Yamahata wrote:
> On Wed, Mar 16, 2011 at 11:34:42PM +0200, Michael S. Tsirkin wrote:
> > On Wed, Mar 16, 2011 at 06:29:18PM +0900, Isaku Yamahata wrote:
> > > Create generic pci p2p bridge device which can be customized
> > > via properties like vendor id/device id and so on.
> > > With this, we can avoid to create many pci p2p bridge which only
> > > differs in those ids.
> > > 
> > > Cc: Michael S. Tsirkin <mst@redhat.com>
> > > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> > 
> > So we added 213 lines and we saved all of 20 in other places?
> > Maybe I miss the point ...
> 
> What are missing is,
> - The patch eliminates logic duplication rather than simple
>   line insertion/deletion.
> - It also simplifies q35 code which is 26/26. Its code saving isn't counted.
> - The lines of newly added copyright notice are counted.
> - If line saving is so important, the numbers of lines can be
>   reduced dramatically by accepting 14 arguments functions instead
>   of using struct. struct initialization bloated line insertion.
>   But I think it doesn't increase code complexity.
> 

But doesn't seem to reduce it either.
Line count is just an attempt to see how well the abstraction works.
All this one does is move from pci_config_set_vendor_id(dev, XXX) to
.vendor_id = XXX. This just does not seem to be a big win.

qdev properties are also user-visible, aren't they? Adding properties
that, if changed, will confuse the guest doesn't seem to be a good idea
either.

> Anyway this patch isn't very critical. I think the available choice is
> 
> - this patch
> - modify the patch to use 14 arguments function.
>   Thus we can save much more lines.
> - Add one more p2p bridge code which q35 uses, accepting same code which
>   differs only in IDs.
> - any other ideas?
> 
> Which option do you prefer?

Add one more bridge for q35.

> > 
> > 
> > > ---
> > >  Makefile.objs  |    2 +-
> > >  hw/pci_p2pbr.c |  151 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  hw/pci_p2pbr.h |   61 +++++++++++++++++++++++
> > >  3 files changed, 213 insertions(+), 1 deletions(-)
> > >  create mode 100644 hw/pci_p2pbr.c
> > >  create mode 100644 hw/pci_p2pbr.h
> > 
> 
> -- 
> yamahata