[Qemu-devel] Re: [PATCH 16/20] pci: kill goto in pci_update_mappings()

[Isaku Yamahata]

On Thu, Nov 12, 2009 at 02:06:22PM +0200, Michael S. Tsirkin wrote:
> On Thu, Nov 12, 2009 at 02:58:44PM +0900, Isaku Yamahata wrote:
> > This patch kills nasty goto in pci_update_mappings().
> > 
> > Signed-off-by: Isaku Yamahata <address@hidden>
> > ---
> >  hw/pci.c |   54 ++++++++++++++++++++++++++++--------------------------
> >  1 files changed, 28 insertions(+), 26 deletions(-)
> > 
> > diff --git a/hw/pci.c b/hw/pci.c
> > index cae3d53..2eff7fe 100644
> > --- a/hw/pci.c
> > +++ b/hw/pci.c
> > @@ -756,35 +756,37 @@ static void pci_update_mappings(PCIDevice *d)
> >                      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 ||
> 
> By the way, any idea why do we need new_addr == 0
> and last_addr == PCI_BAR_UNMAPPED?

As for new_addr == 0, since default BAR value is zero, plural BARs can
overlap with each other. I think qemu can't handle BAR overlap anyway.

As for last_addr == PCI_BAR_UNMAPPED, it avoid to map
around 4GB as discussed before. I observed that guest doesn't boot
without the check. However I didn't track it down further.
Now it's 64bit though.

> What would it take to fix these?
> 
> > -                    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) {
> > +                if (i == PCI_ROM_SLOT &&
> > +                    !(new_addr & PCI_ROM_ADDRESS_ENABLE)) {
> >                      new_addr = PCI_BAR_UNMAPPED;
> > +                } else {
> > +                    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;
> >              }
> >          }
> 
> Nesting is too deep in pci_update_mappings already.
> And this makes it worse. I split out the math into
> a subfunction, and it looks better IMO:
> 
> --->
> 
> From: Michael S. Tsirkin <address@hidden>
> Subject: pci: split up up pci_update mappings
> 
> Split bar address math into a separate function.
> In particular, this gets rid of an ugly forward goto
> into scope that we have there.
> 
> Signed-off-by: Michael S. Tsirkin <address@hidden>

Makes sense. Much more readable.
Acked-by: Isaku Yamahata <address@hidden>


> 
> ---
> 
> diff --git a/hw/pci.c b/hw/pci.c
> index 380d43c..847d31e 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -720,14 +720,77 @@ static void pci_bridge_filter(PCIDevice *d, pcibus_t 
> *addr, pcibus_t *size,
>      }
>  }
>  
> +static pcibus_t pci_bar_address(PCIDevice *d,
> +                             int reg, uint8_t type, pcibus_t size)
> +{
> +    pcibus_t new_addr, last_addr;
> +    int bar = pci_bar(d, reg);
> +    uint16_t cmd = pci_get_word(d->config + PCI_COMMAND);
> +
> +    if (type & PCI_BASE_ADDRESS_SPACE_IO) {
> +        if (!(cmd & PCI_COMMAND_IO)) {
> +            return PCI_BAR_UNMAPPED;
> +        }
> +        new_addr = pci_get_long(d->config + bar) & ~(size - 1);
> +        last_addr = new_addr + size - 1;
> +        /* NOTE: we have only 64K ioports on PC */
> +        if (last_addr <= new_addr || new_addr == 0 || last_addr > 
> UINT16_MAX) {
> +            return PCI_BAR_UNMAPPED;
> +        }
> +        return new_addr;
> +    }
> +
> +    if (!(cmd & PCI_COMMAND_MEMORY)) {
> +        return PCI_BAR_UNMAPPED;
> +    }
> +    if (type & PCI_BASE_ADDRESS_MEM_TYPE_64) {
> +        new_addr = pci_get_quad(d->config + bar);
> +    } else {
> +        new_addr = pci_get_long(d->config + bar);
> +    }
> +    /* the ROM slot has a specific enable bit */
> +    if (reg == PCI_ROM_SLOT && !(new_addr & PCI_ROM_ADDRESS_ENABLE)) {
> +        return PCI_BAR_UNMAPPED;
> +    }
> +    new_addr &= ~(size - 1);
> +    last_addr = new_addr + 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) {
> +        return PCI_BAR_UNMAPPED;
> +    }
> +
> +    /* Now pcibus_t is 64bit.
> +     * Check if 32 bit BAR wraps around explicitly.
> +     * Without this, PC ide doesn't work well.
> +     * TODO: remove this work around.
> +     */
> +    if  (!(type & PCI_BASE_ADDRESS_MEM_TYPE_64) && last_addr >= UINT32_MAX) {
> +        return PCI_BAR_UNMAPPED;
> +    }
> +
> +    /*
> +     * OS is allowed to set BAR beyond its addressable
> +     * bits. For example, 32 bit OS can set 64bit bar
> +     * to >4G. Check it. TODO: we might need to support
> +     * it in the future for e.g. PAE.
> +     */
> +    if (last_addr >= TARGET_PHYS_ADDR_MAX) {
> +        return PCI_BAR_UNMAPPED;
> +    }
> +
> +    return new_addr;
> +}
> +
>  static void pci_update_mappings(PCIDevice *d)
>  {
>      PCIIORegion *r;
> -    int cmd, i;
> -    pcibus_t last_addr, new_addr;
> -    pcibus_t filtered_size;
> +    int i;
> +    pcibus_t new_addr, filtered_size;
>  
> -    cmd = pci_get_word(d->config + PCI_COMMAND);
>      for(i = 0; i < PCI_NUM_REGIONS; i++) {
>          r = &d->io_regions[i];
>  
> @@ -735,59 +798,7 @@ static void pci_update_mappings(PCIDevice *d)
>          if (!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 {
> -                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;
> -                }
> -            } else {
> -            no_mem_map:
> -                new_addr = PCI_BAR_UNMAPPED;
> -            }
> -        }
> +        new_addr = pci_bar_address(d, i, r->type, r->size);
>  
>          /* bridge filtering */
>          filtered_size = r->size;
> 

-- 
yamahata