From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1Kl50M-0008Il-SO for qemu-devel@nongnu.org; Wed, 01 Oct 2008 12:56:51 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1Kl50L-0008HE-9o for qemu-devel@nongnu.org; Wed, 01 Oct 2008 12:56:49 -0400 Received: from [199.232.76.173] (port=35527 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Kl50L-0008Gw-3D for qemu-devel@nongnu.org; Wed, 01 Oct 2008 12:56:49 -0400 Received: from an-out-0708.google.com ([209.85.132.244]:33200) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1Kl4cP-0007XP-0q for qemu-devel@nongnu.org; Wed, 01 Oct 2008 12:32:05 -0400 Received: by an-out-0708.google.com with SMTP id d18so34403and.130 for ; Wed, 01 Oct 2008 09:31:57 -0700 (PDT) Message-ID: <48E3A5BD.8010000@codemonkey.ws> Date: Wed, 01 Oct 2008 11:30:53 -0500 From: Anthony Liguori MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH 4/4] pci: use pci_config_header in pci.c References: <1222870375-13489-1-git-send-email-kraxel@redhat.com> <1222870375-13489-4-git-send-email-kraxel@redhat.com> In-Reply-To: <1222870375-13489-4-git-send-email-kraxel@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Reply-To: qemu-devel@nongnu.org List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org Cc: Gerd Hoffmann , Paul Brook Gerd Hoffmann wrote: > This makes use of the new pci_config_header struct in pci.c, > squashing a bunch of casts and hard-coded magic numbers. > I thought the last bit of feedback that you received from Paul was that it seemed like a waste to go through the trouble of introducing this PCI config structure but then forcing the users to do explicit endian casting. I agree with this and was expecting the updated patches to do the endian conversion automatically. Any reasons for not doing it this way? Regards, Anthony Liguori > Signed-off-by: Gerd Hoffmann > --- > hw/pci.c | 64 +++++++++++++++++++++++++++++++------------------------------- > 1 files changed, 32 insertions(+), 32 deletions(-) > > diff --git a/hw/pci.c b/hw/pci.c > index f2d0c4b..b142709 100644 > --- a/hw/pci.c > +++ b/hw/pci.c > @@ -199,8 +199,9 @@ void pci_register_io_region(PCIDevice *pci_dev, int region_num, > uint32_t size, int type, > PCIMapIORegionFunc *map_func) > { > + struct pci_config_header *conf = (void*)pci_dev->config; > PCIIORegion *r; > - uint32_t addr; > + le32 *addr; > > if ((unsigned int)region_num >= PCI_NUM_REGIONS) > return; > @@ -210,11 +211,11 @@ void pci_register_io_region(PCIDevice *pci_dev, int region_num, > r->type = type; > r->map_func = map_func; > if (region_num == PCI_ROM_SLOT) { > - addr = 0x30; > + addr = &conf->rom_addr; > } else { > - addr = 0x10 + region_num * 4; > + addr = conf->base_address_regs + region_num; > } > - *(uint32_t *)(pci_dev->config + addr) = cpu_to_le32(type); > + *addr = cpu_to_le32(type); > } > > static target_phys_addr_t pci_to_cpu_addr(target_phys_addr_t addr) > @@ -224,23 +225,24 @@ static target_phys_addr_t pci_to_cpu_addr(target_phys_addr_t addr) > > static void pci_update_mappings(PCIDevice *d) > { > + struct pci_config_header *conf = (void*)d->config; > PCIIORegion *r; > int cmd, i; > - uint32_t last_addr, new_addr, config_ofs; > + uint32_t last_addr, new_addr; > + le32 *config_addr; > > - cmd = le16_to_cpu(*(uint16_t *)(d->config + PCI_COMMAND)); > + cmd = le16_to_cpu(conf->command); > for(i = 0; i < PCI_NUM_REGIONS; i++) { > r = &d->io_regions[i]; > if (i == PCI_ROM_SLOT) { > - config_ofs = 0x30; > + config_addr = &conf->rom_addr; > } else { > - config_ofs = 0x10 + i * 4; > + config_addr = conf->base_address_regs + i; > } > if (r->size != 0) { > if (r->type & PCI_ADDRESS_SPACE_IO) { > if (cmd & PCI_COMMAND_IO) { > - new_addr = le32_to_cpu(*(uint32_t *)(d->config + > - config_ofs)); > + new_addr = le32_to_cpu(*config_addr); > new_addr = new_addr & ~(r->size - 1); > last_addr = new_addr + r->size - 1; > /* NOTE: we have only 64K ioports on PC */ > @@ -253,8 +255,7 @@ static void pci_update_mappings(PCIDevice *d) > } > } else { > if (cmd & PCI_COMMAND_MEMORY) { > - new_addr = le32_to_cpu(*(uint32_t *)(d->config + > - config_ofs)); > + new_addr = le32_to_cpu(*config_addr); > /* the ROM slot has a specific enable bit */ > if (i == PCI_ROM_SLOT && !(new_addr & 1)) > goto no_mem_map; > @@ -568,13 +569,14 @@ static pci_class_desc pci_class_descriptions[] = > > static void pci_info_device(PCIDevice *d) > { > + struct pci_config_header *conf = (void*)d->config; > int i, class; > PCIIORegion *r; > pci_class_desc *desc; > > term_printf(" Bus %2d, device %3d, function %d:\n", > d->bus->bus_num, d->devfn >> 3, d->devfn & 7); > - class = le16_to_cpu(*((uint16_t *)(d->config + PCI_CLASS_DEVICE))); > + class = conf->class << 8 | conf->subclass; > term_printf(" "); > desc = pci_class_descriptions; > while (desc->desc && class != desc->class) > @@ -585,11 +587,11 @@ static void pci_info_device(PCIDevice *d) > term_printf("Class %04x", class); > } > term_printf(": PCI device %04x:%04x\n", > - le16_to_cpu(*((uint16_t *)(d->config + PCI_VENDOR_ID))), > - le16_to_cpu(*((uint16_t *)(d->config + PCI_DEVICE_ID)))); > + le16_to_cpu(conf->vendor_id), > + le16_to_cpu(conf->device_id)); > > - if (d->config[PCI_INTERRUPT_PIN] != 0) { > - term_printf(" IRQ %d.\n", d->config[PCI_INTERRUPT_LINE]); > + if (conf->interrupt_line != 0) { > + term_printf(" IRQ %d.\n", conf->interrupt_line); > } > if (class == 0x0604) { > term_printf(" BUS %d.\n", d->config[0x19]); > @@ -686,25 +688,23 @@ static void pci_bridge_write_config(PCIDevice *d, > PCIBus *pci_bridge_init(PCIBus *bus, int devfn, uint32_t id, > pci_map_irq_fn map_irq, const char *name) > { > + struct pci_config_header *conf; > PCIBridge *s; > s = (PCIBridge *)pci_register_device(bus, name, sizeof(PCIBridge), > devfn, NULL, pci_bridge_write_config); > - s->dev.config[0x00] = id >> 16; > - s->dev.config[0x01] = id >> 24; > - s->dev.config[0x02] = id; // device_id > - s->dev.config[0x03] = id >> 8; > - s->dev.config[0x04] = 0x06; // command = bus master, pci mem > - s->dev.config[0x05] = 0x00; > - s->dev.config[0x06] = 0xa0; // status = fast back-to-back, 66MHz, no error > - s->dev.config[0x07] = 0x00; // status = fast devsel > - s->dev.config[0x08] = 0x00; // revision > - s->dev.config[0x09] = 0x00; // programming i/f > - s->dev.config[0x0A] = 0x04; // class_sub = PCI to PCI bridge > - s->dev.config[0x0B] = 0x06; // class_base = PCI_bridge > - s->dev.config[0x0D] = 0x10; // latency_timer > - s->dev.config[0x0E] = 0x81; // header_type > - s->dev.config[0x1E] = 0xa0; // secondary status > + conf = (void*)s->dev.config; > + conf->vendor_id = cpu_to_le16(id >> 16); > + conf->device_id = cpu_to_le16(id & 0xffff); > + conf->command = cpu_to_le16(0x0006); // bus master, pci mem > + conf->status = cpu_to_le16(0x00a0); // fast back-to-back, 66MHz, no error, fast devsel > + conf->revision = 0x00; > + conf->api = 0x00; > + conf->subclass = 0x04; // PCI to PCI bridge > + conf->class = 0x06; // PCI_bridge > + conf->latency_timer = 0x10; > + conf->header_type = 0x81; > > + s->dev.config[0x1E] = 0xa0; // secondary status > s->bus = pci_register_secondary_bus(&s->dev, map_irq); > return s->bus; > } >