[Qemu-devel] Re: [PATCH 22/23] pci: initialize wmask according to pci header type.

["Michael S. Tsirkin" <mst@redhat.com>]

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