| Message ID | 20091112120622.GB11106@redhat.com |
|---|---|
| State | New |
| Headers | show |
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; >
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
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).
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;