Patchwork [V5,28/29] pci: initialize pci config headers depending it pci header type.

login
register
mail settings
Submitter Isaku Yamahata
Date Oct. 9, 2009, 6:29 a.m.
Message ID <1255069742-15724-29-git-send-email-yamahata@valinux.co.jp>
Download mbox | patch
Permalink /patch/35564/
State Under Review
Headers show

Comments

Isaku Yamahata - Oct. 9, 2009, 6:29 a.m.
- Only sets default subsystem id for header type 00.(normal header type)
  because header type 01 doesn't have subsystem id, and uses the register
  for other purpose. So setting default subsystem id doesn't make sense.

- initialize wmask more for header type 01.(bridge header type)
  Without those wmasks, linux was confused not boot.

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
 hw/cirrus_vga.c |    1 -
 hw/pci.c        |   62 ++++++++++++++++++++++++++++++++++++++++++++++++------
 hw/pci.h        |   33 +++++++++++++++++++++++++++++
 3 files changed, 88 insertions(+), 8 deletions(-)
Michael S. Tsirkin - Oct. 9, 2009, 10:42 a.m.
On Fri, Oct 09, 2009 at 03:29:01PM +0900, Isaku Yamahata wrote:
> - Only sets default subsystem id for header type 00.(normal header type)
>   because header type 01 doesn't have subsystem id, and uses the register
>   for other purpose. So setting default subsystem id doesn't make sense.
> 
> - initialize wmask more for header type 01.(bridge header type)
>   Without those wmasks, linux was confused not boot.

It was? Can you investigate please?
Spec says most of these are optional.

> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> ---
>  hw/cirrus_vga.c |    1 -
>  hw/pci.c        |   62 ++++++++++++++++++++++++++++++++++++++++++++++++------
>  hw/pci.h        |   33 +++++++++++++++++++++++++++++
>  3 files changed, 88 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c
> index ef72c62..c1bafd3 100644
> --- a/hw/cirrus_vga.c
> +++ b/hw/cirrus_vga.c
> @@ -180,7 +180,6 @@
>  #define PCI_COMMAND_PALETTESNOOPING         0x0020
>  #define PCI_COMMAND_PARITYDETECTION         0x0040
>  #define PCI_COMMAND_ADDRESSDATASTEPPING     0x0080
> -#define PCI_COMMAND_SERR                    0x0100
>  #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 3a17cd8..6b51bba 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -430,14 +430,52 @@ static void pci_init_wmask(PCIDevice *dev)
>      int i;
>      int config_size = pci_config_size(dev);
>  
> +    pci_set_word(dev->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);

At least INTX disable must not be writeable unless
we implement the masking functionality, and we don't

> +
>      dev->wmask[PCI_CACHE_LINE_SIZE] = 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)
> +
> +    for (i = PCI_CONFIG_HEADER_SIZE; i < config_size; i++)

I dislike this change. We don't use the old value so ++i
is cleaner.

>          dev->wmask[i] = 0xff;
>  }
>  
> +static void pci_init_wmask_bridge(PCIDevice *d)
> +{
> +    /* PCI_PRIMARY_BUS, PCI_SECONDARY_BUS, PCI_SUBORDINATE_BUS and
> +       PCI_SEC_LETENCY_TIMER */
> +    memset(d->wmask + PCI_PRIMARY_BUS, 0xff, 4);
> +
> +    /* sec status isn't emulated (yet) */
> +    d->wmask[PCI_SEC_STATUS] = 0;

0 is default value. Replace with /* TODO: emulate PCI_SEC_STATUS */
 (which bits do we have to emulate there?)

> +    /* base and limit */
> +    d->wmask[PCI_IO_BASE] = PCI_IO_RANGE_MASK & 0xff;
> +    d->wmask[PCI_IO_LIMIT] = PCI_IO_RANGE_MASK & 0xff;


& 0xff not necessary.

> +    pci_set_word(d->wmask + PCI_MEMORY_BASE,
> +                 PCI_MEMORY_RANGE_MASK & 0xffff);
> +    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);
> +
> +    pci_set_word(d->wmask + PCI_BRIDGE_CONTROL, 0xffff);

Can we just memset(xxx, 0xff, yyy) the whole range,
and just clear a single byte that is 0?

> +}
> +
>  static void pci_config_alloc(PCIDevice *pci_dev)
>  {
>      int config_size = pci_config_size(pci_dev);
> @@ -462,7 +500,8 @@ static void pci_config_free(PCIDevice *pci_dev)
>  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,
> +                                         uint8_t header_type)
>  {
>      if (devfn < 0) {
>          for(devfn = bus->devfn_min ; devfn < 256; devfn += 8) {
> @@ -479,9 +518,16 @@ 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);
> +
> +    header_type &= ~PCI_HEADER_TYPE_MULTI_FUNCTION;
> +    if (header_type == PCI_HEADER_TYPE_NORMAL) {
> +        pci_set_default_subsystem_id(pci_dev);
> +    }
>      pci_init_cmask(pci_dev);
>      pci_init_wmask(pci_dev);
> +    if (header_type == PCI_HEADER_TYPE_BRIDGE) {
> +        pci_init_wmask_bridge(pci_dev);
> +    }
>  
>      if (!config_read)
>          config_read = pci_default_read_config;
> @@ -504,7 +550,8 @@ 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_HEADER_TYPE_NORMAL);
>      return pci_dev;
>  }
>  static target_phys_addr_t pci_to_cpu_addr(target_phys_addr_t addr)
> @@ -1093,7 +1140,8 @@ static int pci_qdev_init(DeviceState *qdev, DeviceInfo *base)
>      bus = FROM_QBUS(PCIBus, qdev_get_parent_bus(qdev));
>      devfn = pci_dev->devfn;
>      pci_dev = do_pci_register_device(pci_dev, bus, base->name, devfn,
> -                                     info->config_read, info->config_write);
> +                                     info->config_read, info->config_write,
> +                                     info->header_type);
>      assert(pci_dev);
>      rc = info->init(pci_dev);
>      if (rc != 0)
> diff --git a/hw/pci.h b/hw/pci.h
> index 3cd93e9..a933ef1 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -100,6 +100,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 */
> @@ -162,7 +170,25 @@ 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_MASK      (~0x0fUL)
> +#define PCI_MEMORY_BASE         0x20    /* Memory range behind */
> +#define PCI_MEMORY_LIMIT        0x22
> +#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_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
>  
>  /* Size of the standard PCI config header */
>  #define PCI_CONFIG_HEADER_SIZE 0x40
> @@ -370,6 +396,13 @@ typedef struct {
>      PCIConfigReadFunc *config_read;
>      PCIConfigWriteFunc *config_write;
>  
> +    /* pci config header type */
> +    uint8_t header_type;        /* this is necessary for initialization
> +                                 * code to know its header type before
> +                                 * device specific code can initialize
> +                                 * configuration space.
> +                                 */
> +
>      /* pcie stuff */
>      int is_express;   /* is this device pci express?
>                         * initialization code needs to know this before
> -- 
> 1.6.0.2
Isaku Yamahata - Oct. 13, 2009, 2:31 p.m.
On Fri, Oct 09, 2009 at 12:42:32PM +0200, Michael S. Tsirkin wrote:
> On Fri, Oct 09, 2009 at 03:29:01PM +0900, Isaku Yamahata wrote:
> > - Only sets default subsystem id for header type 00.(normal header type)
> >   because header type 01 doesn't have subsystem id, and uses the register
> >   for other purpose. So setting default subsystem id doesn't make sense.
> > 
> > - initialize wmask more for header type 01.(bridge header type)
> >   Without those wmasks, linux was confused not boot.
> 
> It was? Can you investigate please?
> Spec says most of these are optional.

Linux kernel thinks the bridge doesn't support to forwarding
bus command disabling devices behind the bridge.
If pci-ide is behind the bridge, kernel panics failing to find
root file system.


> > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> > ---
> >  hw/cirrus_vga.c |    1 -
> >  hw/pci.c        |   62 ++++++++++++++++++++++++++++++++++++++++++++++++------
> >  hw/pci.h        |   33 +++++++++++++++++++++++++++++
> >  3 files changed, 88 insertions(+), 8 deletions(-)
> > 
> > diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c
> > index ef72c62..c1bafd3 100644
> > --- a/hw/cirrus_vga.c
> > +++ b/hw/cirrus_vga.c
> > @@ -180,7 +180,6 @@
> >  #define PCI_COMMAND_PALETTESNOOPING         0x0020
> >  #define PCI_COMMAND_PARITYDETECTION         0x0040
> >  #define PCI_COMMAND_ADDRESSDATASTEPPING     0x0080
> > -#define PCI_COMMAND_SERR                    0x0100
> >  #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 3a17cd8..6b51bba 100644
> > --- a/hw/pci.c
> > +++ b/hw/pci.c
> > @@ -430,14 +430,52 @@ static void pci_init_wmask(PCIDevice *dev)
> >      int i;
> >      int config_size = pci_config_size(dev);
> >  
> > +    pci_set_word(dev->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);
> 
> At least INTX disable must not be writeable unless
> we implement the masking functionality, and we don't

OK.

> > +
> >      dev->wmask[PCI_CACHE_LINE_SIZE] = 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)
> > +
> > +    for (i = PCI_CONFIG_HEADER_SIZE; i < config_size; i++)
> 
> I dislike this change. We don't use the old value so ++i
> is cleaner.

Ok. It was an accidental one change I didn't meant.


> >          dev->wmask[i] = 0xff;
> >  }
> >  
> > +static void pci_init_wmask_bridge(PCIDevice *d)
> > +{
> > +    /* PCI_PRIMARY_BUS, PCI_SECONDARY_BUS, PCI_SUBORDINATE_BUS and
> > +       PCI_SEC_LETENCY_TIMER */
> > +    memset(d->wmask + PCI_PRIMARY_BUS, 0xff, 4);
> > +
> > +    /* sec status isn't emulated (yet) */
> > +    d->wmask[PCI_SEC_STATUS] = 0;
> 
> 0 is default value. Replace with /* TODO: emulate PCI_SEC_STATUS */
>  (which bits do we have to emulate there?)

Ok.


> > +    /* base and limit */
> > +    d->wmask[PCI_IO_BASE] = PCI_IO_RANGE_MASK & 0xff;
> > +    d->wmask[PCI_IO_LIMIT] = PCI_IO_RANGE_MASK & 0xff;
> 
> 
> & 0xff not necessary.

OK.

> > +    pci_set_word(d->wmask + PCI_MEMORY_BASE,
> > +                 PCI_MEMORY_RANGE_MASK & 0xffff);
> > +    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);
> > +
> > +    pci_set_word(d->wmask + PCI_BRIDGE_CONTROL, 0xffff);
> 
> Can we just memset(xxx, 0xff, yyy) the whole range,
> and just clear a single byte that is 0?

It can be done for PCI_RREF_{BASE, LIMIT}_UPPER32 with length 4.
However all base/limit needs to be 0xfff0 or 0xfffffff0.
(not 0, but 0xf0) So it doesn't make code cleaner much.



> > +}
> > +
> >  static void pci_config_alloc(PCIDevice *pci_dev)
> >  {
> >      int config_size = pci_config_size(pci_dev);
> > @@ -462,7 +500,8 @@ static void pci_config_free(PCIDevice *pci_dev)
> >  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,
> > +                                         uint8_t header_type)
> >  {
> >      if (devfn < 0) {
> >          for(devfn = bus->devfn_min ; devfn < 256; devfn += 8) {
> > @@ -479,9 +518,16 @@ 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);
> > +
> > +    header_type &= ~PCI_HEADER_TYPE_MULTI_FUNCTION;
> > +    if (header_type == PCI_HEADER_TYPE_NORMAL) {
> > +        pci_set_default_subsystem_id(pci_dev);
> > +    }
> >      pci_init_cmask(pci_dev);
> >      pci_init_wmask(pci_dev);
> > +    if (header_type == PCI_HEADER_TYPE_BRIDGE) {
> > +        pci_init_wmask_bridge(pci_dev);
> > +    }
> >  
> >      if (!config_read)
> >          config_read = pci_default_read_config;
> > @@ -504,7 +550,8 @@ 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_HEADER_TYPE_NORMAL);
> >      return pci_dev;
> >  }
> >  static target_phys_addr_t pci_to_cpu_addr(target_phys_addr_t addr)
> > @@ -1093,7 +1140,8 @@ static int pci_qdev_init(DeviceState *qdev, DeviceInfo *base)
> >      bus = FROM_QBUS(PCIBus, qdev_get_parent_bus(qdev));
> >      devfn = pci_dev->devfn;
> >      pci_dev = do_pci_register_device(pci_dev, bus, base->name, devfn,
> > -                                     info->config_read, info->config_write);
> > +                                     info->config_read, info->config_write,
> > +                                     info->header_type);
> >      assert(pci_dev);
> >      rc = info->init(pci_dev);
> >      if (rc != 0)
> > diff --git a/hw/pci.h b/hw/pci.h
> > index 3cd93e9..a933ef1 100644
> > --- a/hw/pci.h
> > +++ b/hw/pci.h
> > @@ -100,6 +100,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 */
> > @@ -162,7 +170,25 @@ 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_MASK      (~0x0fUL)
> > +#define PCI_MEMORY_BASE         0x20    /* Memory range behind */
> > +#define PCI_MEMORY_LIMIT        0x22
> > +#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_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
> >  
> >  /* Size of the standard PCI config header */
> >  #define PCI_CONFIG_HEADER_SIZE 0x40
> > @@ -370,6 +396,13 @@ typedef struct {
> >      PCIConfigReadFunc *config_read;
> >      PCIConfigWriteFunc *config_write;
> >  
> > +    /* pci config header type */
> > +    uint8_t header_type;        /* this is necessary for initialization
> > +                                 * code to know its header type before
> > +                                 * device specific code can initialize
> > +                                 * configuration space.
> > +                                 */
> > +
> >      /* pcie stuff */
> >      int is_express;   /* is this device pci express?
> >                         * initialization code needs to know this before
>
Michael S. Tsirkin - Oct. 13, 2009, 2:52 p.m.
On Tue, Oct 13, 2009 at 11:31:23PM +0900, Isaku Yamahata wrote:
> On Fri, Oct 09, 2009 at 12:42:32PM +0200, Michael S. Tsirkin wrote:
> > On Fri, Oct 09, 2009 at 03:29:01PM +0900, Isaku Yamahata wrote:
> > > - Only sets default subsystem id for header type 00.(normal header type)
> > >   because header type 01 doesn't have subsystem id, and uses the register
> > >   for other purpose. So setting default subsystem id doesn't make sense.
> > > 
> > > - initialize wmask more for header type 01.(bridge header type)
> > >   Without those wmasks, linux was confused not boot.
> > 
> > It was? Can you investigate please?
> > Spec says most of these are optional.
> 
> Linux kernel thinks the bridge doesn't support to forwarding
> bus command disabling devices behind the bridge.

Sorry, had trouble parsing this sentence.


> If pci-ide is behind the bridge, kernel panics failing to find
> root file system.
> 

Yes, but why does it help to make bus master enable writeable
when we never initiate pci transactions from bridge itself?

> > > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> > > ---
> > >  hw/cirrus_vga.c |    1 -
> > >  hw/pci.c        |   62 ++++++++++++++++++++++++++++++++++++++++++++++++------
> > >  hw/pci.h        |   33 +++++++++++++++++++++++++++++
> > >  3 files changed, 88 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c
> > > index ef72c62..c1bafd3 100644
> > > --- a/hw/cirrus_vga.c
> > > +++ b/hw/cirrus_vga.c
> > > @@ -180,7 +180,6 @@
> > >  #define PCI_COMMAND_PALETTESNOOPING         0x0020
> > >  #define PCI_COMMAND_PARITYDETECTION         0x0040
> > >  #define PCI_COMMAND_ADDRESSDATASTEPPING     0x0080
> > > -#define PCI_COMMAND_SERR                    0x0100
> > >  #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 3a17cd8..6b51bba 100644
> > > --- a/hw/pci.c
> > > +++ b/hw/pci.c
> > > @@ -430,14 +430,52 @@ static void pci_init_wmask(PCIDevice *dev)
> > >      int i;
> > >      int config_size = pci_config_size(dev);
> > >  
> > > +    pci_set_word(dev->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);
> > 
> > At least INTX disable must not be writeable unless
> > we implement the masking functionality, and we don't
> 
> OK.
> 
> > > +
> > >      dev->wmask[PCI_CACHE_LINE_SIZE] = 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)
> > > +
> > > +    for (i = PCI_CONFIG_HEADER_SIZE; i < config_size; i++)
> > 
> > I dislike this change. We don't use the old value so ++i
> > is cleaner.
> 
> Ok. It was an accidental one change I didn't meant.
> 
> 
> > >          dev->wmask[i] = 0xff;
> > >  }
> > >  
> > > +static void pci_init_wmask_bridge(PCIDevice *d)
> > > +{
> > > +    /* PCI_PRIMARY_BUS, PCI_SECONDARY_BUS, PCI_SUBORDINATE_BUS and
> > > +       PCI_SEC_LETENCY_TIMER */
> > > +    memset(d->wmask + PCI_PRIMARY_BUS, 0xff, 4);
> > > +
> > > +    /* sec status isn't emulated (yet) */
> > > +    d->wmask[PCI_SEC_STATUS] = 0;
> > 
> > 0 is default value. Replace with /* TODO: emulate PCI_SEC_STATUS */
> >  (which bits do we have to emulate there?)
> 
> Ok.
> 
> 
> > > +    /* base and limit */
> > > +    d->wmask[PCI_IO_BASE] = PCI_IO_RANGE_MASK & 0xff;
> > > +    d->wmask[PCI_IO_LIMIT] = PCI_IO_RANGE_MASK & 0xff;
> > 
> > 
> > & 0xff not necessary.
> 
> OK.
> 
> > > +    pci_set_word(d->wmask + PCI_MEMORY_BASE,
> > > +                 PCI_MEMORY_RANGE_MASK & 0xffff);
> > > +    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);
> > > +
> > > +    pci_set_word(d->wmask + PCI_BRIDGE_CONTROL, 0xffff);
> > 
> > Can we just memset(xxx, 0xff, yyy) the whole range,
> > and just clear a single byte that is 0?
> 
> It can be done for PCI_RREF_{BASE, LIMIT}_UPPER32 with length 4.
> However all base/limit needs to be 0xfff0 or 0xfffffff0.
> (not 0, but 0xf0) So it doesn't make code cleaner much.
> 
> 
> 
> > > +}
> > > +
> > >  static void pci_config_alloc(PCIDevice *pci_dev)
> > >  {
> > >      int config_size = pci_config_size(pci_dev);
> > > @@ -462,7 +500,8 @@ static void pci_config_free(PCIDevice *pci_dev)
> > >  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,
> > > +                                         uint8_t header_type)
> > >  {
> > >      if (devfn < 0) {
> > >          for(devfn = bus->devfn_min ; devfn < 256; devfn += 8) {
> > > @@ -479,9 +518,16 @@ 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);
> > > +
> > > +    header_type &= ~PCI_HEADER_TYPE_MULTI_FUNCTION;
> > > +    if (header_type == PCI_HEADER_TYPE_NORMAL) {
> > > +        pci_set_default_subsystem_id(pci_dev);
> > > +    }
> > >      pci_init_cmask(pci_dev);
> > >      pci_init_wmask(pci_dev);
> > > +    if (header_type == PCI_HEADER_TYPE_BRIDGE) {
> > > +        pci_init_wmask_bridge(pci_dev);
> > > +    }
> > >  
> > >      if (!config_read)
> > >          config_read = pci_default_read_config;
> > > @@ -504,7 +550,8 @@ 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_HEADER_TYPE_NORMAL);
> > >      return pci_dev;
> > >  }
> > >  static target_phys_addr_t pci_to_cpu_addr(target_phys_addr_t addr)
> > > @@ -1093,7 +1140,8 @@ static int pci_qdev_init(DeviceState *qdev, DeviceInfo *base)
> > >      bus = FROM_QBUS(PCIBus, qdev_get_parent_bus(qdev));
> > >      devfn = pci_dev->devfn;
> > >      pci_dev = do_pci_register_device(pci_dev, bus, base->name, devfn,
> > > -                                     info->config_read, info->config_write);
> > > +                                     info->config_read, info->config_write,
> > > +                                     info->header_type);
> > >      assert(pci_dev);
> > >      rc = info->init(pci_dev);
> > >      if (rc != 0)
> > > diff --git a/hw/pci.h b/hw/pci.h
> > > index 3cd93e9..a933ef1 100644
> > > --- a/hw/pci.h
> > > +++ b/hw/pci.h
> > > @@ -100,6 +100,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 */
> > > @@ -162,7 +170,25 @@ 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_MASK      (~0x0fUL)
> > > +#define PCI_MEMORY_BASE         0x20    /* Memory range behind */
> > > +#define PCI_MEMORY_LIMIT        0x22
> > > +#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_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
> > >  
> > >  /* Size of the standard PCI config header */
> > >  #define PCI_CONFIG_HEADER_SIZE 0x40
> > > @@ -370,6 +396,13 @@ typedef struct {
> > >      PCIConfigReadFunc *config_read;
> > >      PCIConfigWriteFunc *config_write;
> > >  
> > > +    /* pci config header type */
> > > +    uint8_t header_type;        /* this is necessary for initialization
> > > +                                 * code to know its header type before
> > > +                                 * device specific code can initialize
> > > +                                 * configuration space.
> > > +                                 */
> > > +
> > >      /* pcie stuff */
> > >      int is_express;   /* is this device pci express?
> > >                         * initialization code needs to know this before
> > 
> 
> -- 
> yamahata
Michael S. Tsirkin - Oct. 13, 2009, 3:06 p.m.
On Tue, Oct 13, 2009 at 11:31:23PM +0900, Isaku Yamahata wrote:
> On Fri, Oct 09, 2009 at 12:42:32PM +0200, Michael S. Tsirkin wrote:
> > On Fri, Oct 09, 2009 at 03:29:01PM +0900, Isaku Yamahata wrote:
> > > - Only sets default subsystem id for header type 00.(normal header type)
> > >   because header type 01 doesn't have subsystem id, and uses the register
> > >   for other purpose. So setting default subsystem id doesn't make sense.
> > > 
> > > - initialize wmask more for header type 01.(bridge header type)
> > >   Without those wmasks, linux was confused not boot.
> > 
> > It was? Can you investigate please?
> > Spec says most of these are optional.
> 
> Linux kernel thinks the bridge doesn't support to forwarding
> bus command disabling devices behind the bridge.
> If pci-ide is behind the bridge, kernel panics failing to find
> root file system.
> 
> 
> > > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> > > ---
> > >  hw/cirrus_vga.c |    1 -
> > >  hw/pci.c        |   62 ++++++++++++++++++++++++++++++++++++++++++++++++------
> > >  hw/pci.h        |   33 +++++++++++++++++++++++++++++
> > >  3 files changed, 88 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c
> > > index ef72c62..c1bafd3 100644
> > > --- a/hw/cirrus_vga.c
> > > +++ b/hw/cirrus_vga.c
> > > @@ -180,7 +180,6 @@
> > >  #define PCI_COMMAND_PALETTESNOOPING         0x0020
> > >  #define PCI_COMMAND_PARITYDETECTION         0x0040
> > >  #define PCI_COMMAND_ADDRESSDATASTEPPING     0x0080
> > > -#define PCI_COMMAND_SERR                    0x0100
> > >  #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 3a17cd8..6b51bba 100644
> > > --- a/hw/pci.c
> > > +++ b/hw/pci.c
> > > @@ -430,14 +430,52 @@ static void pci_init_wmask(PCIDevice *dev)
> > >      int i;
> > >      int config_size = pci_config_size(dev);
> > >  
> > > +    pci_set_word(dev->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);
> > 
> > At least INTX disable must not be writeable unless
> > we implement the masking functionality, and we don't
> 
> OK.
> 
> > > +
> > >      dev->wmask[PCI_CACHE_LINE_SIZE] = 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)
> > > +
> > > +    for (i = PCI_CONFIG_HEADER_SIZE; i < config_size; i++)
> > 
> > I dislike this change. We don't use the old value so ++i
> > is cleaner.
> 
> Ok. It was an accidental one change I didn't meant.
> 
> 
> > >          dev->wmask[i] = 0xff;
> > >  }
> > >  
> > > +static void pci_init_wmask_bridge(PCIDevice *d)
> > > +{
> > > +    /* PCI_PRIMARY_BUS, PCI_SECONDARY_BUS, PCI_SUBORDINATE_BUS and
> > > +       PCI_SEC_LETENCY_TIMER */
> > > +    memset(d->wmask + PCI_PRIMARY_BUS, 0xff, 4);
> > > +
> > > +    /* sec status isn't emulated (yet) */
> > > +    d->wmask[PCI_SEC_STATUS] = 0;
> > 
> > 0 is default value. Replace with /* TODO: emulate PCI_SEC_STATUS */
> >  (which bits do we have to emulate there?)
> 
> Ok.
> 
> 
> > > +    /* base and limit */
> > > +    d->wmask[PCI_IO_BASE] = PCI_IO_RANGE_MASK & 0xff;
> > > +    d->wmask[PCI_IO_LIMIT] = PCI_IO_RANGE_MASK & 0xff;
> > 
> > 
> > & 0xff not necessary.
> 
> OK.
> 
> > > +    pci_set_word(d->wmask + PCI_MEMORY_BASE,
> > > +                 PCI_MEMORY_RANGE_MASK & 0xffff);
> > > +    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);

& 0xffff is not necessary.

> > > +    pci_set_long(d->wmask + PCI_PREF_BASE_UPPER32, 0xffffffff);
> > > +    pci_set_long(d->wmask + PCI_PREF_LIMIT_UPPER32, 0xffffffff);
> > > +
> > > +    pci_set_word(d->wmask + PCI_BRIDGE_CONTROL, 0xffff);
> > 
> > Can we just memset(xxx, 0xff, yyy) the whole range,
> > and just clear a single byte that is 0?
> 
> It can be done for PCI_RREF_{BASE, LIMIT}_UPPER32 with length 4.

Length 8 actually (they sit together).

> However all base/limit needs to be 0xfff0 or 0xfffffff0.

That's true, I forgot.

> (not 0, but 0xf0) So it doesn't make code cleaner much.



> 
> 
> > > +}
> > > +
> > >  static void pci_config_alloc(PCIDevice *pci_dev)
> > >  {
> > >      int config_size = pci_config_size(pci_dev);
> > > @@ -462,7 +500,8 @@ static void pci_config_free(PCIDevice *pci_dev)
> > >  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,
> > > +                                         uint8_t header_type)
> > >  {
> > >      if (devfn < 0) {
> > >          for(devfn = bus->devfn_min ; devfn < 256; devfn += 8) {
> > > @@ -479,9 +518,16 @@ 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);
> > > +
> > > +    header_type &= ~PCI_HEADER_TYPE_MULTI_FUNCTION;
> > > +    if (header_type == PCI_HEADER_TYPE_NORMAL) {
> > > +        pci_set_default_subsystem_id(pci_dev);
> > > +    }
> > >      pci_init_cmask(pci_dev);
> > >      pci_init_wmask(pci_dev);
> > > +    if (header_type == PCI_HEADER_TYPE_BRIDGE) {
> > > +        pci_init_wmask_bridge(pci_dev);
> > > +    }
> > >  
> > >      if (!config_read)
> > >          config_read = pci_default_read_config;
> > > @@ -504,7 +550,8 @@ 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_HEADER_TYPE_NORMAL);
> > >      return pci_dev;
> > >  }
> > >  static target_phys_addr_t pci_to_cpu_addr(target_phys_addr_t addr)
> > > @@ -1093,7 +1140,8 @@ static int pci_qdev_init(DeviceState *qdev, DeviceInfo *base)
> > >      bus = FROM_QBUS(PCIBus, qdev_get_parent_bus(qdev));
> > >      devfn = pci_dev->devfn;
> > >      pci_dev = do_pci_register_device(pci_dev, bus, base->name, devfn,
> > > -                                     info->config_read, info->config_write);
> > > +                                     info->config_read, info->config_write,
> > > +                                     info->header_type);
> > >      assert(pci_dev);
> > >      rc = info->init(pci_dev);
> > >      if (rc != 0)
> > > diff --git a/hw/pci.h b/hw/pci.h
> > > index 3cd93e9..a933ef1 100644
> > > --- a/hw/pci.h
> > > +++ b/hw/pci.h
> > > @@ -100,6 +100,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 */
> > > @@ -162,7 +170,25 @@ 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_MASK      (~0x0fUL)
> > > +#define PCI_MEMORY_BASE         0x20    /* Memory range behind */
> > > +#define PCI_MEMORY_LIMIT        0x22
> > > +#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_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
> > >  
> > >  /* Size of the standard PCI config header */
> > >  #define PCI_CONFIG_HEADER_SIZE 0x40
> > > @@ -370,6 +396,13 @@ typedef struct {
> > >      PCIConfigReadFunc *config_read;
> > >      PCIConfigWriteFunc *config_write;
> > >  
> > > +    /* pci config header type */
> > > +    uint8_t header_type;        /* this is necessary for initialization
> > > +                                 * code to know its header type before
> > > +                                 * device specific code can initialize
> > > +                                 * configuration space.
> > > +                                 */
> > > +
> > >      /* pcie stuff */
> > >      int is_express;   /* is this device pci express?
> > >                         * initialization code needs to know this before
> > 
> 
> -- 
> yamahata

Patch

diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c
index ef72c62..c1bafd3 100644
--- a/hw/cirrus_vga.c
+++ b/hw/cirrus_vga.c
@@ -180,7 +180,6 @@ 
 #define PCI_COMMAND_PALETTESNOOPING         0x0020
 #define PCI_COMMAND_PARITYDETECTION         0x0040
 #define PCI_COMMAND_ADDRESSDATASTEPPING     0x0080
-#define PCI_COMMAND_SERR                    0x0100
 #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 3a17cd8..6b51bba 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -430,14 +430,52 @@  static void pci_init_wmask(PCIDevice *dev)
     int i;
     int config_size = pci_config_size(dev);
 
+    pci_set_word(dev->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);
+
     dev->wmask[PCI_CACHE_LINE_SIZE] = 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)
+
+    for (i = PCI_CONFIG_HEADER_SIZE; i < config_size; i++)
         dev->wmask[i] = 0xff;
 }
 
+static void pci_init_wmask_bridge(PCIDevice *d)
+{
+    /* PCI_PRIMARY_BUS, PCI_SECONDARY_BUS, PCI_SUBORDINATE_BUS and
+       PCI_SEC_LETENCY_TIMER */
+    memset(d->wmask + PCI_PRIMARY_BUS, 0xff, 4);
+
+    /* sec status isn't emulated (yet) */
+    d->wmask[PCI_SEC_STATUS] = 0;
+
+    /* base and limit */
+    d->wmask[PCI_IO_BASE] = PCI_IO_RANGE_MASK & 0xff;
+    d->wmask[PCI_IO_LIMIT] = PCI_IO_RANGE_MASK & 0xff;
+    pci_set_word(d->wmask + PCI_MEMORY_BASE,
+                 PCI_MEMORY_RANGE_MASK & 0xffff);
+    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);
+
+    pci_set_word(d->wmask + PCI_BRIDGE_CONTROL, 0xffff);
+}
+
 static void pci_config_alloc(PCIDevice *pci_dev)
 {
     int config_size = pci_config_size(pci_dev);
@@ -462,7 +500,8 @@  static void pci_config_free(PCIDevice *pci_dev)
 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,
+                                         uint8_t header_type)
 {
     if (devfn < 0) {
         for(devfn = bus->devfn_min ; devfn < 256; devfn += 8) {
@@ -479,9 +518,16 @@  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);
+
+    header_type &= ~PCI_HEADER_TYPE_MULTI_FUNCTION;
+    if (header_type == PCI_HEADER_TYPE_NORMAL) {
+        pci_set_default_subsystem_id(pci_dev);
+    }
     pci_init_cmask(pci_dev);
     pci_init_wmask(pci_dev);
+    if (header_type == PCI_HEADER_TYPE_BRIDGE) {
+        pci_init_wmask_bridge(pci_dev);
+    }
 
     if (!config_read)
         config_read = pci_default_read_config;
@@ -504,7 +550,8 @@  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_HEADER_TYPE_NORMAL);
     return pci_dev;
 }
 static target_phys_addr_t pci_to_cpu_addr(target_phys_addr_t addr)
@@ -1093,7 +1140,8 @@  static int pci_qdev_init(DeviceState *qdev, DeviceInfo *base)
     bus = FROM_QBUS(PCIBus, qdev_get_parent_bus(qdev));
     devfn = pci_dev->devfn;
     pci_dev = do_pci_register_device(pci_dev, bus, base->name, devfn,
-                                     info->config_read, info->config_write);
+                                     info->config_read, info->config_write,
+                                     info->header_type);
     assert(pci_dev);
     rc = info->init(pci_dev);
     if (rc != 0)
diff --git a/hw/pci.h b/hw/pci.h
index 3cd93e9..a933ef1 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -100,6 +100,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 */
@@ -162,7 +170,25 @@  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_MASK      (~0x0fUL)
+#define PCI_MEMORY_BASE         0x20    /* Memory range behind */
+#define PCI_MEMORY_LIMIT        0x22
+#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_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
 
 /* Size of the standard PCI config header */
 #define PCI_CONFIG_HEADER_SIZE 0x40
@@ -370,6 +396,13 @@  typedef struct {
     PCIConfigReadFunc *config_read;
     PCIConfigWriteFunc *config_write;
 
+    /* pci config header type */
+    uint8_t header_type;        /* this is necessary for initialization
+                                 * code to know its header type before
+                                 * device specific code can initialize
+                                 * configuration space.
+                                 */
+
     /* pcie stuff */
     int is_express;   /* is this device pci express?
                        * initialization code needs to know this before