Re: [Qemu-devel] [PATCH 4/4] pci: use pci_config_header in pci.c

[Anthony Liguori <anthony@codemonkey.ws>]

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 <kraxel@redhat.com>
> ---
>  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;
>  }
>