Patchwork Re: [PATCH 16/20] pci: kill goto in pci_update_mappings()

login
register
mail settings
Submitter Michael S. Tsirkin
Date Nov. 12, 2009, 12:06 p.m.
Message ID <20091112120622.GB11106@redhat.com>
Download mbox | patch
Permalink /patch/38242/
State New
Headers show

Comments

Michael S. Tsirkin - Nov. 12, 2009, 12:06 p.m.
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?
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>

---
Isaku Yamahata - Nov. 12, 2009, 1:12 p.m.
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.

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 <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;
>
Michael S. Tsirkin - Nov. 12, 2009, 1:13 p.m.
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
Michael S. Tsirkin - Nov. 12, 2009, 1:29 p.m.
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.
> 
> As for last_addr == PCI_BAR_UNMAPPED, it avoid to map
> around 4GB as discussed before.

So it should be ~0x0ull and not PCI_BAR_UNMAPPED,
PCI_BAR_UNMAPPED could be any value e.g. 0x1 would do
as well.

> I observed that guest doesn't boot
> without the check. However I didn't track it down further.
> Now it's 64bit though.

I really think we need to move mapping regions out of devices
and into pci.c. Then we can finally support overlapping BARs
(by being careful not to map common regions).

Patch

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;