[SeaBIOS] [Qemu-devel] [PATCH v2] map 64-bit PCI BARs at location provided by emulator

[Igor Mammedov]

On Sun, 13 Oct 2013 18:59:20 +0300
"Michael S. Tsirkin" <mst at redhat.com> wrote:

> On Sun, Oct 13, 2013 at 05:11:54PM +0200, Igor Mammedov wrote:
> > On Sun, 13 Oct 2013 15:31:23 +0300
> > "Michael S. Tsirkin" <mst at redhat.com> wrote:
> > 
> > > On Sun, Oct 13, 2013 at 02:13:44PM +0200, Igor Mammedov wrote:
> > > > Currently 64-bit PCI BARs are unconditionally mapped by BIOS right
> > > > over 4G + RamSizeOver4G location, which doesn't allow to reserve
> > > > extra space before 64-bit PCI window. For memory hotplug an extra
> > > > RAM space might be reserved after present 64-bit RAM end and BIOS
> > > > should map 64-bit PCI BARs after it.
> > > > 
> > > > Introduce "etc/pcimem64-start" romfile to provide BIOS a hint
> > > > where it should start mapping of 64-bit PCI BARs. If romfile is
> > > > missing, BIOS reverts to legacy behavior and starts mapping right
> > > > after high memory.
> > > > 
> > > > Signed-off-by: Igor Mammedov <imammedo at redhat.com>
> > > > ---
> > > > v2:
> > > >   * place 64-bit window behind high RAM end if "etc/pcimem64-start"
> > > >     points below it.
> > > 
> > > Hmm I had an alternative suggestion of passing smbios
> > > tables in from QEMU (seabios already has a mechanism for this).
> > > We could then simply increase the existing RamSize variable.
> > > 
> > > What do you think about this idea?
> > There is several issues with RamSizeOver4G I see:
> > 1. As Gerd pointed out it is also used to setup a part of e820 tables.
> >    It possibly could be also changed see "[Qemu-devel] [RfC PATCH] e820: pass
> >    high memory too" but it will be more complicated to keep seabios
> >    backward/forward compatible if we use RamSizeOver4G for passing.
> 
> What are the compatibility worries, exactly?
> 
> The way I see it, if QEMU wants to be compatible it can't
> use memory hotplug. In that case RamSizeOver4G will have
> the old meaning which should work without issues.
QEMU can't know what BIOS version is used and error out in wrong case, and that
could cause hard to guess what going wrong bugs in the guest.

> 
> > 2. It doesn't scale if we are going to use more than 1Tb future.
> >    Extending RamSizeOver4G value via additional CMOS port wasn't welcomed
> >    either, see
> > http://www.seabios.org/pipermail/seabios/2010-September/000911.html
> > 
> > It looks like fw_cfg interface is preferred one and "etc/pcimem64-start"
> > approach doesn't have backward/forward compatibility issues.
> 
> Hmm but you aren't passing in memory size - pcimem64-start
> merely tells bios where to put PCI devices.
> So how would it help?
If a lot of memory is reserved for memory hotplug (terabytes), that could
put window behind what RamSizeOver4G could address it.

> > > 
> > > 
> > > > ---
> > > >  src/fw/pciinit.c | 13 ++++++++++++-
> > > >  1 file changed, 12 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/src/fw/pciinit.c b/src/fw/pciinit.c
> > > > index b29db99..798309b 100644
> > > > --- a/src/fw/pciinit.c
> > > > +++ b/src/fw/pciinit.c
> > > > @@ -18,6 +18,8 @@
> > > >  #include "paravirt.h" // RamSize
> > > >  #include "string.h" // memset
> > > >  #include "util.h" // pci_setup
> > > > +#include "byteorder.h" // le64_to_cpu
> > > > +#include "romfile.h" // romfile_loadint
> > > >  
> > > >  #define PCI_DEVICE_MEM_MIN     0x1000
> > > >  #define PCI_BRIDGE_IO_MIN      0x1000
> > > > @@ -764,6 +766,15 @@ static void pci_bios_map_devices(struct pci_bus *busses)
> > > >  {
> > > >      if (pci_bios_init_root_regions(busses)) {
> > > >          struct pci_region r64_mem, r64_pref;
> > > > +        u64 ram64_end = 0x100000000ULL + RamSizeOver4G;
> > > > +        u64 base64 = le64_to_cpu(romfile_loadint("etc/pcimem64-start",
> > > > +                                 ram64_end));
> > > > +        if (base64 < ram64_end) {
> > > > +            dprintf(1, "ignorig etc/pcimem64-start [0x%llx] below present RAM, "
> > > > +                       "placing 64-bit PCI window behind RAM end: %llx",
> > > > +                        base64, ram64_end);
> > > > +            base64 = ram64_end;
> > > > +        }
> > > >          r64_mem.list.first = NULL;
> > > >          r64_pref.list.first = NULL;
> > > >          pci_region_migrate_64bit_entries(&busses[0].r[PCI_REGION_TYPE_MEM],
> > > > @@ -779,7 +790,7 @@ static void pci_bios_map_devices(struct pci_bus *busses)
> > > >          u64 align_mem = pci_region_align(&r64_mem);
> > > >          u64 align_pref = pci_region_align(&r64_pref);
> > > >  
> > > > -        r64_mem.base = ALIGN(0x100000000LL + RamSizeOver4G, align_mem);
> > > > +        r64_mem.base = ALIGN(base64, align_mem);
> > > >          r64_pref.base = ALIGN(r64_mem.base + sum_mem, align_pref);
> > > >          pcimem64_start = r64_mem.base;
> > > >          pcimem64_end = r64_pref.base + sum_pref;
> > > > -- 
> > > > 1.8.3.1
> > > 
> > 
> > 
> > -- 
> > Regards,
> >   Igor


-- 
Regards,
  Igor