Patchwork [07/26] pci/p2pbr: generic pci p2p bridge

login
register
mail settings
Submitter Isaku Yamahata
Date March 16, 2011, 9:29 a.m.
Message ID <392c8eaaeb8f0729ab39b261d0dda21208c05ea6.1300266238.git.yamahata@valinux.co.jp>
Download mbox | patch
Permalink /patch/87210/
State New
Headers show

Comments

Isaku Yamahata - March 16, 2011, 9:29 a.m.
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
Michael S. Tsirkin - March 16, 2011, 9:34 p.m.
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 ...


> ---
>  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
Isaku Yamahata - March 17, 2011, 2:08 a.m.
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.

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?

> 
> 
> > ---
> >  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
>
Michael S. Tsirkin - March 17, 2011, 5:17 a.m.
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
Isaku Yamahata - March 17, 2011, 5:26 a.m.
On Thu, Mar 17, 2011 at 07:17:18AM +0200, Michael S. Tsirkin wrote:
> > 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.

Okay.
How about other pci related patches? I think they are trivial.
Michael S. Tsirkin - March 17, 2011, 5:31 a.m.
On Thu, Mar 17, 2011 at 02:26:24PM +0900, Isaku Yamahata wrote:
> On Thu, Mar 17, 2011 at 07:17:18AM +0200, Michael S. Tsirkin wrote:
> > > 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.
> 
> Okay.
> How about other pci related patches? I think they are trivial.

I'll look at the usage to see how good new APIs are.

> -- 
> yamahata

Patch

differs in those ids.

Cc: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
 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

diff --git a/Makefile.objs b/Makefile.objs
index a52f42f..5cb7010 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -171,7 +171,7 @@  hw-obj-y =
 hw-obj-y += vl.o loader.o
 hw-obj-$(CONFIG_VIRTIO) += virtio.o virtio-console.o
 hw-obj-y += fw_cfg.o
-hw-obj-$(CONFIG_PCI) += pci.o pci_bridge.o
+hw-obj-$(CONFIG_PCI) += pci.o pci_bridge.o pci_p2pbr.o
 hw-obj-$(CONFIG_PCI) += msix.o msi.o
 hw-obj-$(CONFIG_PCI) += pci_host.o pcie_host.o
 hw-obj-$(CONFIG_PCI) += ioh3420.o xio3130_upstream.o xio3130_downstream.o
diff --git a/hw/pci_p2pbr.c b/hw/pci_p2pbr.c
new file mode 100644
index 0000000..e5b03e2
--- /dev/null
+++ b/hw/pci_p2pbr.c
@@ -0,0 +1,151 @@ 
+/*
+ * QEMU PCI P2P generic bridge.
+ * In order to avoid create many P2P bridge device which only differs
+ * in vendor id/device id and so on.
+ *
+ * Copyright (c) 2011 Isaku Yamahata <yamahata at valinux co jp>
+ *                    VA Linux Systems Japan K.K.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include "pci_bridge.h"
+#include "pci_internals.h"
+#include "pci_p2pbr.h"
+
+typedef struct PCIP2PBridge {
+    struct PCIBridge br;
+
+    /* device specific initialization */
+    pci_p2pbr_init_fn initfn;
+
+    /* properties */
+    uint16_t vendor_id;
+    uint16_t device_id;
+    uint8_t revision_id;
+    uint8_t prog_interface;
+
+    uint8_t ssvid_cap;
+    uint16_t svid;
+    uint16_t ssid;
+} PCIP2PBridge;
+
+static int pci_p2pbr_initfn(PCIDevice *d)
+{
+    PCIBridge *br = DO_UPCAST(PCIBridge, dev, d);
+    PCIP2PBridge *p2pbr = DO_UPCAST(PCIP2PBridge, br, br);
+    uint8_t *config = d->config;
+    int rc;
+
+    rc = pci_bridge_initfn(d);
+    if (rc < 0) {
+        return rc;
+    }
+
+    pci_config_set_vendor_id(config, p2pbr->vendor_id);
+    pci_config_set_device_id(config, p2pbr->device_id);
+    pci_config_set_revision(config, p2pbr->revision_id);
+    pci_config_set_prog_interface(config, p2pbr->prog_interface);
+
+    if (p2pbr->ssvid_cap > 0) {
+        rc = pci_bridge_ssvid_init(d, p2pbr->ssvid_cap,
+                                   p2pbr->svid, p2pbr->ssid);
+        if (rc < 0) {
+            return rc;
+        }
+    }
+
+    if (p2pbr->initfn) {
+        return p2pbr->initfn(d);
+    }
+
+    return 0;
+}
+
+#define PCI_P2P_BRIDGE  "PCI P2P bridge"
+
+static PCIDeviceInfo pci_p2pbr_info = {
+    .qdev.name = PCI_P2P_BRIDGE,
+    .qdev.desc = "PCI PCI-to-PCI bridge",
+    .qdev.size = sizeof(PCIP2PBridge),
+    .qdev.reset = pci_bridge_reset,
+    .qdev.vmsd = &vmstate_pci_device,
+
+    .is_bridge = 1,
+    .init = pci_p2pbr_initfn,
+    .exit = pci_bridge_exitfn,
+    .config_write = pci_bridge_write_config,
+
+    .qdev.props = (Property[]) {
+        DEFINE_PROP_UINT16("vendor_id", PCIP2PBridge, vendor_id, 0),
+        DEFINE_PROP_UINT16("device_id", PCIP2PBridge, device_id, 0),
+        DEFINE_PROP_UINT8("revision_id", PCIP2PBridge, revision_id, 0),
+        DEFINE_PROP_UINT8("prog_interface", PCIP2PBridge, prog_interface, 0),
+
+        DEFINE_PROP_UINT8("ssvid_cap", PCIP2PBridge, ssvid_cap, 0),
+        DEFINE_PROP_UINT16("svid", PCIP2PBridge, svid, 0),
+        DEFINE_PROP_UINT16("ssid", PCIP2PBridge, ssid, 0),
+
+        DEFINE_PROP_END_OF_LIST(),
+    }
+};
+
+static void pci_p2pbr_register(void)
+{
+    pci_qdev_register(&pci_p2pbr_info);
+}
+
+device_init(pci_p2pbr_register);
+
+PCIBridge *pci_p2pbr_create(const PCIP2PBridgeInit *init)
+{
+    PCIDevice *d;
+    PCIBridge *br;
+    PCIP2PBridge *p2pbr;
+
+    d = pci_create_multifunction(init->bus, init->devfn, init->multifunction,
+                                 PCI_P2P_BRIDGE);
+
+    br = DO_UPCAST(PCIBridge, dev, d);
+    pci_bridge_map_irq(br, init->bus_name, init->map_irq);
+
+    p2pbr = DO_UPCAST(PCIP2PBridge, br, br);
+    p2pbr->initfn = init->initfn;
+
+    return br;
+}
+
+void pci_p2pbr_prop_set(PCIBridge *br, const PCIP2PBridgeProp *prop)
+{
+    DeviceState *qdev = &br->dev.qdev;
+
+    qdev_prop_set_uint16(qdev, "vendor_id", prop->vendor_id);
+    qdev_prop_set_uint16(qdev, "device_id", prop->device_id);
+    qdev_prop_set_uint8(qdev, "revision_id", prop->revision_id);
+    qdev_prop_set_uint8(qdev, "prog_interface", prop->prog_interface);
+
+    qdev_prop_set_uint8(qdev, "ssvid_cap", prop->ssvid_cap);
+    qdev_prop_set_uint16(qdev, "svid", prop->svid);
+    qdev_prop_set_uint16(qdev, "ssid", prop->ssid);
+}
+
+/* convenience function to create pci p2p bridge */
+PCIBridge *pci_p2pbr_create_simple(const PCIP2PBridgeInit *init,
+                                   const PCIP2PBridgeProp *prop)
+{
+    PCIBridge *br = pci_p2pbr_create(init);
+    pci_p2pbr_prop_set(br, prop);
+    qdev_init_nofail(&br->dev.qdev);
+    return br;
+}
diff --git a/hw/pci_p2pbr.h b/hw/pci_p2pbr.h
new file mode 100644
index 0000000..ee23ebb
--- /dev/null
+++ b/hw/pci_p2pbr.h
@@ -0,0 +1,61 @@ 
+/*
+ * QEMU PCI P2P generic bridge.
+ * In order to avoid create many P2P bridge device which only differs
+ * in vendor id/device id and so on.
+ *
+ * Copyright (c) 2011 Isaku Yamahata <yamahata at valinux co jp>
+ *                    VA Linux Systems Japan K.K.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef QEMU_PCI_P2PBR_H
+#define QEMU_PCI_P2PBR_H
+
+#include "qdev.h"
+
+typedef int (*pci_p2pbr_init_fn)(PCIDevice *d);
+
+typedef struct PCIP2PBridgeInit
+{
+    PCIBus *bus;
+    uint8_t devfn;
+    bool multifunction;
+
+    const char* bus_name;
+    pci_map_irq_fn map_irq;
+
+    pci_p2pbr_init_fn initfn;
+} PCIP2PBridgeInit;
+
+typedef struct PCIP2PBridgeProp
+{
+    uint16_t vendor_id;
+    uint16_t device_id;
+    uint8_t revision_id;
+    uint8_t prog_interface;
+
+    uint8_t ssvid_cap;
+    uint8_t svid;
+    uint8_t ssid;
+} PCIP2PBridgeProp;
+
+/* When setting PCIP2PBridgeProb, zero clear it for future compatibility */
+PCIBridge *pci_p2pbr_create(const PCIP2PBridgeInit *init);
+void pci_p2pbr_prop_set(PCIBridge *br, const PCIP2PBridgeProp *prop);
+
+PCIBridge *pci_p2pbr_create_simple(const PCIP2PBridgeInit *init,
+                                   const PCIP2PBridgeProp *prop);
+
+#endif /* QEMU_PCI_P2PBR_H */