Re: [Qemu-devel] [PATCH 0/2 v2] pc: inform SeaBIOS where 64-bit PCI hole begins

[Michael S. Tsirkin]

On Wed, Oct 30, 2013 at 02:24:54PM +0100, Igor Mammedov wrote:
> On Tue, 29 Oct 2013 20:52:42 +0200
> "Michael S. Tsirkin" <address@hidden> wrote:
> 
> > On Tue, Oct 29, 2013 at 04:28:25PM +0100, Igor Mammedov wrote:
> > > On Tue, 29 Oct 2013 17:10:47 +0200
> > > "Michael S. Tsirkin" <address@hidden> wrote:
> > > 
> > > > On Tue, Oct 29, 2013 at 01:57:33PM +0100, Igor Mammedov wrote:
> > > > > * simplify PCI address space mapping into system address space,
> > > > >   replacing code duplication in piix/q53 PCs with a helper function
> > > > > 
> > > > > * add fw_cfg 'etc/pcimem64-minimum-address' to allow QEMU reserve
> > > > >   additional address space before 64-bit PCI hole. Which will be
> > > > >   need for reserving memory hotplug region in highmem.
> > > > >   SeaBIOS counterpart: http://patchwork.ozlabs.org/patch/283623/
> > > > 
> > > > I'd like to see if we can figure out the migration issue with
> > > > memory layout.
> > > It seems that there isn't migration issue here.
> > 
> > Hmm earlier you thought there was - it's ok now?
> > 
> > > > Because if we do, and get rid of the separate 64 bit
> > > > region as a concept, exposing the start of this non-existent
> > > > region in FW CFG will make very little sense IMHO.
> > > Well, BIOS have to know where it could start 64-bit BARs mappings
> > > and
> > > telling it explicitly where, looks like a good way to do it.
> > 
> > As far as I can tell, BIOS can start any mappings anywhere it wants to
> > as long as they don't overlap anything else.
> > What is has to know is what hardware is there.
> lets suppose we are describing HW to Seabios (about which it doesn't really
> needs to know when we move ACPI tables into QEMU)
>  1. we inform Seabios where memory hotplug region ends (describing new hw)
>     and name parameter "etc/mem-hoplug-region-end" and make appropriate change
>     to Seabois so it would know about memory hotlug region.
>  2. than in several releases we decide to add another device that would
>     reserve address space after memory hotplug region. That would require
>     another modification of Seabios to teach it about new hardware so it
>     could place 64-bit PCI mappings after it.
> So describing a new hardware each time will only increase amount of PV
> interfaces overtime, breaking old BIOSes and forcing us to maintain
> compatibility layers for old versions in Seabios and QEMU.

I don't think we can predict the future.
It seems just as likely that BIOS will need to drive the
new hardware so it will not be enough to have "start pci here".
In particular, non-contiguous hotpluggable memory does not seem all that
far-fetched, and we might want to put PCI devices in the holes
that are left free.

Maybe, if you want it make it kind of generic you can just say
"reserved-memory-end" with the same value.

> 
> Opposite approach in this series tries to minimize all above mentioned
> problems by providing Seabios with an explicit information that it needs
> to make correct 64-bit PCI BARs mappings and it won't introduce
> compatibility issues with Seabios if we reserve extra space behind memory
> hotlpug region in future.

Point is that the value you expose has nothing to do
with PCI. Yes BIOS might use it for that but interfaces
need to tell what they are, not what they are used for.

> > 
> > 
> > > > 
> > > > > v2:
> > > > >  *  use negative priority to map PCI address space under RAM memory
> > > > >     regions which allows simplify code by removing pci_hole &
> > > > >     pci_hole64 memory region aliases
> > > > > 
> > > > > Series depends on:
> > > > >  "memory: Change MemoryRegion priorities from unsigned to signed:
> > > > > 
> > > > > Git tree for testing:
> > > > >   https://github.com/imammedo/qemu/commits/pcimem64-minimum-address-v2
> > > > > 
> > > > > Igor Mammedov (1):
> > > > >   pc: add 'etc/pcimem64-minimum-address' fw_cfg interface to SeaBIOS
> > > > > 
> > > > > Michael S. Tsirkin (1):
> > > > >   pc: map PCI address space as catchall region for not mapped 
> > > > > addresses
> > > > > 
> > > > >  hw/i386/pc.c              |   28 ++++++++++++++++------------
> > > > >  hw/i386/pc_piix.c         |    2 --
> > > > >  hw/pci-host/piix.c        |   27 +++++----------------------
> > > > >  hw/pci-host/q35.c         |   28 ++++++----------------------
> > > > >  include/hw/i386/pc.h      |   15 +++------------
> > > > >  include/hw/pci-host/q35.h |    2 --
> > > > >  6 files changed, 30 insertions(+), 72 deletions(-)
> >