Re: [Qemu-devel] [PATCH 3/3] Changes related to secondary buses and 64bit regions

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

On Wed, Dec 28, 2011 at 06:35:55PM +1300, Alexey Korolev wrote:
> All devices behind a bridge need to have all their regions consecutive and
> not overlapping with all the normal memory ranges.
> Since prefetchable memory is described by one record, we must avoid the situations
> when 32bit and 64bit prefetchable regions are present within one secondary bus.

How do we avoid this? Assume we have two devices:
a 32 bit and a 64 bit one, behind a bridge.
There are two main things we can do:
1. Make the 64 bit device only use the low 32 bit
2. Put the 32 bit one in the non-prefetcheable range

1 probably makes more sense for small BARs
2 probably makes more sense for large ones

Try also looking at e.g. linux bus scanning code for more ideas.

Another thing I don't see addressed here is that support for 64 bit
ranges is I think optional in the bridge.

> 
> Signed-off-by: Alexey Korolev<alexey.korolev@endace.com>

Whitespace is corrupted: checkyour mail setup?
There should be spaces around operators:
a < b, I see a< b. Sometimes a<  b (two spaces after).

> ---
>  src/pciinit.c |   69 +++++++++++++++++++++++++++++++++++++++-----------------
>  1 files changed, 48 insertions(+), 21 deletions(-)
> 
> diff --git a/src/pciinit.c b/src/pciinit.c
> index a574e38..92942d5 100644
> --- a/src/pciinit.c
> +++ b/src/pciinit.c
> @@ -17,6 +17,7 @@
> 
>  #define PCI_BRIDGE_IO_MIN      0x1000
>  #define PCI_BRIDGE_MEM_MIN   0x100000
> +#define PCI_BRIDGE_MEM_MAX   0x80000000
> 
>  enum pci_region_type {
>      PCI_REGION_TYPE_IO,
> @@ -45,6 +46,7 @@ struct pci_bus {
>          s64 base;
>          s64 bases[32 - PCI_MEM_INDEX_SHIFT];
>      } r[PCI_REGION_TYPE_COUNT];
> +    int is64;
>      struct pci_device *bus_dev;
>  };
> 
> @@ -369,6 +371,26 @@ static void pci_bios_bus_reserve(struct pci_bus *bus, int type, u32 size)
>          bus->r[type].max = size;
>  }
> 
> +static void pci_bios_secondary_bus_reserve(struct pci_bus *parent,
> +                                           struct pci_bus *s, int type)
> +{
> +    u32 limit = (type == PCI_REGION_TYPE_IO) ?
> +        PCI_BRIDGE_IO_MIN : PCI_BRIDGE_MEM_MIN;
> +
> +    if (s->r[type].sum>  PCI_BRIDGE_MEM_MAX) {
> +        panic("Size: %08x%08x is too big\n",
> +                (u32)s->r[type].sum, (u32)(s->r[type].sum>>32));
> +    }
> +    s->r[type].size = (u32)s->r[type].sum;
> +    if (s->r[type].size<  limit)
> +        s->r[type].size = limit;
> +    s->r[type].size = pci_size_roundup(s->r[type].size);
> +
> +    pci_bios_bus_reserve(parent, type, s->r[type].size);
> +    dprintf(1, "    size: %x, type %s\n",
> +                s->r[type].size, region_type_name[type]);
> +}
> +
>  static void pci_bios_check_devices(struct pci_bus *busses)
>  {
>      dprintf(1, "PCI: check devices\n");
> @@ -392,8 +414,10 @@ static void pci_bios_check_devices(struct pci_bus *busses)
>              pci_bios_bus_reserve(bus, type, size);
>              pci->bars[i].addr = val;
>              pci->bars[i].size = size;
> -            if (type == PCI_REGION_TYPE_PREFMEM_64)
> +            if (type == PCI_REGION_TYPE_PREFMEM_64) {
> +                bus->is64 = 1;
>                  i++;
> +            }
>          }
>      }
> 
> @@ -404,22 +428,21 @@ static void pci_bios_check_devices(struct pci_bus *busses)
>          if (!s->bus_dev)
>              continue;
>          struct pci_bus *parent =&busses[pci_bdf_to_bus(s->bus_dev->bdf)];
> +
> +        if (s->r[PCI_REGION_TYPE_PREFMEM_64].sum&&

Space before && here and elsewhere.

> +                   s->r[PCI_REGION_TYPE_PREFMEM].sum) {
> +           panic("Sparse PCI prefmem regions on the bus %d\n", secondary_bus);
> +        }
> +
> +        dprintf(1, "PCI: secondary bus %d\n", secondary_bus);
>          int type;
>          for (type = 0; type<  PCI_REGION_TYPE_COUNT; type++) {
> -            u32 limit = (type == PCI_REGION_TYPE_IO) ?
> -                PCI_BRIDGE_IO_MIN : PCI_BRIDGE_MEM_MIN;
> -            s->r[type].size = s->r[type].sum;
> -            if (s->r[type].size<  limit)
> -                s->r[type].size = limit;
> -            s->r[type].size = pci_size_roundup(s->r[type].size);
> -            pci_bios_bus_reserve(parent, type, s->r[type].size);
> -        }
> -        dprintf(1, "PCI: secondary bus %d sizes: io %x, mem %x, prefmem %x\n",
> -                secondary_bus,
> -                s->r[PCI_REGION_TYPE_IO].size,
> -                s->r[PCI_REGION_TYPE_MEM].size,
> -                s->r[PCI_REGION_TYPE_PREFMEM].size);
> -    }
> +            if ((type == PCI_REGION_TYPE_PREFMEM_64&&  !s->is64) ||
> +                (type == PCI_REGION_TYPE_PREFMEM&&  s->is64))
> +                continue;

Can't figure this out. What does this do?

> +            pci_bios_secondary_bus_reserve(parent, s, type);
> +       }
> +   }
>  }
> 
>  #define ROOT_BASE(top, sum, max) ALIGN_DOWN((top)-(sum),(max) ?: 1)
> @@ -507,14 +530,17 @@ static void pci_bios_map_devices(struct pci_bus *busses)
>          struct pci_bus *parent =&busses[pci_bdf_to_bus(bdf)];
>          int type;
>          for (type = 0; type<  PCI_REGION_TYPE_COUNT; type++) {
> +            if ((type == PCI_REGION_TYPE_PREFMEM_64&&  !s->is64) ||
> +                (type == PCI_REGION_TYPE_PREFMEM&&  s->is64))
> +                continue;
>              s->r[type].base = pci_bios_bus_get_addr(
>                  parent, type, s->r[type].size);
>          }
>          dprintf(1, "PCI: init bases bus %d (secondary)\n", secondary_bus);
>          pci_bios_init_bus_bases(s);
> 
> -        u32 base = s->r[PCI_REGION_TYPE_IO].base;
> -        u32 limit = base + s->r[PCI_REGION_TYPE_IO].size - 1;
> +        s64 base = s->r[PCI_REGION_TYPE_IO].base;
> +        s64 limit = base + s->r[PCI_REGION_TYPE_IO].size - 1;
>          pci_config_writeb(bdf, PCI_IO_BASE, base>>  PCI_IO_SHIFT);
>          pci_config_writew(bdf, PCI_IO_BASE_UPPER16, 0);
>          pci_config_writeb(bdf, PCI_IO_LIMIT, limit>>  PCI_IO_SHIFT);
> @@ -525,12 +551,13 @@ static void pci_bios_map_devices(struct pci_bus *busses)
>          pci_config_writew(bdf, PCI_MEMORY_BASE, base>>  PCI_MEMORY_SHIFT);
>          pci_config_writew(bdf, PCI_MEMORY_LIMIT, limit>>  PCI_MEMORY_SHIFT);
> 
> -        base = s->r[PCI_REGION_TYPE_PREFMEM].base;
> -        limit = base + s->r[PCI_REGION_TYPE_PREFMEM].size - 1;
> +        type = s->is64 ? PCI_REGION_TYPE_PREFMEM_64 : PCI_REGION_TYPE_PREFMEM;
> +        base = s->r[type].base;
> +        limit = base + s->r[type].size - 1;
>          pci_config_writew(bdf, PCI_PREF_MEMORY_BASE, base>>  PCI_PREF_MEMORY_SHIFT);
>          pci_config_writew(bdf, PCI_PREF_MEMORY_LIMIT, limit>>  PCI_PREF_MEMORY_SHIFT);
> -        pci_config_writel(bdf, PCI_PREF_BASE_UPPER32, 0);
> -        pci_config_writel(bdf, PCI_PREF_LIMIT_UPPER32, 0);
> +        pci_config_writel(bdf, PCI_PREF_BASE_UPPER32, base>>  32);
> +        pci_config_writel(bdf, PCI_PREF_LIMIT_UPPER32, limit>>  32);
>      }
> 
>      // Map regions on each device.
> -- 
> 1.7.5.4
>