[22/23] pci: initialize wmask according to pci header type.

- initialize wmask according to pci header type.
  So far, header of type 01  was not correctly initialized.
- only sets default subsystem id for header type 00.
  header type 01 doesn't have subsystem id, and uses the register
  for other purpose. So setting default subsystem id doesn't make sense.

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
 hw/cirrus_vga.c |    2 +-
 hw/pci.c        |   98 +++++++++++++++++++++++++++++++++++++++++++++++++-----
 hw/pci.h        |   36 ++++++++++++++++++++
 3 files changed, 126 insertions(+), 10 deletions(-)

On Mon, Oct 05, 2009 at 07:07:02PM +0900, Isaku Yamahata wrote:
> - initialize wmask according to pci header type.
>   So far, header of type 01  was not correctly initialized.

I think it was correct. Which register has incorrect value?

It works because BAR for regular device is at the same location as
base/limit for bridge.  value at which offset was wrong.  Since no one
uses bridges and everyone uses regular devices, there's value in using
common code. So let's just add a comment explaing why code works, and be
done with it.

> - only sets default subsystem id for header type 00.
>   header type 01 doesn't have subsystem id, and uses the register
>   for other purpose. So setting default subsystem id doesn't make sense.

But then it's easier to reuse regular device code and override the value
to 0 in bridge after the fact than fork a ton of code.

> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> ---
>  hw/cirrus_vga.c |    2 +-
>  hw/pci.c        |   98 +++++++++++++++++++++++++++++++++++++++++++++++++-----
>  hw/pci.h        |   36 ++++++++++++++++++++
>  3 files changed, 126 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c
> index 2a6aba8..55ef4c1 100644
> --- a/hw/cirrus_vga.c
> +++ b/hw/cirrus_vga.c
> @@ -180,7 +180,7 @@
>  #define PCI_COMMAND_PALETTESNOOPING         0x0020
>  #define PCI_COMMAND_PARITYDETECTION         0x0040
>  #define PCI_COMMAND_ADDRESSDATASTEPPING     0x0080
> -#define PCI_COMMAND_SERR                    0x0100
> +// #define PCI_COMMAND_SERR                    0x0100 /* duplicated */

so remove it?

>  #define PCI_COMMAND_BACKTOBACKTRANS         0x0200
>  // PCI 0x08, 0xff000000 (0x09-0x0b:class,0x08:rev)
>  #define PCI_CLASS_BASE_DISPLAY        0x03
> diff --git a/hw/pci.c b/hw/pci.c
> index fb93b99..2694e83 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -381,16 +381,32 @@ static void pci_init_cmask(PCIDevice *dev)
>      dev->cmask[PCI_CAPABILITY_LIST] = 0xff;
>  }
>  
> -static void pci_init_wmask(PCIDevice *dev)
> +static void pci_conf_init_type00(PCIDevice *dev)

I think it was a good idea to split wmask init from conf init. Small
functions doing one thing and doing it well. Let's not merge them.

>  {
>      int i;
>      uint32_t config_size = pcie_config_size(dev);
>  
> +    pci_set_default_subsystem_id(dev);
> +
> +    pci_set_word(&dev->wmask[PCI_COMMAND],

dev->wmask + COMMAND

> +                 PCI_COMMAND_IO |
> +                 PCI_COMMAND_MEMORY |
> +                 PCI_COMMAND_MASTER |
> +                 PCI_COMMAND_SPECIAL |
> +                 PCI_COMMAND_INVALIDATE |
> +                 PCI_COMMAND_VGA_PALETTE |
> +                 PCI_COMMAND_PARITY |
> +                 PCI_COMMAND_WAIT |
> +                 PCI_COMMAND_SERR |
> +                 PCI_COMMAND_FAST_BACK |
> +                 PCI_COMMAND_INTX_DISABLE);
> +
>      dev->wmask[PCI_CACHE_LINE_SIZE] = 0xff;
> +    dev->wmask[PCI_LATENCY_TIMER] = 0xff;
>      dev->wmask[PCI_INTERRUPT_LINE] = 0xff;
> -    dev->wmask[PCI_COMMAND] = PCI_COMMAND_IO | PCI_COMMAND_MEMORY
> -                              | PCI_COMMAND_MASTER;
> -    for (i = PCI_CONFIG_HEADER_SIZE; i < config_size; ++i)
> +
> +    /* device dependent part */
> +    for (i = PCI_CONFIG_HEADER_SIZE; i < config_size; i++)
>          dev->wmask[i] = 0xff;
>  }
>  
> @@ -410,10 +426,12 @@ static void pci_config_alloc(PCIDevice *pci_dev)
>  }
>  
>  /* -1 for devfn means auto assign */
> +typedef void (*pci_conf_init_t)(PCIDevice *d);
>  static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
>                                           const char *name, int devfn,
>                                           PCIConfigReadFunc *config_read,
> -                                         PCIConfigWriteFunc *config_write)
> +                                         PCIConfigWriteFunc *config_write,
> +                                         pci_conf_init_t conf_init)
>  {
>      if (devfn < 0) {
>          for(devfn = bus->devfn_min ; devfn < 256; devfn += 8) {
> @@ -430,9 +448,8 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
>      pstrcpy(pci_dev->name, sizeof(pci_dev->name), name);
>      memset(pci_dev->irq_state, 0, sizeof(pci_dev->irq_state));
>      pci_config_alloc(pci_dev);
> -    pci_set_default_subsystem_id(pci_dev);
>      pci_init_cmask(pci_dev);
> -    pci_init_wmask(pci_dev);
> +    conf_init(pci_dev);

Oh no, not another callback.
Let users just customize it *after* device is registered.

>  
>      if (!config_read)
>          config_read = pci_default_read_config;
> @@ -455,9 +472,11 @@ PCIDevice *pci_register_device(PCIBus *bus, const char *name,
>  
>      pci_dev = qemu_mallocz(instance_size);
>      pci_dev = do_pci_register_device(pci_dev, bus, name, devfn,
> -                                     config_read, config_write);
> +                                     config_read, config_write,
> +                                     pci_conf_init_type00);
>      return pci_dev;
>  }
> +
>  static target_phys_addr_t pci_to_cpu_addr(target_phys_addr_t addr)
>  {
>      return addr + pci_mem_base;
> @@ -1194,6 +1213,59 @@ PCIDevice *pci_find_device(PCIBus *bus, int bus_num, int slot, int function)
>      return bus->devices[PCI_DEVFN(slot, function)];
>  }
>  
> +static void pci_conf_init_type01(PCIDevice *d)
> +{
> +    uint32_t addr;
> +    uint32_t config_size = pcie_config_size(d);
> +
> +    pci_set_word(&d->wmask[PCI_COMMAND],
> +                 PCI_COMMAND_IO |
> +                 PCI_COMMAND_MEMORY |
> +                 PCI_COMMAND_MASTER |
> +                 PCI_COMMAND_SPECIAL |
> +                 PCI_COMMAND_INVALIDATE |
> +                 PCI_COMMAND_VGA_PALETTE |
> +                 PCI_COMMAND_PARITY |
> +                 PCI_COMMAND_WAIT |
> +                 PCI_COMMAND_SERR |
> +                 PCI_COMMAND_FAST_BACK |
> +                 PCI_COMMAND_INTX_DISABLE);
> +
> +    d->wmask[PCI_CACHE_LINE_SIZE] = 0xff;
> +    d->wmask[PCI_LATENCY_TIMER] = 0xff;
> +
> +    d->wmask[PCI_PRIMARY_BUS] = 0xff;
> +    d->wmask[PCI_SECONDARY_BUS] = 0xff;
> +    d->wmask[PCI_SUBORDINATE_BUS] = 0xff;
> +    d->wmask[PCI_SEC_LATENCY_TIMER] = 0xff;
> +    d->wmask[PCI_IO_BASE] = PCI_IO_RANGE_MASK & 0xff;
> +    d->wmask[PCI_IO_LIMIT] = PCI_IO_RANGE_MASK & 0xff;
> +
> +    /* sec status isn't emulated (yet) */
> +    d->wmask[PCI_SEC_STATUS] = 0;
> +
> +    pci_set_word(&d->wmask[PCI_MEMORY_BASE], PCI_MEMORY_RANGE_MASK & 0xffff);

d->wmask + PCI_MEMORY_BASE, same elsewhere.
Especially because you don't address a single byte with pci_set_word,
so you really pass it pointer into array, not pointer to specific field
which just happens to be inside array.

> +    pci_set_word(&d->wmask[PCI_MEMORY_LIMIT], PCI_MEMORY_RANGE_MASK & 0xffff);
> +    pci_set_word(&d->wmask[PCI_PREF_MEMORY_BASE], PCI_PREF_RANGE_MASK & 0xffff);
> +    pci_set_word(&d->wmask[PCI_PREF_MEMORY_LIMIT], PCI_PREF_RANGE_MASK & 0xffff);
> +    pci_set_long(&d->wmask[PCI_PREF_BASE_UPPER32], 0xffffffff);
> +    pci_set_long(&d->wmask[PCI_PREF_LIMIT_UPPER32], 0xffffffff);


This is a lot of code, but the result is that a ton of registers
are filled with all ones. Let's just memset a range to 0xff.
When you do it this way, you will see that code is basically
identical to regular devices.

> +
> +    d->wmask[PCI_INTERRUPT_LINE] = 0xff;
> +    pci_set_word(&d->wmask[PCI_BRIDGE_CONTROL], 0xffff);
> +
> +    /* device dependent part */
> +    for (addr = PCI_CONFIG_HEADER_SIZE; addr < config_size; addr++)
> +        d->wmask[addr] = 0xff;
> +}
> +
> +static void pci_conf_init_type02(PCIDevice *d)
> +{
> +    fprintf(stderr,
> +            "ERROR: pci header type 02(CARDBUS) isn't supported yet.\n");
> +    abort();
> +}
> +

so why write it? type 0x3 isn't supported either.

>  int pci_bridge_initfn(PCIDevice *pci_dev)
>  {
>      uint8_t *pci_conf = pci_dev->config;
> @@ -1263,11 +1335,19 @@ static int pci_qdev_init(DeviceState *qdev, DeviceInfo *base)
>      PCIDeviceInfo *info = container_of(base, PCIDeviceInfo, qdev);
>      PCIBus *bus;
>      int devfn;
> +    const pci_conf_init_t conf_init[] = {
> +        pci_conf_init_type00,
> +        pci_conf_init_type01,
> +        pci_conf_init_type02,
> +    };
>  
>      bus = FROM_QBUS(PCIBus, qdev_get_parent_bus(qdev));
>      devfn = pci_dev->devfn;
> +    assert(0 <= info->header_type);

so make it unsigned?

> +    assert(info->header_type <= ARRAY_SIZE(conf_init));
>      pci_dev = do_pci_register_device(pci_dev, bus, base->name, devfn,
> -                                     info->config_read, info->config_write);
> +                                     info->config_read, info->config_write,
> +                                     conf_init[info->header_type]);

Overdoing table-driven here. Simple switch would catch
errors faster, e.g. with default: value.

>      assert(pci_dev);
>      return info->init(pci_dev);
>  }
> diff --git a/hw/pci.h b/hw/pci.h
> index 5cf0b59..aadd1a5 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -106,6 +106,14 @@ typedef struct PCIIORegion {
>  #define  PCI_COMMAND_IO		0x1	/* Enable response in I/O space */
>  #define  PCI_COMMAND_MEMORY	0x2	/* Enable response in Memory space */
>  #define  PCI_COMMAND_MASTER	0x4	/* Enable bus master */
> +#define  PCI_COMMAND_SPECIAL	0x8	/* Enable response to special cycles */
> +#define  PCI_COMMAND_INVALIDATE 0x10	/* Use memory write and invalidate */
> +#define  PCI_COMMAND_VGA_PALETTE 0x20	/* Enable palette snooping */
> +#define  PCI_COMMAND_PARITY	0x40	/* Enable parity checking */
> +#define  PCI_COMMAND_WAIT	0x80	/* Enable address/data stepping */
> +#define  PCI_COMMAND_SERR	0x100	/* Enable SERR */
> +#define  PCI_COMMAND_FAST_BACK	0x200	/* Enable back-to-back writes */
> +#define  PCI_COMMAND_INTX_DISABLE 0x400 /* INTx Emulation Disable */
>  #define PCI_STATUS              0x06    /* 16 bits */
>  #define PCI_REVISION_ID         0x08    /* 8 bits  */
>  #define PCI_CLASS_PROG		0x09	/* Reg. Level Programming Interface */
> @@ -164,7 +172,32 @@ typedef struct PCIIORegion {
>  
>  /* Header type 1 (PCI-to-PCI bridges) */
>  #define PCI_SUBORDINATE_BUS     0x1a    /* Highest bus number behind the bridge */
> +#define PCI_SEC_LATENCY_TIMER   0x1b    /* Latency timer for secondary interface */
> +#define PCI_IO_BASE             0x1c    /* I/O range behind the bridge */
> +#define PCI_IO_LIMIT            0x1d
> +#define  PCI_IO_RANGE_TYPE_MASK 0x0fUL  /* I/O bridging type */
> +#define  PCI_IO_RANGE_TYPE_16   0x00
> +#define  PCI_IO_RANGE_TYPE_32   0x01
> +#define  PCI_IO_RANGE_MASK      (~0x0fUL)
> +#define PCI_MEMORY_BASE         0x20    /* Memory range behind */
> +#define PCI_MEMORY_LIMIT        0x22
> +#define  PCI_MEMORY_RANGE_TYPE_MASK 0x0fUL
> +#define  PCI_MEMORY_RANGE_MASK  (~0x0fUL)
> +#define PCI_PREF_MEMORY_BASE    0x24    /* Prefetchable memory range behind */
> +#define PCI_PREF_MEMORY_LIMIT   0x26
> +#define  PCI_PREF_RANGE_TYPE_MASK 0x0fUL
> +#define  PCI_PREF_RANGE_TYPE_32 0x00
> +#define  PCI_PREF_RANGE_TYPE_64 0x01
> +#define  PCI_PREF_RANGE_MASK    (~0x0fUL)
> +#define PCI_PREF_BASE_UPPER32   0x28    /* Upper half of prefetchable memory range */
> +#define PCI_PREF_LIMIT_UPPER32  0x2c
> +#define PCI_IO_BASE_UPPER16     0x30    /* Upper half of I/O addresses */
> +#define PCI_IO_LIMIT_UPPER16    0x32
> +/* 0x34 same as for htype 0 */
> +/* 0x35-0x3b is reserved */
>  #define PCI_ROM_ADDRESS1        0x38    /* Same as PCI_ROM_ADDRESS, but for htype 1 */
> +/* 0x3c-0x3d are same as for htype 0 */
> +#define PCI_BRIDGE_CONTROL      0x3e

There's a ton of registers here we do not really need, since you can
initialize a range to 0xff without looking at what is in the range.  And
frankly I prefer it this way rather than spend time checking each value.

>  /* Size of the standard PCI config header */
>  #define PCI_CONFIG_HEADER_SIZE 0x40
> @@ -377,6 +410,9 @@ typedef struct {
>      PCIConfigReadFunc *config_read;
>      PCIConfigWriteFunc *config_write;
>  
> +    /* pci config header type */
> +    uint8_t header_type;
> +

We don't need this in PCIDeviceInfo, this is part of
config space already.

>      /* pcie stuff */
>      int pcie;
>  } PCIDeviceInfo;
> -- 
> 1.6.0.2