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 <yamah...@valinux.co.jp> > > --- > > 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