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

[Alexey Korolev]

On 29/12/11 15:56, Kevin O'Connor wrote:
> 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.
It's a way to account all memory sections on the root bus, as
on the root bus we can have all 4 regions.
I don't like this part as well, it causes confusions.

One possible solution is to have different descriptors for secondary buses
and the root bus.  In that case we can have 3 sections per secondary bus
and the root bus will contain memory regions without any 'physical' meaning.

> >   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.
No problem.
Addresses could not exceed 40bit's so we basically may touch the last bit
only when negative value is stored.
> > @@ -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.
It is bit confusing but this doesn't touch actual types. So even if
we have a 64bit not-prefetchable BAR code will be working.
> > @@ -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).
Agree, something has hold in my mind that BAR size it is limited to 4GB,
but just looked into PCI spec - there are no limitations stated.
Well this requires bigger changes, as 64bit BAR size accounting touches
more computations comparing to 64bit BAR addresses.

It makes sense to figure out how shall we account all memory sections on 
the root bus.
Will it be separated structures for root bus and secondary buses? Do you 
have another idea?