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

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

On Thu, Nov 12, 2009 at 10:12:07PM +0900, Isaku Yamahata wrote:
> 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 <yamahata@valinux.co.jp>
> > > ---
> > >  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.

Heh, for that matter, BARs can overlap anyway.
0 just makes it more likely.

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

Hmm. Would be nice to figure all this out.

> > 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 <mst@redhat.com>
> > 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 <mst@redhat.com>
> 
> Makes sense. Much more readable.
> Acked-by: Isaku Yamahata <yamahata@valinux.co.jp>
> 
> 
> > 
> > ---
> > 
> > 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