From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=51036 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Ot2oG-0000EW-GG for qemu-devel@nongnu.org; Tue, 07 Sep 2010 14:22:21 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1Ot2oF-0006Cs-5j for qemu-devel@nongnu.org; Tue, 07 Sep 2010 14:22:20 -0400 Received: from mail-qy0-f180.google.com ([209.85.216.180]:37893) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1Ot2oE-0006Cn-WE for qemu-devel@nongnu.org; Tue, 07 Sep 2010 14:22:19 -0400 Received: by qyk31 with SMTP id 31so6009887qyk.4 for ; Tue, 07 Sep 2010 11:22:18 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <1283860398-11637-3-git-send-email-agraf@suse.de> References: <1283860398-11637-1-git-send-email-agraf@suse.de> <1283860398-11637-3-git-send-email-agraf@suse.de> From: Blue Swirl Date: Tue, 7 Sep 2010 18:21:58 +0000 Message-ID: Subject: Re: [Qemu-devel] [PATCH 2/4] PPC: Qdev'ify e500 pci Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Graf Cc: QEMU Developers On Tue, Sep 7, 2010 at 11:53 AM, Alexander Graf wrote: > The e500 PCI controller isn't qdev'ified yet. This leads to severe issues > when running with -drive. > > To be able to use a virtio disk with an e500 VM, let's convert the PCI > controller over to qdev. > > Signed-off-by: Alexander Graf > --- > =C2=A0hw/ppce500_pci.c | =C2=A0106 +++++++++++++++++++++++++++++++++++++-= ---------------- > =C2=A01 files changed, 73 insertions(+), 33 deletions(-) > > diff --git a/hw/ppce500_pci.c b/hw/ppce500_pci.c > index 8ac99f2..3fa42d2 100644 > --- a/hw/ppce500_pci.c > +++ b/hw/ppce500_pci.c > @@ -73,11 +73,11 @@ struct pci_inbound { > =C2=A0}; > > =C2=A0struct PPCE500PCIState { > + =C2=A0 =C2=A0PCIHostState pci_state; > =C2=A0 =C2=A0 struct pci_outbound pob[PPCE500_PCI_NR_POBS]; > =C2=A0 =C2=A0 struct pci_inbound pib[PPCE500_PCI_NR_PIBS]; > =C2=A0 =C2=A0 uint32_t gasket_time; > - =C2=A0 =C2=A0PCIHostState pci_state; > - =C2=A0 =C2=A0PCIDevice *pci_dev; > + =C2=A0 =C2=A0uint64_t base_addr; > =C2=A0}; > > =C2=A0typedef struct PPCE500PCIState PPCE500PCIState; > @@ -221,7 +221,7 @@ static void ppce500_pci_save(QEMUFile *f, void *opaqu= e) > =C2=A0 =C2=A0 PPCE500PCIState *controller =3D opaque; > =C2=A0 =C2=A0 int i; > > - =C2=A0 =C2=A0pci_device_save(controller->pci_dev, f); > + =C2=A0 =C2=A0/* pci_device_save(controller->pci_dev, f); */ Why, is loading/saving broken? > > =C2=A0 =C2=A0 for (i =3D 0; i < PPCE500_PCI_NR_POBS; i++) { > =C2=A0 =C2=A0 =C2=A0 =C2=A0 qemu_put_be32s(f, &controller->pob[i].potar); > @@ -247,7 +247,7 @@ static int ppce500_pci_load(QEMUFile *f, void *opaque= , int version_id) > =C2=A0 =C2=A0 if (version_id !=3D 1) > =C2=A0 =C2=A0 =C2=A0 =C2=A0 return -EINVAL; > > - =C2=A0 =C2=A0pci_device_load(controller->pci_dev, f); > + =C2=A0 =C2=A0/* pci_device_load(controller->pci_dev, f); */ > > =C2=A0 =C2=A0 for (i =3D 0; i < PPCE500_PCI_NR_POBS; i++) { > =C2=A0 =C2=A0 =C2=A0 =C2=A0 qemu_get_be32s(f, &controller->pob[i].potar); > @@ -269,55 +269,95 @@ static int ppce500_pci_load(QEMUFile *f, void *opaq= ue, int version_id) > > =C2=A0PCIBus *ppce500_pci_init(qemu_irq pci_irqs[4], target_phys_addr_t r= egisters) > =C2=A0{ > - =C2=A0 =C2=A0PPCE500PCIState *controller; > + =C2=A0 =C2=A0DeviceState *dev; > + =C2=A0 =C2=A0PCIBus *b; > + =C2=A0 =C2=A0PCIHostState *h; > + =C2=A0 =C2=A0PPCE500PCIState *s; > =C2=A0 =C2=A0 PCIDevice *d; > - =C2=A0 =C2=A0int index; > =C2=A0 =C2=A0 static int ppce500_pci_id; > > - =C2=A0 =C2=A0controller =3D qemu_mallocz(sizeof(PPCE500PCIState)); > + =C2=A0 =C2=A0dev =3D qdev_create(NULL, "e500-pcihost"); > + =C2=A0 =C2=A0h =3D FROM_SYSBUS(PCIHostState, sysbus_from_qdev(dev)); > + =C2=A0 =C2=A0s =3D DO_UPCAST(PPCE500PCIState, pci_state, h); > + > + =C2=A0 =C2=A0qdev_prop_set_uint64(dev, "base_addr", registers); This property should not be needed. You should simply use sysbus_mmio_map() here. See for example grackle_pci.c. > + =C2=A0 =C2=A0b =3D pci_register_bus(&s->pci_state.busdev.qdev, NULL, mp= c85xx_pci_set_irq, > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 mpc85xx_pci_map_irq, pci_irqs, PCI_DEVFN(0x11, 0), 4); > + > + =C2=A0 =C2=A0s->pci_state.bus =3D b; > + =C2=A0 =C2=A0qdev_init_nofail(dev); > + =C2=A0 =C2=A0d =3D pci_create_simple(b, 0, "e500-host-bridge"); > + > + =C2=A0 =C2=A0/* XXX load/save code not tested. */ > + =C2=A0 =C2=A0register_savevm(&d->qdev, "ppce500_pci", ppce500_pci_id++, > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A01,= ppce500_pci_save, ppce500_pci_load, s); It would be nice if you also converted the device to VMState and vmsd. A reset function would be cool too, if it's needed after Anthony's reset changes. > > - =C2=A0 =C2=A0controller->pci_state.bus =3D pci_register_bus(NULL, "pci"= , > - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 mpc85xx_pci_set_irq, > - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 mpc85xx_pci_map_irq, > - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 pci_irqs, PCI_DEVFN(0x11, 0), > - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 4); > - =C2=A0 =C2=A0d =3D pci_register_device(controller->pci_state.bus, > - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0"host bridge", sizeof(PCIDevice), > - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A00, NULL, NULL); > + =C2=A0 =C2=A0return b; > +} > > - =C2=A0 =C2=A0pci_config_set_vendor_id(d->config, PCI_VENDOR_ID_FREESCAL= E); > - =C2=A0 =C2=A0pci_config_set_device_id(d->config, PCI_DEVICE_ID_MPC8533E= ); > - =C2=A0 =C2=A0pci_config_set_class(d->config, PCI_CLASS_PROCESSOR_POWERP= C); > +static int e500_pcihost_initfn(SysBusDevice *dev) > +{ > + =C2=A0 =C2=A0PCIHostState *h; > + =C2=A0 =C2=A0PPCE500PCIState *s; > + =C2=A0 =C2=A0target_phys_addr_t registers; > + =C2=A0 =C2=A0int index; > > - =C2=A0 =C2=A0controller->pci_dev =3D d; > + =C2=A0 =C2=A0h =3D FROM_SYSBUS(PCIHostState, sysbus_from_qdev(dev)); > + =C2=A0 =C2=A0s =3D DO_UPCAST(PPCE500PCIState, pci_state, h); > + =C2=A0 =C2=A0registers =3D (target_phys_addr_t)s->base_addr; > > =C2=A0 =C2=A0 /* CFGADDR */ > - =C2=A0 =C2=A0index =3D pci_host_conf_register_mmio(&controller->pci_sta= te, 0); > + =C2=A0 =C2=A0index =3D pci_host_conf_register_mmio(&s->pci_state, 0); > =C2=A0 =C2=A0 if (index < 0) > - =C2=A0 =C2=A0 =C2=A0 =C2=A0goto free; > + =C2=A0 =C2=A0 =C2=A0 =C2=A0return -1; > =C2=A0 =C2=A0 cpu_register_physical_memory(registers + PCIE500_CFGADDR, 4= , index); Instead of cpu_register_physical_memory(), you should use sysbus_register_mmio(). > > =C2=A0 =C2=A0 /* CFGDATA */ > - =C2=A0 =C2=A0index =3D pci_host_data_register_mmio(&controller->pci_sta= te, 0); > + =C2=A0 =C2=A0index =3D pci_host_data_register_mmio(&s->pci_state, 0); > =C2=A0 =C2=A0 if (index < 0) > - =C2=A0 =C2=A0 =C2=A0 =C2=A0goto free; > + =C2=A0 =C2=A0 =C2=A0 =C2=A0return -1; > =C2=A0 =C2=A0 cpu_register_physical_memory(registers + PCIE500_CFGDATA, 4= , index); > > =C2=A0 =C2=A0 index =3D cpu_register_io_memory(e500_pci_reg_read, > - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 e500_pci_reg_write, contro= ller); > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 e500_pci_reg_write, s); > =C2=A0 =C2=A0 if (index < 0) > - =C2=A0 =C2=A0 =C2=A0 =C2=A0goto free; > + =C2=A0 =C2=A0 =C2=A0 =C2=A0return -1; > =C2=A0 =C2=A0 cpu_register_physical_memory(registers + PCIE500_REG_BASE, > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0PCIE500_REG_SIZE, index= ); > + =C2=A0 =C2=A0return 0; > +} > > - =C2=A0 =C2=A0/* XXX load/save code not tested. */ > - =C2=A0 =C2=A0register_savevm(&d->qdev, "ppce500_pci", ppce500_pci_id++, > - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A01,= ppce500_pci_save, ppce500_pci_load, controller); > +static int e500_host_bridge_initfn(PCIDevice *dev) > +{ > + =C2=A0 =C2=A0pci_config_set_vendor_id(dev->config, PCI_VENDOR_ID_FREESC= ALE); > + =C2=A0 =C2=A0pci_config_set_device_id(dev->config, PCI_DEVICE_ID_MPC853= 3E); > + =C2=A0 =C2=A0pci_config_set_class(dev->config, PCI_CLASS_PROCESSOR_POWE= RPC); > + > + =C2=A0 =C2=A0return 0; > +} > + > +static PCIDeviceInfo e500_host_bridge_info =3D { > + =C2=A0 =C2=A0.qdev.name =C2=A0 =C2=A0=3D "e500-host-bridge", > + =C2=A0 =C2=A0.qdev.desc =C2=A0 =C2=A0=3D "Host bridge", > + =C2=A0 =C2=A0.qdev.size =C2=A0 =C2=A0=3D sizeof(PCIDevice), > + =C2=A0 =C2=A0.qdev.no_user =3D 1, > + =C2=A0 =C2=A0.init =C2=A0 =C2=A0 =C2=A0 =C2=A0 =3D e500_host_bridge_ini= tfn, > +}; > > - =C2=A0 =C2=A0return controller->pci_state.bus; > +static SysBusDeviceInfo e500_pcihost_info =3D { > + =C2=A0 =C2=A0.init =C2=A0 =C2=A0 =C2=A0 =C2=A0 =3D e500_pcihost_initfn, > + =C2=A0 =C2=A0.qdev.name =C2=A0 =C2=A0=3D "e500-pcihost", > + =C2=A0 =C2=A0.qdev.size =C2=A0 =C2=A0=3D sizeof(PPCE500PCIState), > + =C2=A0 =C2=A0.qdev.no_user =3D 1, > + =C2=A0 =C2=A0.qdev.props =3D (Property[]) { > + =C2=A0 =C2=A0 =C2=A0 =C2=A0DEFINE_PROP_UINT64("base_addr", PPCE500PCISt= ate, base_addr, 0), > + =C2=A0 =C2=A0 =C2=A0 =C2=A0DEFINE_PROP_END_OF_LIST(), > + =C2=A0 =C2=A0} > +}; > > -free: > - =C2=A0 =C2=A0printf("%s error\n", __func__); > - =C2=A0 =C2=A0qemu_free(controller); > - =C2=A0 =C2=A0return NULL; > +static void e500_pci_register(void) > +{ > + =C2=A0 =C2=A0sysbus_register_withprop(&e500_pcihost_info); > + =C2=A0 =C2=A0pci_qdev_register(&e500_host_bridge_info); > =C2=A0} > +device_init(e500_pci_register); > -- > 1.6.0.2 > > >