[Qemu-devel] Re: [PATCH V6 28/32] pci: initialize pci config headers depending it pci header type.

[Isaku Yamahata]

On Tue, Nov 03, 2009 at 04:27:18PM +0200, Michael S. Tsirkin wrote:
> On Fri, Oct 30, 2009 at 09:21:22PM +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,
> >   and lspci was confused not to print out expected IO/memory range.
> > 
> > Signed-off-by: Isaku Yamahata <address@hidden>
> > ---
> >  hw/cirrus_vga.c |    1 -
> >  hw/pci.c        |   42 ++++++++++++++++++++++++++++++++++++++----
> >  hw/pci.h        |   29 +++++++++++++++++++++++++++++
> >  3 files changed, 67 insertions(+), 5 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 46b22ec..beefae3 100644
> > --- a/hw/pci.c
> > +++ b/hw/pci.c
> > @@ -445,6 +445,30 @@ static void pci_init_wmask(PCIDevice *dev)
> >          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);
> > +
> > +    /* 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_PREF_BASE_UPPER32 and PCI_PREF_LIMIT_UPPER32 */
> > +    memset(d->wmask + PCI_PREF_BASE_UPPER32, 0xff, 8);
> > +
> > +    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);
> > @@ -467,7 +491,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) {
> > @@ -484,9 +509,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;
> 
> Instead of this, can we have pci_init_bridge that will simply
> be called after device has been initialized?
> Down the road we will be able to move it to pci_bridge.c

Makes sense.
I will address it when I implement pci hot plug which requires
pcie port emulator.


> 
> 
> > @@ -509,7 +541,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)
> > @@ -1059,7 +1092,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 7991bfd..3f50294 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 */
> > @@ -119,16 +127,30 @@ typedef struct PCIIORegion {
> >  #define PCI_PRIMARY_BUS            0x18    /* Primary bus number */
> >  #define PCI_SECONDARY_BUS  0x19    /* Secondary bus number */
> >  #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_SEC_STATUS             0x1e    /* Secondary status register, 
> > only bit 14 used */
> > +#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_SUBSYSTEM_VENDOR_ID 0x2c    /* 16 bits */
> >  #define PCI_SUBSYSTEM_ID        0x2e    /* 16 bits */
> >  #define PCI_ROM_ADDRESS            0x30    /* Bits 31..11 are address, 
> > 10..1 reserved */
> >  #define  PCI_ROM_ADDRESS_ENABLE    0x01
> > +#define PCI_IO_BASE_UPPER16     0x30    /* Upper half of I/O addresses */
> > +#define PCI_IO_LIMIT_UPPER16    0x32
> >  #define PCI_CAPABILITY_LIST        0x34    /* Offset of first capability 
> > list entry */
> >  #define PCI_ROM_ADDRESS1   0x38    /* Same as PCI_ROM_ADDRESS, but for 
> > htype 1 */
> >  #define PCI_INTERRUPT_LINE 0x3c    /* 8 bits */
> >  #define PCI_INTERRUPT_PIN  0x3d    /* 8 bits */
> >  #define PCI_MIN_GNT                0x3e    /* 8 bits */
> > +#define PCI_BRIDGE_CONTROL      0x3e
> >  #define PCI_MAX_LAT                0x3f    /* 8 bits */
> >  
> >  /* Capability lists */
> 
> 
> Many of these are unused. I'd rather we only defined what we use.
> 
> > @@ -358,6 +380,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.
> > +                                 */
> 
> 
> The text probably belongs in commit comment.
> Comments that tell how field is used quickly get out of date.
> Say *what* it is is enough.
> 
> > +
> >      /* pcie stuff */
> >      int is_express;   /* is this device pci express?
> >                         * initialization code needs to know this before
> 

-- 
yamahata