[Qemu-devel] Re: [PATCH V6 29/32] pci: cosmetic on pci_upadte_mappings()

["Michael S. Tsirkin" <mst@redhat.com>]

On Fri, Oct 30, 2009 at 09:21:23PM +0900, Isaku Yamahata wrote:
> Remove one indentation of pci_update_mappings.
> Just for cosmetics, no logic change.

Good stuff. But since you are not afraid of churn for
cosmetics, let's fix a couple more nits?

> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> ---
>  hw/pci.c |  146 ++++++++++++++++++++++++++++++++------------------------------
>  1 files changed, 76 insertions(+), 70 deletions(-)
> 
> diff --git a/hw/pci.c b/hw/pci.c
> index beefae3..b43f6c6 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -637,86 +637,92 @@ static void pci_update_mappings(PCIDevice *d)
>      cmd = pci_get_word(d->config + PCI_COMMAND);
>      for(i = 0; i < PCI_NUM_REGIONS; i++) {
>          r = &d->io_regions[i];
> -        if (r->size != 0) {
> -            if (r->type & PCI_BASE_ADDRESS_SPACE_IO) {
> -                if (cmd & PCI_COMMAND_IO) {
> -                    new_addr = pci_get_long(d->config + pci_bar(d, i));
> -                    new_addr = new_addr & ~(r->size - 1);
> -                    last_addr = new_addr + r->size - 1;
> -                    /* NOTE: we have only 64K ioports on PC */
> -                    if (last_addr <= new_addr || new_addr == 0 ||
> -                        last_addr >= 0x10000) {
> -                        new_addr = PCI_BAR_UNMAPPED;
> -                    }
> -                } else {
> +
> +        /* this region isn't registered */
> +        if (r->size == 0)

Let's replace by !r->size?

> +            continue;
> +
> +        if (r->type & PCI_BASE_ADDRESS_SPACE_IO) {
> +            if (cmd & PCI_COMMAND_IO) {
> +                new_addr = pci_get_long(d->config + pci_bar(d, i));
> +                new_addr = new_addr & ~(r->size - 1);
> +                last_addr = new_addr + r->size - 1;
> +                /* NOTE: we have only 64K ioports on PC */
> +                if (last_addr <= new_addr || new_addr == 0 ||
> +                    last_addr >= 0x10000) {
>                      new_addr = PCI_BAR_UNMAPPED;
>                  }
>              } else {
> -                if (cmd & PCI_COMMAND_MEMORY) {
> -                    if (r->type & PCI_BASE_ADDRESS_MEM_TYPE_64) {
> -                        new_addr = pci_get_quad(d->config + pci_bar(d, i));
> -                    } else {
> -                        new_addr = pci_get_long(d->config + pci_bar(d, i));
> -                    }
> -                    /* the ROM slot has a specific enable bit */
> -                    if (i == PCI_ROM_SLOT && !(new_addr & PCI_ROM_ADDRESS_ENABLE))
> -                        goto no_mem_map;
> -                    new_addr = new_addr & ~(r->size - 1);
> -                    last_addr = new_addr + r->size - 1;
> -                    /* NOTE: we do not support wrapping */
> -                    /* XXX: as we cannot support really dynamic
> -                       mappings, we handle specific values as invalid
> -                       mappings. */
> -                    if (last_addr <= new_addr || new_addr == 0 ||
> -                        last_addr == PCI_BAR_UNMAPPED ||
> -
> -                        /* Now pcibus_t is 64bit.
> -                         * Check if 32 bit BAR wrap around explicitly.
> -                         * Without this, PC ide doesn't work well.
> -                         * TODO: remove this work around.
> -                         */
> -                        (!(r->type & PCI_BASE_ADDRESS_MEM_TYPE_64) &&
> -                         last_addr >= UINT32_MAX) ||
> -
> -                        /*
> -                         * OS is allowed to set BAR beyond its addressable
> -                         * bits. For example, 32 bit OS can set 64bit bar
> -                         * to >4G. Check it.
> -                         */
> -                        last_addr >= TARGET_PHYS_ADDR_MAX) {
> -                        new_addr = PCI_BAR_UNMAPPED;
> -                    }
> +                new_addr = PCI_BAR_UNMAPPED;
> +            }
> +        } else {
> +            if (cmd & PCI_COMMAND_MEMORY) {
> +                if (r->type & PCI_BASE_ADDRESS_MEM_TYPE_64) {
> +                    new_addr = pci_get_quad(d->config + pci_bar(d, i));
>                  } else {
> -                no_mem_map:
> +                    new_addr = pci_get_long(d->config + pci_bar(d, i));
> +                }
> +                /* the ROM slot has a specific enable bit */
> +                if (i == PCI_ROM_SLOT && !(new_addr & PCI_ROM_ADDRESS_ENABLE))
> +                    goto no_mem_map;
> +                new_addr = new_addr & ~(r->size - 1);
> +                last_addr = new_addr + r->size - 1;
> +                /* NOTE: we do not support wrapping */
> +                /* XXX: as we cannot support really dynamic
> +                   mappings, we handle specific values as invalid
> +                   mappings. */
> +                if (last_addr <= new_addr || new_addr == 0 ||
> +                    last_addr == PCI_BAR_UNMAPPED ||
> +
> +                    /* Now pcibus_t is 64bit.
> +                     * Check if 32 bit BAR wrap around explicitly.
> +                     * Without this, PC ide doesn't work well.
> +                     * TODO: remove this work around.
> +                     */
> +                    (!(r->type & PCI_BASE_ADDRESS_MEM_TYPE_64) &&
> +                     last_addr >= UINT32_MAX) ||
> +
> +                    /*
> +                     * OS is allowed to set BAR beyond its addressable
> +                     * bits. For example, 32 bit OS can set 64bit bar
> +                     * to >4G. Check it.
> +                     */
> +                    last_addr >= TARGET_PHYS_ADDR_MAX) {
>                      new_addr = PCI_BAR_UNMAPPED;
>                  }
> +            } else {
> +            no_mem_map:
> +                new_addr = PCI_BAR_UNMAPPED;

Grr goto into scope ...
Can't we get rid of this as well?
Let's just move the whole if() statement to a subfunction,
then we can return PCI_BAR_UNMAPPED instead of goto.



>              }
> -            /* now do the real mapping */
> -            if (new_addr != r->addr) {
> -                if (r->addr != PCI_BAR_UNMAPPED) {
> -                    if (r->type & PCI_BASE_ADDRESS_SPACE_IO) {
> -                        int class;
> -                        /* NOTE: specific hack for IDE in PC case:
> -                           only one byte must be mapped. */
> -                        class = pci_get_word(d->config + PCI_CLASS_DEVICE);
> -                        if (class == 0x0101 && r->size == 4) {
> -                            isa_unassign_ioport(r->addr + 2, 1);
> -                        } else {
> -                            isa_unassign_ioport(r->addr, r->size);
> -                        }
> -                    } else {
> -                        cpu_register_physical_memory(pci_to_cpu_addr(r->addr),
> -                                                     r->size,
> -                                                     IO_MEM_UNASSIGNED);
> -                        qemu_unregister_coalesced_mmio(r->addr, r->size);
> -                    }
> -                }
> -                r->addr = new_addr;
> -                if (r->addr != PCI_BAR_UNMAPPED) {
> -                    r->map_func(d, i, r->addr, r->size, r->type);
> +        }
> +
> +        /* This bar isn't changed */
> +        if (new_addr == r->addr)
> +            continue;
> +
> +        /* now do the real mapping */
> +        if (r->addr != PCI_BAR_UNMAPPED) {
> +            if (r->type & PCI_BASE_ADDRESS_SPACE_IO) {
> +                int class;
> +                /* NOTE: specific hack for IDE in PC case:
> +                   only one byte must be mapped. */
> +                class = pci_get_word(d->config + PCI_CLASS_DEVICE);
> +                if (class == 0x0101 && r->size == 4) {
> +                    isa_unassign_ioport(r->addr + 2, 1);
> +                } else {
> +                    isa_unassign_ioport(r->addr, r->size);
>                  }
> +            } else {
> +                cpu_register_physical_memory(pci_to_cpu_addr(r->addr),
> +                                             r->size,
> +                                             IO_MEM_UNASSIGNED);
> +                qemu_unregister_coalesced_mmio(r->addr, r->size);
>              }
>          }
> +        r->addr = new_addr;
> +        if (r->addr != PCI_BAR_UNMAPPED) {
> +            r->map_func(d, i, r->addr, r->size, r->type);
> +        }
>      }
>  }
>  
> -- 
> 1.6.0.2