[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Qemu-devel] Re: [PATCH 16/20] pci: kill goto in pci_update_mappings()
From: |
Michael S. Tsirkin |
Subject: |
[Qemu-devel] Re: [PATCH 16/20] pci: kill goto in pci_update_mappings() |
Date: |
Thu, 12 Nov 2009 14:06:22 +0200 |
User-agent: |
Mutt/1.5.19 (2009-01-05) |
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?
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>
---
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;
- [Qemu-devel] [PATCH 05/20] pci: rename pci_addr_to_dev(), pcie_mmcfg_addr_to_dev()., (continued)
- [Qemu-devel] [PATCH 05/20] pci: rename pci_addr_to_dev(), pcie_mmcfg_addr_to_dev()., Isaku Yamahata, 2009/11/12
- [Qemu-devel] [PATCH 11/20] pci: clean up of pci_init_wmask()., Isaku Yamahata, 2009/11/12
- [Qemu-devel] [PATCH 07/20] pci: remove pci_sub_bus() by open coding., Isaku Yamahata, 2009/11/12
- [Qemu-devel] [PATCH 18/20] pci: fix pci_config_get_io_base()., Isaku Yamahata, 2009/11/12
- [Qemu-devel] [PATCH 16/20] pci: kill goto in pci_update_mappings(), Isaku Yamahata, 2009/11/12
- [Qemu-devel] Re: [PATCH 16/20] pci: kill goto in pci_update_mappings(),
Michael S. Tsirkin <=
- [Qemu-devel] [PATCH 14/20] pci: remove unused constants., Isaku Yamahata, 2009/11/12
- [Qemu-devel] [PATCH 19/20] pci: pci bridge related clean up., Isaku Yamahata, 2009/11/12
- [Qemu-devel] [PATCH 13/20] pci: move typedef, PCIHostState, PCIExpressHost to qemu-common.h., Isaku Yamahata, 2009/11/12
- [Qemu-devel] [PATCH 20/20] pci: remove goto in pci_bridge_filter()., Isaku Yamahata, 2009/11/12