From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1MuoHP-0006QN-Sx for qemu-devel@nongnu.org; Mon, 05 Oct 2009 10:11:11 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1MuoHL-0006Q7-Cx for qemu-devel@nongnu.org; Mon, 05 Oct 2009 10:11:11 -0400 Received: from [199.232.76.173] (port=57028 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1MuoHL-0006Q4-3x for qemu-devel@nongnu.org; Mon, 05 Oct 2009 10:11:07 -0400 Received: from mx1.redhat.com ([209.132.183.28]:57123) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1MuoHK-0003VX-DF for qemu-devel@nongnu.org; Mon, 05 Oct 2009 10:11:06 -0400 Date: Mon, 5 Oct 2009 16:09:03 +0200 From: "Michael S. Tsirkin" Message-ID: <20091005140902.GD31056@redhat.com> References: <1254737223-16129-1-git-send-email-yamahata@valinux.co.jp> <1254737223-16129-23-git-send-email-yamahata@valinux.co.jp> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1254737223-16129-23-git-send-email-yamahata@valinux.co.jp> Subject: [Qemu-devel] Re: [PATCH 22/23] pci: initialize wmask according to pci header type. List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Isaku Yamahata Cc: qemu-devel@nongnu.org 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 > --- > 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