Re: [Qemu-devel] [PATCH 2/3] Add a new PCI region type to supports 64 bit ranges

[Kevin O'Connor <kevin@koconnor.net>]

On Wed, Dec 28, 2011 at 06:26:05PM +1300, Alexey Korolev wrote:
> This patch adds PCI_REGION_TYPE_PREFMEM_64 region type and modifies types of
> variables to make it possible to work with 64 bit addresses.
> 
> Why I've added just one region type PCI_REGION_TYPE_PREFMEM_64 and haven't
> added PCI_REGION_TYPE_MEM_64? According to PCI architecture
> specification, the
> bridges can describe 64bit ranges for prefetchable type of memory
> only. So it's very
> unlikely that devices exporting 64bit non-prefetchable BARs. Anyway
> this code will work
> with 64bit non-prefetchable BARs unless the PCI device is not behind
> the secondary bus.
[...]
> --- a/src/pciinit.c
> +++ b/src/pciinit.c
> @@ -22,6 +22,7 @@ enum pci_region_type {
>      PCI_REGION_TYPE_IO,
>      PCI_REGION_TYPE_MEM,
>      PCI_REGION_TYPE_PREFMEM,
> +    PCI_REGION_TYPE_PREFMEM_64,
>      PCI_REGION_TYPE_COUNT,
>  };

This doesn't seem right.  A 64bit bar is not a new category - it's
just a way of representing larger values within the existing
categories.  Tracking of 64bit prefmem sections separately from
regular prefmem sections doesn't make sense, because both need to be
allocated from the same pool when behind a bridge.

>  struct pci_bus {
>      struct {
> -        /* pci region stats */
> -        u32 count[32 - PCI_MEM_INDEX_SHIFT];
> -        u32 sum, max;
>          /* seconday bus region sizes */
>          u32 size;
> -        /* pci region assignments */
> -        u32 bases[32 - PCI_MEM_INDEX_SHIFT];
> -        u32 base;
> +        /* pci region stats */
> +        u32 max;
> +        u32 count[32 - PCI_MEM_INDEX_SHIFT];
> +        s64 sum;
> +         /* pci region assignments */
> +        s64 base;
> +        s64 bases[32 - PCI_MEM_INDEX_SHIFT];

Why the choice of s64 over u64?  Given the amount of bit manipulation
on these values, I think using u64 would be safer.

> @@ -69,6 +72,8 @@ static enum pci_region_type pci_addr_to_type(u32 addr)
>  {
>      if (addr & PCI_BASE_ADDRESS_SPACE_IO)
>          return PCI_REGION_TYPE_IO;
> +    if (addr & PCI_BASE_ADDRESS_MEM_TYPE_64)
> +        return PCI_REGION_TYPE_PREFMEM_64;

This seems dangerous - a 64bit bar can be non-prefetchable - getting
this wrong could cause random (hard to debug) crashes.

> @@ -378,19 +383,16 @@ static void pci_bios_check_devices(struct
> pci_bus *busses)
>          struct pci_bus *bus = &busses[pci_bdf_to_bus(pci->bdf)];
>          int i;
>          for (i = 0; i < PCI_NUM_REGIONS; i++) {
> -            u32 val, size;
> +            u32 val, size, type;
>              pci_bios_get_bar(pci, i, &val, &size);
>              if (val == 0)
>                  continue;
> 
> -            pci_bios_bus_reserve(bus, pci_addr_to_type(val), size);
> +            type = pci_addr_to_type(val);
> +            pci_bios_bus_reserve(bus, type, size);
>              pci->bars[i].addr = val;
>              pci->bars[i].size = size;
> -            pci->bars[i].is64 = (!(val & PCI_BASE_ADDRESS_SPACE_IO) &&
> -                                 (val & PCI_BASE_ADDRESS_MEM_TYPE_MASK)
> -                                 == PCI_BASE_ADDRESS_MEM_TYPE_64);
> -
> -            if (pci->bars[i].is64)
> +            if (type == PCI_REGION_TYPE_PREFMEM_64)
>                  i++;

If there is a 64bit bar, then the size could be over 32bits - the code
really needs to handle this (or it could cause overlapping regions
which result in random crashes).

-Kevin