[SeaBIOS] [PATCHv2] pci: load memory window setup from host

[Michael S. Tsirkin]

On Sun, Jul 14, 2013 at 06:51:11PM -0400, Kevin O'Connor wrote:
> On Tue, Apr 30, 2013 at 09:34:55AM +0300, Michael S. Tsirkin wrote:
> > Load memory window setup for pci from host.
> > This makes it possible for host to make sure
> > setup matches hardware exactly: especially important
> > for when ACPI tables are loaded from host.
> > This will also make it easier to add more chipsets
> > down the road.
> 
> I'm struggling to understand this patch.  I think the code may need
> some refactoring.
> 
> [...]
> > --- a/src/pciinit.c
> > +++ b/src/pciinit.c
> > @@ -13,6 +13,7 @@
> >  #include "config.h" // CONFIG_*
> >  #include "memmap.h" // add_e820
> >  #include "paravirt.h" // RamSize
> > +#include "byteorder.h" // le64_to_cpu
> >  #include "dev-q35.h"
> >  
> >  /* PM Timer ticks per second (HZ) */
> > @@ -61,6 +62,13 @@ struct pci_bus {
> >      struct pci_device *bus_dev;
> >  };
> >  
> > +struct pci_mem {
> > +	u64 start32;
> 
> SeaBIOS uses spaces; not tabs.
> 
> [...]
> >  void i440fx_mem_addr_setup(struct pci_device *dev, void *arg)
> >  {
> > +    if (arg)
> > +	/* use supplied memory */;
> 
> Huh?
> 
> [...]
> >  /****************************************************************
> >   * Main setup code
> >   ****************************************************************/
> > -
> >  void
> >  pci_setup(void)
> >  {
> > @@ -823,25 +868,39 @@ pci_setup(void)
> >      dprintf(1, "=== PCI device probing ===\n");
> >      pci_probe_devices();
> >  
> > -    pcimem_start = RamSize;
> > -    pci_bios_init_platform();
> > +    struct pci_mem *mem = pci_mem_get();
> > +
> > +    if (mem) {
> > +        pcimem_start = mem->start32;
> > +        pcimem_end = mem->end32;
> > +        pcimem64_start = mem->start64;
> > +        pcimem64_end = mem->end64;
> > +    } else {
> > +        pcimem_start = RamSize;
> > +    }
> > +
> > +    pci_bios_init_platform(mem);
> >  
> >      dprintf(1, "=== PCI new allocation pass #1 ===\n");
> >      struct pci_bus *busses = malloc_tmp(sizeof(*busses) * (MaxPCIBus + 1));
> >      if (!busses) {
> >          warn_noalloc();
> > -        return;
> > +        goto done;
> >      }
> >      memset(busses, 0, sizeof(*busses) * (MaxPCIBus + 1));
> >      if (pci_bios_check_devices(busses))
> > -        return;
> > +        goto done;
> >  
> >      dprintf(1, "=== PCI new allocation pass #2 ===\n");
> > -    pci_bios_map_devices(busses);
> > +    pci_bios_map_devices(busses, mem);
> >  
> >      pci_bios_init_devices();
> >  
> >      free(busses);
> >  
> >      pci_enable_default_vga();
> > +
> > +done:
> > +    if (mem)
> > +        free(mem);
> 
> I don't understand why dynamic memory is used, and I don't understand
> why "mem" is passed around and inspected in so many places.
> 
> -Kevin

This was an attempt to keep the patch minimal.
If we don't want to test the flag in many places,
code will have to be refactored.

-- 
MST