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

[Alexey Korolev]

On 29/12/11 00:43, Michael S. Tsirkin wrote:
> 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
 It was my first implementation. Unfortunately older versions of Linux 
(Like 2.6.18) hang during startup with this.
As far as I remember it was qemu-0.15 so may be 1.0 have no such an 
issue. I will check this.
> 2. Put the 32 bit one in the non-prefetcheable range
I'd rather not do this. Bios should not change memory region types. It 
will confuse guest OS drivers.
> 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<address@hidden>
> Whitespace is corrupted: checkyour mail setup?
> There should be spaces around operators:
> a<  b, I see a<  b. Sometimes a<   b (two spaces after).
Yes, it's thunderbird :(. Sorry about that.
It seems the patches need to have some functional changes anyway.
> > ---
> >   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?
Bios will panic if it founds prefmem BARs in both 32bit and 64bit areas.
Otherwise we will pick one which exists or 32bit one if both are not 
present.

Thanks
Alexey