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 <yamah...@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
