From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=48506 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1OWaQZ-0001w4-Eo for qemu-devel@nongnu.org; Wed, 07 Jul 2010 15:37:06 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1OWaQ1-0001UC-BL for qemu-devel@nongnu.org; Wed, 07 Jul 2010 15:36:30 -0400 Received: from mail-pw0-f45.google.com ([209.85.160.45]:48211) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1OWaQ1-0001TX-3X for qemu-devel@nongnu.org; Wed, 07 Jul 2010 15:36:29 -0400 Received: by pwi2 with SMTP id 2so2288pwi.4 for ; Wed, 07 Jul 2010 12:36:25 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <4C34CF5D.3070903@codemonkey.ws> References: <4C34CF5D.3070903@codemonkey.ws> From: Blue Swirl Date: Wed, 7 Jul 2010 19:36:04 +0000 Message-ID: Subject: Re: [Qemu-devel] [PATCH, RFC] pci: handle BAR mapping at pci level 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: Anthony Liguori Cc: qemu-devel , "Michael S. Tsirkin" On Wed, Jul 7, 2010 at 7:02 PM, Anthony Liguori wro= te: > On 07/07/2010 12:53 PM, Blue Swirl wrote: >> >> Add I/O port registration functions which separate registration >> from the mapping stage. >> >> Move IOIO and MMIO BAR mapping to pci.c. >> >> TODO: fix dirty logging, coalesced MMIO and base address comparisons >> (eepro100 etc). Bridge filtering may be broken. Broke virtio-pci and MSI= X. >> >> Signed-off-by: Blue Swirl >> --- >> i386 boots but resets. PPC and Sparc64 can't even start. >> >> Patch also available at >> git://repo.or.cz/qemu/blueswirl.git >> >> It may be worthwhile to break this into some kind of smaller steps. >> >> =C2=A0hw/ac97.c =C2=A0 =C2=A0 =C2=A0 =C2=A0 | =C2=A0 60 +++++++++++-----= ---- >> =C2=A0hw/cirrus_vga.c =C2=A0 | =C2=A0 40 +++----------- >> =C2=A0hw/e1000.c =C2=A0 =C2=A0 =C2=A0 =C2=A0| =C2=A0 37 +----------- >> =C2=A0hw/eepro100.c =C2=A0 =C2=A0 | =C2=A0 77 ++++++++++---------------- >> =C2=A0hw/es1370.c =C2=A0 =C2=A0 =C2=A0 | =C2=A0 32 +++++------ >> =C2=A0hw/ide/cmd646.c =C2=A0 | =C2=A0149 >> +++++++++++++++++++++++++++++++------------------- >> =C2=A0hw/ide/piix.c =C2=A0 =C2=A0 | =C2=A0 74 ++++++++++++++++--------- >> =C2=A0hw/ide/via.c =C2=A0 =C2=A0 =C2=A0| =C2=A0 67 ++++++++++++++-------= - >> =C2=A0hw/isa.h =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0| =C2=A0 =C2=A01 + >> =C2=A0hw/isa_mmio.c =C2=A0 =C2=A0 | =C2=A0 17 +++++- >> =C2=A0hw/lsi53c895a.c =C2=A0 | =C2=A0 60 ++++++-------------- >> =C2=A0hw/macio.c =C2=A0 =C2=A0 =C2=A0 =C2=A0| =C2=A0107 +++++++++++-----= -------------------- >> =C2=A0hw/ne2000.c =C2=A0 =C2=A0 =C2=A0 | =C2=A0 66 +++++++++++++++------= - >> =C2=A0hw/openpic.c =C2=A0 =C2=A0 =C2=A0| =C2=A0 36 ++---------- >> =C2=A0hw/pci.c =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0| =C2=A0158 >> ++++++++++++++++++++++++++++++++-------------------- >> =C2=A0hw/pci.h =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0| =C2=A0 18 +++++- >> =C2=A0hw/pcnet.c =C2=A0 =C2=A0 =C2=A0 =C2=A0| =C2=A0 62 ++++++++++------= ----- >> =C2=A0hw/ppc_mac.h =C2=A0 =C2=A0 =C2=A0| =C2=A0 =C2=A05 +- >> =C2=A0hw/ppc_newworld.c | =C2=A0 =C2=A02 +- >> =C2=A0hw/ppc_oldworld.c | =C2=A0 =C2=A04 +- >> =C2=A0hw/rtl8139.c =C2=A0 =C2=A0 =C2=A0| =C2=A0 42 +++++--------- >> =C2=A0hw/sun4u.c =C2=A0 =C2=A0 =C2=A0 =C2=A0| =C2=A0 29 +++------ >> =C2=A0hw/usb-ohci.c =C2=A0 =C2=A0 | =C2=A0 10 +--- >> =C2=A0hw/usb-uhci.c =C2=A0 =C2=A0 | =C2=A0 31 +++++----- >> =C2=A0hw/vga-pci.c =C2=A0 =C2=A0 =C2=A0| =C2=A0 22 +------ >> =C2=A0hw/virtio-pci.c =C2=A0 | =C2=A0 39 ++++++------- >> =C2=A0hw/vmware_vga.c =C2=A0 | =C2=A0107 ++++++++++++++++++-------------= ----- >> =C2=A0hw/wdt_i6300esb.c | =C2=A0 38 +++++-------- >> =C2=A0ioport.c =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0| =C2=A0119 +++++++++++= +++++++++++++++++++++++++---- >> =C2=A0ioport.h =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0| =C2=A0 =C2=A06 ++ >> =C2=A030 files changed, 778 insertions(+), 737 deletions(-) >> >> diff --git a/hw/ac97.c b/hw/ac97.c >> index 4319bc8..28d0c19 100644 >> --- a/hw/ac97.c >> +++ b/hw/ac97.c >> @@ -1234,31 +1234,29 @@ static const VMStateDescription vmstate_ac97 =3D= { >> =C2=A0 =C2=A0 =C2=A0} >> =C2=A0}; >> >> -static void ac97_map (PCIDevice *pci_dev, int region_num, >> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0pcibus_t addr, pcibus_t size, int type) >> -{ >> - =C2=A0 =C2=A0AC97LinkState *s =3D DO_UPCAST (AC97LinkState, dev, pci_d= ev); >> - =C2=A0 =C2=A0PCIDevice *d =3D&s->dev; >> - >> - =C2=A0 =C2=A0if (!region_num) { >> - =C2=A0 =C2=A0 =C2=A0 =C2=A0s->base[0] =3D addr; >> - =C2=A0 =C2=A0 =C2=A0 =C2=A0register_ioport_read (addr, 256 * 1, 1, nam= _readb, d); >> - =C2=A0 =C2=A0 =C2=A0 =C2=A0register_ioport_read (addr, 256 * 2, 2, nam= _readw, d); >> - =C2=A0 =C2=A0 =C2=A0 =C2=A0register_ioport_read (addr, 256 * 4, 4, nam= _readl, d); >> - =C2=A0 =C2=A0 =C2=A0 =C2=A0register_ioport_write (addr, 256 * 1, 1, na= m_writeb, d); >> - =C2=A0 =C2=A0 =C2=A0 =C2=A0register_ioport_write (addr, 256 * 2, 2, na= m_writew, d); >> - =C2=A0 =C2=A0 =C2=A0 =C2=A0register_ioport_write (addr, 256 * 4, 4, na= m_writel, d); >> - =C2=A0 =C2=A0} >> - =C2=A0 =C2=A0else { >> - =C2=A0 =C2=A0 =C2=A0 =C2=A0s->base[1] =3D addr; >> - =C2=A0 =C2=A0 =C2=A0 =C2=A0register_ioport_read (addr, 64 * 1, 1, nabm= _readb, d); >> - =C2=A0 =C2=A0 =C2=A0 =C2=A0register_ioport_read (addr, 64 * 2, 2, nabm= _readw, d); >> - =C2=A0 =C2=A0 =C2=A0 =C2=A0register_ioport_read (addr, 64 * 4, 4, nabm= _readl, d); >> - =C2=A0 =C2=A0 =C2=A0 =C2=A0register_ioport_write (addr, 64 * 1, 1, nab= m_writeb, d); >> - =C2=A0 =C2=A0 =C2=A0 =C2=A0register_ioport_write (addr, 64 * 2, 2, nab= m_writew, d); >> - =C2=A0 =C2=A0 =C2=A0 =C2=A0register_ioport_write (addr, 64 * 4, 4, nab= m_writel, d); >> - =C2=A0 =C2=A0} >> -} >> +static IOPortWriteFunc * const nam_writes[] =3D { >> + =C2=A0 =C2=A0nam_writeb, >> + =C2=A0 =C2=A0nam_writew, >> + =C2=A0 =C2=A0nam_writel, >> +}; >> + >> +static IOPortReadFunc * const nam_reads[] =3D { >> + =C2=A0 =C2=A0nam_readb, >> + =C2=A0 =C2=A0nam_readw, >> + =C2=A0 =C2=A0nam_readl, >> +}; >> + >> +static IOPortWriteFunc * const nabm_writes[] =3D { >> + =C2=A0 =C2=A0nabm_writeb, >> + =C2=A0 =C2=A0nabm_writew, >> + =C2=A0 =C2=A0nabm_writel, >> +}; >> + >> +static IOPortReadFunc * const nabm_reads[] =3D { >> + =C2=A0 =C2=A0nabm_readb, >> + =C2=A0 =C2=A0nabm_readw, >> + =C2=A0 =C2=A0nabm_readl, >> +}; >> >> =C2=A0static void ac97_on_reset (void *opaque) >> =C2=A0{ >> @@ -1280,6 +1278,7 @@ static int ac97_initfn (PCIDevice *dev) >> =C2=A0{ >> =C2=A0 =C2=A0 =C2=A0AC97LinkState *s =3D DO_UPCAST (AC97LinkState, dev, = dev); >> =C2=A0 =C2=A0 =C2=A0uint8_t *c =3D s->dev.config; >> + =C2=A0 =C2=A0int io_index; >> >> =C2=A0 =C2=A0 =C2=A0pci_config_set_vendor_id (c, PCI_VENDOR_ID_INTEL); /= * ro */ >> =C2=A0 =C2=A0 =C2=A0pci_config_set_device_id (c, PCI_DEVICE_ID_INTEL_828= 01AA_5); /* ro */ >> @@ -1321,9 +1320,14 @@ static int ac97_initfn (PCIDevice *dev) >> =C2=A0 =C2=A0 =C2=A0/* TODO: RST# value should be 0. */ >> =C2=A0 =C2=A0 =C2=A0c[PCI_INTERRUPT_PIN] =3D 0x01; =C2=A0 =C2=A0 =C2=A0/= * intr_pn interrupt pin ro */ >> >> - =C2=A0 =C2=A0pci_register_bar (&s->dev, 0, 256 * 4, PCI_BASE_ADDRESS_S= PACE_IO, >> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0ac97_map); >> - =C2=A0 =C2=A0pci_register_bar (&s->dev, 1, 64 * 4, PCI_BASE_ADDRESS_SP= ACE_IO, >> ac97_map); >> + =C2=A0 =C2=A0pci_register_bar(&s->dev, 0, 256 * 4, PCI_BASE_ADDRESS_SP= ACE_IO); >> + =C2=A0 =C2=A0io_index =3D cpu_register_io(nam_reads, nam_writes, 256 *= 4, s); >> + =C2=A0 =C2=A0pci_bar_map(&s->dev, 0, 0, 0, 256 * 4, io_index); >> + >> + =C2=A0 =C2=A0pci_register_bar(&s->dev, 1, 64 * 4, PCI_BASE_ADDRESS_SPA= CE_IO); >> + =C2=A0 =C2=A0io_index =3D cpu_register_io(nabm_reads, nabm_writes, 64 = * 4, s); >> + =C2=A0 =C2=A0pci_bar_map(&s->dev, 1, 0, 0, 64 * 4, io_index); >> > > Any reason to keep this a three step process? =C2=A0I see no reason not t= o unify > the io and mem function pointers into a single type. I was also considering this, but the function signatures don't match now. With a lot more effort (or smaller steps) it could be doable. > These callbacks should > be working off of pci bus addresses and should not be tied directly to CP= U > memory/pio callbacks. The CPU callbacks may be useful outside of PCI code, like ISA.