[Qemu-devel] Re: [PATCH V6 31/32] pci: implement pci bridge filtering.

[Michael S. Tsirkin]

On Fri, Oct 30, 2009 at 09:21:25PM +0900, Isaku Yamahata wrote:
> This patch implements pci bridge filtering.
> 
> TODO: currently almost all the map funcions assumes
> filtered_size == size and addr & ~(size - 1) == addr.
> However with bridge filtering, they aren't always true.
> Teach them such cases, such that filtered_size < size and
> addr & (size - 1) != 0.
> 
> Signed-off-by: Isaku Yamahata <address@hidden>

Thanks for looking into this, good stuff.
A couple of minor nits.

> ---
>  hw/pci.c |  155 
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
>  hw/pci.h |    3 +
>  2 files changed, 151 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/pci.c b/hw/pci.c
> index d565f6e..336c1a3 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -560,10 +560,10 @@ static void pci_unregister_io_regions(PCIDevice 
> *pci_dev)
>          if (!r->size || r->addr == PCI_BAR_UNMAPPED)
>              continue;
>          if (r->type == PCI_BASE_ADDRESS_SPACE_IO) {
> -            isa_unassign_ioport(r->addr, r->size);
> +            isa_unassign_ioport(r->addr, r->filtered_size);
>          } else {
>              cpu_register_physical_memory(pci_to_cpu_addr(r->addr),
> -                                                     r->size,
> +                                                     r->filtered_size,
>                                                       IO_MEM_UNASSIGNED);
>          }
>      }
> @@ -608,6 +608,7 @@ void pci_register_bar(PCIDevice *pci_dev, int region_num,
>      r = &pci_dev->io_regions[region_num];
>      r->addr = PCI_BAR_UNMAPPED;
>      r->size = size;
> +    r->filtered_size = size;
>      r->type = type;
>      r->map_func = map_func;
>  
> @@ -628,11 +629,111 @@ void pci_register_bar(PCIDevice *pci_dev, int 
> region_num,
>      }
>  }
>  
> +static uint32_t pci_config_get_io_base(PCIDevice *d,
> +                                       uint32_t base, uint32_t base_upper16)
> +{

base_upper16 unused?

> +    uint32_t val;
> +
> +    val = ((uint32_t)d->config[base] & PCI_IO_RANGE_MASK) << 8;
> +    if (d->config[base] & PCI_IO_RANGE_TYPE_32) {
> +        val |= (uint32_t)pci_get_word(d->config + PCI_IO_BASE_UPPER16) << 16;
> +    }
> +    return val;
> +}
> +
> +static uint64_t pci_config_get_memory_base(PCIDevice *d, uint32_t base)
> +{
> +    return ((uint64_t)pci_get_word(d->config + base) & PCI_MEMORY_RANGE_MASK)
> +        << 16;
> +}
> +
> +static uint64_t pci_config_get_pref_base(PCIDevice *d,
> +                                         uint32_t base, uint32_t upper)
> +{
> +    uint64_t val;
> +    val = ((uint64_t)pci_get_word(d->config + base) &
> +           PCI_PREF_RANGE_MASK) << 16;
> +    val |= (uint64_t)pci_get_long(d->config + upper) << 32;
> +    return val;
> +}

When is it 64 bit and when pcibus_t?
Also, bus might not support 64 bit addressing: need to check?


> +
> +static pcibus_t pci_bridge_get_base(PCIDevice *bridge, uint8_t type)
> +{
> +    pcibus_t base;
> +    if (type & PCI_BASE_ADDRESS_SPACE_IO) {
> +        base = pci_config_get_io_base(bridge,
> +                                      PCI_IO_BASE, PCI_IO_BASE_UPPER16);
> +    } else {
> +        if (type & PCI_BASE_ADDRESS_MEM_PREFETCH) {
> +            base = pci_config_get_pref_base(
> +                bridge, PCI_PREF_MEMORY_BASE, PCI_PREF_BASE_UPPER32);
> +        } else {
> +            base = pci_config_get_memory_base(bridge, PCI_MEMORY_BASE);
> +        }
> +    }
> +
> +    return base;
> +}
> +
> +static pcibus_t pci_bridge_get_limit(PCIDevice *bridge, uint8_t type)
> +{
> +    pcibus_t limit;
> +    if (type & PCI_BASE_ADDRESS_SPACE_IO) {
> +        limit = pci_config_get_io_base(bridge,
> +                                      PCI_IO_LIMIT, PCI_IO_LIMIT_UPPER16);
> +        limit |= 0xfff;         /* PCI bridge spec 3.2.5.6. */
> +    } else {
> +        if (type & PCI_BASE_ADDRESS_MEM_PREFETCH) {
> +            limit = pci_config_get_pref_base(
> +                bridge, PCI_PREF_MEMORY_LIMIT, PCI_PREF_LIMIT_UPPER32);
> +        } else {
> +            limit = pci_config_get_memory_base(bridge, PCI_MEMORY_LIMIT);
> +        }
> +        limit |= 0xfffff;       /* PCI bridge spec 3.2.5.{1, 8}. */
> +    }
> +    return limit;
> +}
> +
> +static void pci_bridge_filter(PCIDevice *d, pcibus_t *addr, pcibus_t *size,
> +                              uint8_t type)
> +{
> +    pcibus_t base = *addr;
> +    pcibus_t limit = *addr + *size - 1;
> +    PCIDevice *br;
> +
> +    for (br = d->bus->parent_dev; br; br = br->bus->parent_dev) {
> +        uint16_t cmd = pci_get_word(d->config + PCI_COMMAND);
> +
> +        if (type & PCI_BASE_ADDRESS_SPACE_IO) {
> +            if (!(cmd & PCI_COMMAND_IO)) {
> +                goto no_map;
> +            }
> +        } else {
> +            if (!(cmd & PCI_COMMAND_MEMORY)) {
> +                goto no_map;
> +            }
> +        }
> +
> +        base = MAX(base, pci_bridge_get_base(br, type));
> +        limit = MIN(limit, pci_bridge_get_limit(br, type));
> +    }
> +
> +    if (base > limit) {
> +    no_map:

Could you code this without goto into scope please?
Using subfunctions usually helps.

> +        *addr = PCI_BAR_UNMAPPED;
> +        *size = 0;
> +    } else {
> +        *addr = base;
> +        *size = limit - base + 1;
> +    }
> +}
> +
>  static void pci_update_mappings(PCIDevice *d)
>  {
>      PCIIORegion *r;
>      int cmd, i;
>      pcibus_t last_addr, new_addr;
> +    pcibus_t filtered_size;
>  
>      cmd = pci_get_word(d->config + PCI_COMMAND);
>      for(i = 0; i < PCI_NUM_REGIONS; i++) {
> @@ -696,8 +797,14 @@ static void pci_update_mappings(PCIDevice *d)
>              }
>          }
>  
> +        /* bridge filtering */
> +        filtered_size = r->size;
> +        if (new_addr != PCI_BAR_UNMAPPED) {
> +            pci_bridge_filter(d, &new_addr, &filtered_size, r->type);
> +        }
> +
>          /* This bar isn't changed */
> -        if (new_addr == r->addr)
> +        if (new_addr == r->addr && filtered_size == r->filtered_size)
>              continue;
>  
>          /* now do the real mapping */
> @@ -710,18 +817,26 @@ static void pci_update_mappings(PCIDevice *d)
>                  if (class == 0x0101 && r->size == 4) {
>                      isa_unassign_ioport(r->addr + 2, 1);
>                  } else {
> -                    isa_unassign_ioport(r->addr, r->size);
> +                    isa_unassign_ioport(r->addr, r->filtered_size);
>                  }
>              } else {
>                  cpu_register_physical_memory(pci_to_cpu_addr(r->addr),
> -                                             r->size,
> +                                             r->filtered_size,
>                                               IO_MEM_UNASSIGNED);
> -                qemu_unregister_coalesced_mmio(r->addr, r->size);
> +                qemu_unregister_coalesced_mmio(r->addr, r->filtered_size);
>              }
>          }
>          r->addr = new_addr;
> +        r->filtered_size = filtered_size;
>          if (r->addr != PCI_BAR_UNMAPPED) {
> -            r->map_func(d, i, r->addr, r->size, r->type);
> +            /*
> +             * TODO: currently almost all the map funcions assumes
> +             * filtered_size == size and addr & ~(size - 1) == addr.
> +             * However with bridge filtering, they aren't always true.
> +             * Teach them such cases, such that filtered_size < size and
> +             * addr & (size - 1) != 0.
> +             */
> +            r->map_func(d, i, r->addr, r->filtered_size, r->type);
>          }
>      }
>  }
> @@ -994,10 +1109,36 @@ typedef struct {
>      uint32_t did;
>  } PCIBridge;
>  
> +
> +static void pci_bridge_update_mappings_fn(PCIBus *b, PCIDevice *d)
> +{
> +    pci_update_mappings(d);
> +}
> +
> +static void pci_bridge_update_mappings(PCIBus *b)
> +{
> +    PCIBus *child;
> +
> +    pci_for_each_device_under_bus(b, pci_bridge_update_mappings_fn);
> +
> +    QLIST_FOREACH(child, &b->child, sibling) {
> +        pci_bridge_update_mappings(child);
> +    }
> +}
> +
>  static void pci_bridge_write_config(PCIDevice *d,
>                               uint32_t address, uint32_t val, int len)
>  {
>      pci_default_write_config(d, address, val, len);
> +
> +    if (/* io base/limit */
> +        ranges_overlap(address, len, PCI_IO_BASE, 2) ||
> +
> +        /* memory base/limit, prefetchable base/limit and
> +           io base/limit upper 16 */
> +        ranges_overlap(address, len, PCI_MEMORY_BASE, 20)) {
> +        pci_bridge_update_mappings(d->bus);
> +    }
>  }
>  
>  PCIBus *pci_find_bus(PCIBus *bus, int bus_num)
> diff --git a/hw/pci.h b/hw/pci.h
> index 3f50294..9a56d0d 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -86,6 +86,7 @@ typedef struct PCIIORegion {
>      pcibus_t addr; /* current PCI mapping address. -1 means not mapped */
>  #define PCI_BAR_UNMAPPED (~(pcibus_t)0)
>      pcibus_t size;
> +    pcibus_t filtered_size;
>      uint8_t type;
>      PCIMapIORegionFunc *map_func;
>  } PCIIORegion;
> @@ -130,6 +131,7 @@ typedef struct PCIIORegion {
>  #define PCI_SEC_LATENCY_TIMER   0x1b    /* Latency timer for secondary 
> interface */
>  #define PCI_IO_BASE             0x1c    /* I/O range behind the bridge */
>  #define PCI_IO_LIMIT            0x1d
> +#define  PCI_IO_RANGE_TYPE_32        0x01
>  #define  PCI_IO_RANGE_MASK      (~0x0fUL)
>  #define PCI_SEC_STATUS               0x1e    /* Secondary status register, 
> only bit 14 used */
>  #define PCI_MEMORY_BASE         0x20    /* Memory range behind */
> @@ -139,6 +141,7 @@ typedef struct PCIIORegion {
>  #define PCI_PREF_MEMORY_LIMIT   0x26
>  #define  PCI_PREF_RANGE_MASK    (~0x0fUL)
>  #define PCI_PREF_BASE_UPPER32   0x28    /* Upper half of prefetchable memory 
> range */
> +#define PCI_PREF_LIMIT_UPPER32       0x2c
>  #define PCI_SUBSYSTEM_VENDOR_ID 0x2c    /* 16 bits */
>  #define PCI_SUBSYSTEM_ID        0x2e    /* 16 bits */
>  #define PCI_ROM_ADDRESS              0x30    /* Bits 31..11 are address, 
> 10..1 reserved */
> -- 
> 1.6.0.2