Re: [Qemu-devel] [PATCH, RFC] pci: handle BAR mapping at pci level

[Blue Swirl <blauwirbel@gmail.com>]

On Wed, Jul 7, 2010 at 7:02 PM, Anthony Liguori <anthony@codemonkey.ws> wrote:
> 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 MSIX.
>>
>> Signed-off-by: Blue Swirl<blauwirbel@gmail.com>
>> ---
>> 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.
>>
>>  hw/ac97.c         |   60 +++++++++++---------
>>  hw/cirrus_vga.c   |   40 +++-----------
>>  hw/e1000.c        |   37 +-----------
>>  hw/eepro100.c     |   77 ++++++++++----------------
>>  hw/es1370.c       |   32 +++++------
>>  hw/ide/cmd646.c   |  149
>> +++++++++++++++++++++++++++++++-------------------
>>  hw/ide/piix.c     |   74 ++++++++++++++++---------
>>  hw/ide/via.c      |   67 ++++++++++++++--------
>>  hw/isa.h          |    1 +
>>  hw/isa_mmio.c     |   17 +++++-
>>  hw/lsi53c895a.c   |   60 ++++++--------------
>>  hw/macio.c        |  107 +++++++++++-------------------------
>>  hw/ne2000.c       |   66 +++++++++++++++-------
>>  hw/openpic.c      |   36 ++----------
>>  hw/pci.c          |  158
>> ++++++++++++++++++++++++++++++++--------------------
>>  hw/pci.h          |   18 +++++-
>>  hw/pcnet.c        |   62 ++++++++++-----------
>>  hw/ppc_mac.h      |    5 +-
>>  hw/ppc_newworld.c |    2 +-
>>  hw/ppc_oldworld.c |    4 +-
>>  hw/rtl8139.c      |   42 +++++---------
>>  hw/sun4u.c        |   29 +++------
>>  hw/usb-ohci.c     |   10 +---
>>  hw/usb-uhci.c     |   31 +++++-----
>>  hw/vga-pci.c      |   22 +------
>>  hw/virtio-pci.c   |   39 ++++++-------
>>  hw/vmware_vga.c   |  107 ++++++++++++++++++------------------
>>  hw/wdt_i6300esb.c |   38 +++++--------
>>  ioport.c          |  119 ++++++++++++++++++++++++++++++++++++----
>>  ioport.h          |    6 ++
>>  30 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 = {
>>      }
>>  };
>>
>> -static void ac97_map (PCIDevice *pci_dev, int region_num,
>> -                      pcibus_t addr, pcibus_t size, int type)
>> -{
>> -    AC97LinkState *s = DO_UPCAST (AC97LinkState, dev, pci_dev);
>> -    PCIDevice *d =&s->dev;
>> -
>> -    if (!region_num) {
>> -        s->base[0] = addr;
>> -        register_ioport_read (addr, 256 * 1, 1, nam_readb, d);
>> -        register_ioport_read (addr, 256 * 2, 2, nam_readw, d);
>> -        register_ioport_read (addr, 256 * 4, 4, nam_readl, d);
>> -        register_ioport_write (addr, 256 * 1, 1, nam_writeb, d);
>> -        register_ioport_write (addr, 256 * 2, 2, nam_writew, d);
>> -        register_ioport_write (addr, 256 * 4, 4, nam_writel, d);
>> -    }
>> -    else {
>> -        s->base[1] = addr;
>> -        register_ioport_read (addr, 64 * 1, 1, nabm_readb, d);
>> -        register_ioport_read (addr, 64 * 2, 2, nabm_readw, d);
>> -        register_ioport_read (addr, 64 * 4, 4, nabm_readl, d);
>> -        register_ioport_write (addr, 64 * 1, 1, nabm_writeb, d);
>> -        register_ioport_write (addr, 64 * 2, 2, nabm_writew, d);
>> -        register_ioport_write (addr, 64 * 4, 4, nabm_writel, d);
>> -    }
>> -}
>> +static IOPortWriteFunc * const nam_writes[] = {
>> +    nam_writeb,
>> +    nam_writew,
>> +    nam_writel,
>> +};
>> +
>> +static IOPortReadFunc * const nam_reads[] = {
>> +    nam_readb,
>> +    nam_readw,
>> +    nam_readl,
>> +};
>> +
>> +static IOPortWriteFunc * const nabm_writes[] = {
>> +    nabm_writeb,
>> +    nabm_writew,
>> +    nabm_writel,
>> +};
>> +
>> +static IOPortReadFunc * const nabm_reads[] = {
>> +    nabm_readb,
>> +    nabm_readw,
>> +    nabm_readl,
>> +};
>>
>>  static void ac97_on_reset (void *opaque)
>>  {
>> @@ -1280,6 +1278,7 @@ static int ac97_initfn (PCIDevice *dev)
>>  {
>>      AC97LinkState *s = DO_UPCAST (AC97LinkState, dev, dev);
>>      uint8_t *c = s->dev.config;
>> +    int io_index;
>>
>>      pci_config_set_vendor_id (c, PCI_VENDOR_ID_INTEL); /* ro */
>>      pci_config_set_device_id (c, PCI_DEVICE_ID_INTEL_82801AA_5); /* ro */
>> @@ -1321,9 +1320,14 @@ static int ac97_initfn (PCIDevice *dev)
>>      /* TODO: RST# value should be 0. */
>>      c[PCI_INTERRUPT_PIN] = 0x01;      /* intr_pn interrupt pin ro */
>>
>> -    pci_register_bar (&s->dev, 0, 256 * 4, PCI_BASE_ADDRESS_SPACE_IO,
>> -                      ac97_map);
>> -    pci_register_bar (&s->dev, 1, 64 * 4, PCI_BASE_ADDRESS_SPACE_IO,
>> ac97_map);
>> +    pci_register_bar(&s->dev, 0, 256 * 4, PCI_BASE_ADDRESS_SPACE_IO);
>> +    io_index = cpu_register_io(nam_reads, nam_writes, 256 * 4, s);
>> +    pci_bar_map(&s->dev, 0, 0, 0, 256 * 4, io_index);
>> +
>> +    pci_register_bar(&s->dev, 1, 64 * 4, PCI_BASE_ADDRESS_SPACE_IO);
>> +    io_index = cpu_register_io(nabm_reads, nabm_writes, 64 * 4, s);
>> +    pci_bar_map(&s->dev, 1, 0, 0, 64 * 4, io_index);
>>
>
> Any reason to keep this a three step process?  I see no reason not to 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 CPU
> memory/pio callbacks.

The CPU callbacks may be useful outside of PCI code, like ISA.