[Qemu-devel] [PULL for-1.8 0/2] pc last minute fixes for 1.8

[Qemu-devel] [PULL for-1.8 0/2] pc last minute fixes for 1.8

[Michael S. Tsirkin]

The following changes since commit 5c5432e7d630592ddcc1876ac8a1505f8f14ef15:

  Merge remote-tracking branch 'luiz/queue/qmp' into staging (2013-11-13 11:49:27 -0800)

are available in the git repository at:

  git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git tags/for_anthony

for you to fetch changes up to 420508fbba2a6e8eaff008715b5f7eff83f8e865:

  doc: fix hardcoded helper path (2013-11-18 13:45:10 +0200)

----------------------------------------------------------------
pc last minute fixes for 1.8

This has a patch that drops an unused FW CFG entry.
I think it's best to include it before 1.7 to avoid
the need to maintain it in compat machine types.

There's also a doc bugfix by Amos: I'm guessing
doc fixes are still fair game even at this late stage.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

----------------------------------------------------------------

Amos Kong (1):
  doc: fix hardcoded helper path

Igor Mammedov (1):
  pc: disable pci-info

 hw/i386/pc_piix.c | 2 +-
 hw/i386/pc_q35.c  | 2 +-
 qemu-options.hx   | 6 +++---
 3 files changed, 5 insertions(+), 5 deletions(-)

-- 
MST

[Qemu-devel] [PULL for-1.8 1/2] pc: disable pci-info

[Michael S. Tsirkin]

From: Igor Mammedov <imammedo@redhat.com>

The BIOS that we ship in 1.7 does not use pci info
from host and so far isn't going to use it.
Taking in account problems it caused see 9604f70fdf and
to avoid future incompatibility issues, it's safest to
disable that interface by default for all machine types
including 1.7 as it was never exposed/used by guest.
And properly remove/cleanup it during 1.8 development cycle.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/i386/pc_piix.c | 2 +-
 hw/i386/pc_q35.c  | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 4fdb7b6..094c421 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -58,7 +58,7 @@ static const int ide_iobase2[MAX_IDE_BUS] = { 0x3f6, 0x376 };
 static const int ide_irq[MAX_IDE_BUS] = { 14, 15 };
 
 static bool has_pvpanic;
-static bool has_pci_info = true;
+static bool has_pci_info;
 static bool has_acpi_build = true;
 
 /* PC hardware initialisation */
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 4c191d3..1af8e2b 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -48,7 +48,7 @@
 #define MAX_SATA_PORTS     6
 
 static bool has_pvpanic;
-static bool has_pci_info = true;
+static bool has_pci_info;
 static bool has_acpi_build = true;
 
 /* PC hardware initialisation */
-- 
MST

[Qemu-devel] [PULL for-1.8 2/2] doc: fix hardcoded helper path

[Michael S. Tsirkin]

From: Amos Kong <akong@redhat.com>

The install directory of qemu-bridge-helper is configurable,
but we use a fixed path in the documentation.

DEFAULT_BRIDGE_HELPER macro isn't available in texi mode,
we should always use "/path/to/" prefix for dynamic paths
(e.g.: /path/to/image, /path/to/linux, etc).

Signed-off-by: Amos Kong <akong@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 qemu-options.hx | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/qemu-options.hx b/qemu-options.hx
index 5dc8b75..8b94264 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -1605,7 +1605,7 @@ to disable script execution.
 
 If running QEMU as an unprivileged user, use the network helper
 @var{helper} to configure the TAP interface. The default network
-helper executable is @file{/usr/local/libexec/qemu-bridge-helper}.
+helper executable is @file{/path/to/qemu-bridge-helper}.
 
 @option{fd}=@var{h} can be used to specify the handle of an already
 opened host TAP interface.
@@ -1629,7 +1629,7 @@ qemu-system-i386 linux.img \
 #launch a QEMU instance with the default network helper to
 #connect a TAP device to bridge br0
 qemu-system-i386 linux.img \
-                 -net nic -net tap,"helper=/usr/local/libexec/qemu-bridge-helper"
+                 -net nic -net tap,"helper=/path/to/qemu-bridge-helper"
 @end example
 
 @item -netdev bridge,id=@var{id}[,br=@var{bridge}][,helper=@var{helper}]
@@ -1638,7 +1638,7 @@ Connect a host TAP network interface to a host bridge device.
 
 Use the network helper @var{helper} to configure the TAP interface and
 attach it to the bridge. The default network helper executable is
-@file{/usr/local/libexec/qemu-bridge-helper} and the default bridge
+@file{/path/to/qemu-bridge-helper} and the default bridge
 device is @file{br0}.
 
 Examples:
-- 
MST

Re: [Qemu-devel] [PULL for-1.8 0/2] pc last minute fixes for 1.8

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1016 bytes --]

On 11/18/2013 04:53 AM, Michael S. Tsirkin wrote:
> The following changes since commit 5c5432e7d630592ddcc1876ac8a1505f8f14ef15:
> 
>   Merge remote-tracking branch 'luiz/queue/qmp' into staging (2013-11-13 11:49:27 -0800)
> 
> are available in the git repository at:
> 
>   git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git tags/for_anthony
> 
> for you to fetch changes up to 420508fbba2a6e8eaff008715b5f7eff83f8e865:
> 
>   doc: fix hardcoded helper path (2013-11-18 13:45:10 +0200)
> 
> ----------------------------------------------------------------
> pc last minute fixes for 1.8
> 
> This has a patch that drops an unused FW CFG entry.
> I think it's best to include it before 1.7 to avoid
> the need to maintain it in compat machine types.

Which is it?  Last minute fixes to be included in 1.7, or some of the
first patches to be applied for 1.8 once 1.7 is out the door?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PULL for-1.8 0/2] pc last minute fixes for 1.8

[Michael S. Tsirkin]

On Mon, Nov 18, 2013 at 08:06:48AM -0700, Eric Blake wrote:
> On 11/18/2013 04:53 AM, Michael S. Tsirkin wrote:
> > The following changes since commit 5c5432e7d630592ddcc1876ac8a1505f8f14ef15:
> > 
> >   Merge remote-tracking branch 'luiz/queue/qmp' into staging (2013-11-13 11:49:27 -0800)
> > 
> > are available in the git repository at:
> > 
> >   git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git tags/for_anthony
> > 
> > for you to fetch changes up to 420508fbba2a6e8eaff008715b5f7eff83f8e865:
> > 
> >   doc: fix hardcoded helper path (2013-11-18 13:45:10 +0200)
> > 
> > ----------------------------------------------------------------
> > pc last minute fixes for 1.8
> > 
> > This has a patch that drops an unused FW CFG entry.
> > I think it's best to include it before 1.7 to avoid
> > the need to maintain it in compat machine types.
> 
> Which is it?  Last minute fixes to be included in 1.7, or some of the
> first patches to be applied for 1.8 once 1.7 is out the door?

Ugh. That should have been 1.7.

Anthony, would you like me to sign with
proper name and re-send?

> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
> 

Re: [Qemu-devel] [PULL for-1.8 1/2] pc: disable pci-info

[Laszlo Ersek]

On 11/18/13 12:53, Michael S. Tsirkin wrote:
> From: Igor Mammedov <imammedo@redhat.com>
> 
> The BIOS that we ship in 1.7 does not use pci info
> from host and so far isn't going to use it.
> Taking in account problems it caused see 9604f70fdf and
> to avoid future incompatibility issues, it's safest to
> disable that interface by default for all machine types
> including 1.7 as it was never exposed/used by guest.
> And properly remove/cleanup it during 1.8 development cycle.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  hw/i386/pc_piix.c | 2 +-
>  hw/i386/pc_q35.c  | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)

etc/pci-info is precisely the form and contents that OVMF needs to
download ACPI tables from qemu.

<http://sourceforge.net/mailarchive/forum.php?thread_name=1385450282-27007-3-git-send-email-lersek%40redhat.com&forum_name=edk2-devel>

Please keep this exported in 1.8, for OVMF's sake.

Thanks
Laszlo

Re: [Qemu-devel] [PULL for-1.8 1/2] pc: disable pci-info

[Michael S. Tsirkin]

On Tue, Nov 26, 2013 at 09:12:50AM +0100, Laszlo Ersek wrote:
> On 11/18/13 12:53, Michael S. Tsirkin wrote:
> > From: Igor Mammedov <imammedo@redhat.com>
> > 
> > The BIOS that we ship in 1.7 does not use pci info
> > from host and so far isn't going to use it.
> > Taking in account problems it caused see 9604f70fdf and
> > to avoid future incompatibility issues, it's safest to
> > disable that interface by default for all machine types
> > including 1.7 as it was never exposed/used by guest.
> > And properly remove/cleanup it during 1.8 development cycle.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>
> > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  hw/i386/pc_piix.c | 2 +-
> >  hw/i386/pc_q35.c  | 2 +-
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> etc/pci-info is precisely the form and contents that OVMF needs to
> download ACPI tables from qemu.
> 
> <http://sourceforge.net/mailarchive/forum.php?thread_name=1385450282-27007-3-git-send-email-lersek%40redhat.com&forum_name=edk2-devel>
> 
> Please keep this exported in 1.8, for OVMF's sake.
> 
> Thanks
> Laszlo

This pull request was misnamed, it was merged for 1.7.
Problem is pci-info can't be implemented correctly as defined:
for example we don't know where does MMCONFIG resize before
it is configured.
This patch was acked by several people so we'll need a stronger justification
for re-introducing it.
seabios manages to enumerate PCI with information exported from qemu
so why can't OVMF?

I think it's down to other qemu bugs (such as _CRS not covering
all of PCI memory), we shall just fix them.

-- 
MST

Re: [Qemu-devel] [PULL for-1.8 1/2] pc: disable pci-info

[Gerd Hoffmann]

[-- Attachment #1: Type: text/plain, Size: 182 bytes --]

  Hi,

> I think it's down to other qemu bugs (such as _CRS not covering
> all of PCI memory), we shall just fix them.

i.e. the attached patch should fixup things.

cheers,
  Gerd


[-- Attachment #2: Type: text/x-patch, Size: 1432 bytes --]

>From a81b8d66e24fd298ce7654d424a378337e6cf132 Mon Sep 17 00:00:00 2001
From: Gerd Hoffmann <kraxel@redhat.com>
Date: Tue, 26 Nov 2013 11:46:11 +0100
Subject: [PATCH] piix: fix 32bit pci hole

Make the 32bit pci hole start at end of ram, so all possible address
space is covered.  Of course the firmware can use less than that.
Leaving space unused is no problem, mapping pci bars outside the
hole causes problems though.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/pci-host/piix.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
index edc974e..1414a2b 100644
--- a/hw/pci-host/piix.c
+++ b/hw/pci-host/piix.c
@@ -345,15 +345,7 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
     f->ram_memory = ram_memory;
 
     i440fx = I440FX_PCI_HOST_BRIDGE(dev);
-    /* Set PCI window size the way seabios has always done it. */
-    /* Power of 2 so bios can cover it with a single MTRR */
-    if (ram_size <= 0x80000000) {
-        i440fx->pci_info.w32.begin = 0x80000000;
-    } else if (ram_size <= 0xc0000000) {
-        i440fx->pci_info.w32.begin = 0xc0000000;
-    } else {
-        i440fx->pci_info.w32.begin = 0xe0000000;
-    }
+    i440fx->pci_info.w32.begin = ram_size;
 
     memory_region_init_alias(&f->pci_hole, OBJECT(d), "pci-hole", f->pci_address_space,
                              pci_hole_start, pci_hole_size);
-- 
1.8.3.1

Re: [Qemu-devel] [PULL for-1.8 1/2] pc: disable pci-info

[Michael S. Tsirkin]

On Tue, Nov 26, 2013 at 12:00:51PM +0100, Gerd Hoffmann wrote:
>   Hi,
> 
> > I think it's down to other qemu bugs (such as _CRS not covering
> > all of PCI memory), we shall just fix them.
> 
> i.e. the attached patch should fixup things.
> 
> cheers,
>   Gerd
> 

Yes, I think it's a start. Q35 is a bit harder because of the MMIO
region.  Do we want to tweak end too? There's all kind of
stuff there so need to be careful ...

> >From a81b8d66e24fd298ce7654d424a378337e6cf132 Mon Sep 17 00:00:00 2001
> From: Gerd Hoffmann <kraxel@redhat.com>
> Date: Tue, 26 Nov 2013 11:46:11 +0100
> Subject: [PATCH] piix: fix 32bit pci hole
> 
> Make the 32bit pci hole start at end of ram, so all possible address
> space is covered.  Of course the firmware can use less than that.
> Leaving space unused is no problem, mapping pci bars outside the
> hole causes problems though.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/pci-host/piix.c | 10 +---------
>  1 file changed, 1 insertion(+), 9 deletions(-)
> 
> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> index edc974e..1414a2b 100644
> --- a/hw/pci-host/piix.c
> +++ b/hw/pci-host/piix.c
> @@ -345,15 +345,7 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
>      f->ram_memory = ram_memory;
>  
>      i440fx = I440FX_PCI_HOST_BRIDGE(dev);
> -    /* Set PCI window size the way seabios has always done it. */
> -    /* Power of 2 so bios can cover it with a single MTRR */
> -    if (ram_size <= 0x80000000) {
> -        i440fx->pci_info.w32.begin = 0x80000000;
> -    } else if (ram_size <= 0xc0000000) {
> -        i440fx->pci_info.w32.begin = 0xc0000000;
> -    } else {
> -        i440fx->pci_info.w32.begin = 0xe0000000;
> -    }
> +    i440fx->pci_info.w32.begin = ram_size;
>  
>      memory_region_init_alias(&f->pci_hole, OBJECT(d), "pci-hole", f->pci_address_space,
>                               pci_hole_start, pci_hole_size);
> -- 
> 1.8.3.1
> 

Re: [Qemu-devel] [PULL for-1.8 1/2] pc: disable pci-info

[Laszlo Ersek]

On 11/26/13 10:10, Michael S. Tsirkin wrote:

> seabios manages to enumerate PCI with information exported from qemu
> so why can't OVMF?

SeaBIOS and qemu duplicate logic (code) between each other.

src/fw/pciinit.c grabs RamSize over fw_cfg, and i440fx_mem_addr_setup()
sets "pcimem_start" to one of three values based on RamSize. (One of the
three is not explicit there, it's the build default 0xe0000000.)

The same code is visible in qemu in i440fx_init().

I duplicated the same logic in OVMF's PEI one week ago, and it solved
the problem. See the patch attached to
<http://thread.gmane.org/gmane.comp.bios.tianocore.devel/4881/focus=4995>.

But code/logic duplication is ugly, which is why I've been looking for a
better, dynamic solution ever since.

> I think it's down to other qemu bugs (such as _CRS not covering
> all of PCI memory), we shall just fix them.

I don't know how to fix them. I don't know how to enumerate all PCI
regions in use, plus all unassigned ranges, from below, like with a
MemoryListener.

If I understand correctly, Igor suggested to track devices as they map
their regions, but (again, if I understood correctly) you didn't seem to
like the idea.

Thanks
Laszlo

Re: [Qemu-devel] [PULL for-1.8 1/2] pc: disable pci-info

[Laszlo Ersek]

[-- Attachment #1: Type: text/plain, Size: 2640 bytes --]

On 11/26/13 15:04, Michael S. Tsirkin wrote:
> On Tue, Nov 26, 2013 at 12:00:51PM +0100, Gerd Hoffmann wrote:
>>   Hi,
>>
>>> I think it's down to other qemu bugs (such as _CRS not covering
>>> all of PCI memory), we shall just fix them.
>>
>> i.e. the attached patch should fixup things.
>>
>> cheers,
>>   Gerd
>>
> 
> Yes, I think it's a start. Q35 is a bit harder because of the MMIO
> region.  Do we want to tweak end too? There's all kind of
> stuff there so need to be careful ...

I sent you a very similar patch last evening, and you ignored it. In
that same email I asked about the mmconfig stuff as well, and you
ignored that too. Attached.

>> >From a81b8d66e24fd298ce7654d424a378337e6cf132 Mon Sep 17 00:00:00 2001
>> From: Gerd Hoffmann <kraxel@redhat.com>
>> Date: Tue, 26 Nov 2013 11:46:11 +0100
>> Subject: [PATCH] piix: fix 32bit pci hole
>>
>> Make the 32bit pci hole start at end of ram, so all possible address
>> space is covered.  Of course the firmware can use less than that.
>> Leaving space unused is no problem, mapping pci bars outside the
>> hole causes problems though.
>>
>> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>> ---
>>  hw/pci-host/piix.c | 10 +---------
>>  1 file changed, 1 insertion(+), 9 deletions(-)
>>
>> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
>> index edc974e..1414a2b 100644
>> --- a/hw/pci-host/piix.c
>> +++ b/hw/pci-host/piix.c
>> @@ -345,15 +345,7 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
>>      f->ram_memory = ram_memory;
>>  
>>      i440fx = I440FX_PCI_HOST_BRIDGE(dev);
>> -    /* Set PCI window size the way seabios has always done it. */
>> -    /* Power of 2 so bios can cover it with a single MTRR */
>> -    if (ram_size <= 0x80000000) {
>> -        i440fx->pci_info.w32.begin = 0x80000000;
>> -    } else if (ram_size <= 0xc0000000) {
>> -        i440fx->pci_info.w32.begin = 0xc0000000;
>> -    } else {
>> -        i440fx->pci_info.w32.begin = 0xe0000000;
>> -    }
>> +    i440fx->pci_info.w32.begin = ram_size;

This doesn't clamp the w32.begin value into [0x80000000, 0xe0000000],
which seems wrong.

Guys, I'm confused and annoyed out of my brains by the divergence here.
In my perception Michael, Igor, and Gerd are all proposing different
things, and my ideas are either rejected or ignored (even though they
occasionally overlap with ideas from others). After struggling with this
for more than a week, and having being awake for 27 hrs, I'm officially
stopping to care right now. Ping me when qemu has something to offer
that's neither ridden by SeaBIOS legacy nor requires an AML interpreter
in OVMF's PEI phase.

Thanks,
Laszlo

[-- Attachment #2: Attached Message --]
[-- Type: message/rfc822, Size: 4243 bytes --]

From: Laszlo Ersek <lersek@redhat.com>
To: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Subject: Re: changing the pci32 window in qemu's DSDT/SSDT
Date: Mon, 25 Nov 2013 15:46:58 +0100
Message-ID: <529362E2.1040001@redhat.com>

CC'ing Igor

On 11/25/13 14:24, Michael S. Tsirkin wrote:
> On Sun, Nov 24, 2013 at 06:43:38PM +0100, Laszlo Ersek wrote:
>> Hello Michael,
>>
>> I didn't completely get your point on IRC regarding this subject. You
>> were saying that OVMF shouldn't be changed, but the boundaries
>> exported by qemu should (and that qemu should stop caring about
>> SeaBIOS tradition in this regard).
>>
>> You mentioned MMCONFIG and I don't understand that reference. Can you
>> please elaborate how you think we should proceed?
>>
>> Thanks!
>> Laszlo
>
> I refer to this:
>     /* Leave enough space for the biggest MCFG BAR */
>     /* TODO: this matches current bios behaviour, but
>      * it's not a power of two, which means an MTRR
>      * can't cover it exactly.
>      */
>     s->mch.pci_info.w32.begin = MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT +
>         MCH_HOST_BRIDGE_PCIEXBAR_MAX;
>     s->mch.pci_info.w32.end = IO_APIC_DEFAULT_ADDRESS;
>
> We must make sure CRS does not include the mmcfg region.

Sorry but I still don't understand :)

First, OVMF is currently for i440fx only, and the quoted part is from
"hw/pci-host/q35.c". I of course agree that we should fix this in a
uniform manner in qemu -- just saying that for OVMF the following would
suffice:

  diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
  index edc974e..9753fea 100644
  --- a/hw/pci-host/piix.c
  +++ b/hw/pci-host/piix.c
  @@ -345,12 +345,10 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
       f->ram_memory = ram_memory;

       i440fx = I440FX_PCI_HOST_BRIDGE(dev);
  -    /* Set PCI window size the way seabios has always done it. */
  -    /* Power of 2 so bios can cover it with a single MTRR */
       if (ram_size <= 0x80000000) {
           i440fx->pci_info.w32.begin = 0x80000000;
  -    } else if (ram_size <= 0xc0000000) {
  -        i440fx->pci_info.w32.begin = 0xc0000000;
  +    } else if (ram_size <= 0xe0000000) {
  +        i440fx->pci_info.w32.begin = ram_size;
       } else {
           i440fx->pci_info.w32.begin = 0xe0000000;
       }

Second, the code that you quoted from "hw/pci-host/q35.c" seems to do
exactly what you're saying we must do.

  static Property mch_props[] = {
      DEFINE_PROP_UINT64(PCIE_HOST_MCFG_BASE, Q35PCIHost, parent_obj.base_addr,
                          MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT),

This sets the default mmconfig base address at
MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT. The code you qouted seems to ensure
that the range starting at MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT, for a
length of MCH_HOST_BRIDGE_PCIEXBAR_MAX (ie from 0xb0000000 to
0xc0000000) is not included in CRS. In other words, it sets w32.begin to
right above this BAR (to 0xc0000000), hence the mmcfg region is already
excluded from CRS.

So, what do you mean? Do you want to make w32.begin depend on the
PCIE_HOST_MCFG_BASE property dynamically (ie. to notice when the user
changes that option on the command line)?

  diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
  index c043998..48e6758 100644
  --- a/hw/pci-host/q35.c
  +++ b/hw/pci-host/q35.c
  @@ -181,11 +181,7 @@ static void q35_host_initfn(Object *obj)
                           NULL, NULL, NULL, NULL);

       /* Leave enough space for the biggest MCFG BAR */
  -    /* TODO: this matches current bios behaviour, but
  -     * it's not a power of two, which means an MTRR
  -     * can't cover it exactly.
  -     */
  -    s->mch.pci_info.w32.begin = MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT +
  +    s->mch.pci_info.w32.begin = PCIE_HOST_BRIDGE(obj)->base_addr +
           MCH_HOST_BRIDGE_PCIEXBAR_MAX;
       s->mch.pci_info.w32.end = IO_APIC_DEFAULT_ADDRESS;
   }

Sorry for being dense...

Thanks
Laszlo

Re: [Qemu-devel] [PULL for-1.8 1/2] pc: disable pci-info

[Michael S. Tsirkin]

On Tue, Nov 26, 2013 at 03:22:28PM +0100, Laszlo Ersek wrote:
> On 11/26/13 15:04, Michael S. Tsirkin wrote:
> > On Tue, Nov 26, 2013 at 12:00:51PM +0100, Gerd Hoffmann wrote:
> >>   Hi,
> >>
> >>> I think it's down to other qemu bugs (such as _CRS not covering
> >>> all of PCI memory), we shall just fix them.
> >>
> >> i.e. the attached patch should fixup things.
> >>
> >> cheers,
> >>   Gerd
> >>
> > 
> > Yes, I think it's a start. Q35 is a bit harder because of the MMIO
> > region.  Do we want to tweak end too? There's all kind of
> > stuff there so need to be careful ...
> 
> I sent you a very similar patch last evening, and you ignored it. In
> that same email I asked about the mmconfig stuff as well, and you
> ignored that too. Attached.

I thought Igor answered this reasonably well actually.
I answered myself just in case ...


> >> >From a81b8d66e24fd298ce7654d424a378337e6cf132 Mon Sep 17 00:00:00 2001
> >> From: Gerd Hoffmann <kraxel@redhat.com>
> >> Date: Tue, 26 Nov 2013 11:46:11 +0100
> >> Subject: [PATCH] piix: fix 32bit pci hole
> >>
> >> Make the 32bit pci hole start at end of ram, so all possible address
> >> space is covered.  Of course the firmware can use less than that.
> >> Leaving space unused is no problem, mapping pci bars outside the
> >> hole causes problems though.
> >>
> >> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> >> ---
> >>  hw/pci-host/piix.c | 10 +---------
> >>  1 file changed, 1 insertion(+), 9 deletions(-)
> >>
> >> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> >> index edc974e..1414a2b 100644
> >> --- a/hw/pci-host/piix.c
> >> +++ b/hw/pci-host/piix.c
> >> @@ -345,15 +345,7 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
> >>      f->ram_memory = ram_memory;
> >>  
> >>      i440fx = I440FX_PCI_HOST_BRIDGE(dev);
> >> -    /* Set PCI window size the way seabios has always done it. */
> >> -    /* Power of 2 so bios can cover it with a single MTRR */
> >> -    if (ram_size <= 0x80000000) {
> >> -        i440fx->pci_info.w32.begin = 0x80000000;
> >> -    } else if (ram_size <= 0xc0000000) {
> >> -        i440fx->pci_info.w32.begin = 0xc0000000;
> >> -    } else {
> >> -        i440fx->pci_info.w32.begin = 0xe0000000;
> >> -    }
> >> +    i440fx->pci_info.w32.begin = ram_size;
> 
> This doesn't clamp the w32.begin value into [0x80000000, 0xe0000000],
> which seems wrong.
> 
> Guys, I'm confused and annoyed out of my brains by the divergence here.
> In my perception Michael, Igor, and Gerd are all proposing different
> things, and my ideas are either rejected or ignored (even though they
> occasionally overlap with ideas from others). After struggling with this
> for more than a week, and having being awake for 27 hrs, I'm officially
> stopping to care right now.

Awake for 27 hours is not good.
Take a break, get some rest :)
I think what you propose below makes sense overall,
and it also seems that everyone agrees on this.

However there are some minor implementation issues and people started
arguing about them.
Also, everyone here is human, it's easy to miss a mail in a long thread.


> Ping me when qemu has something to offer
> that's neither ridden by SeaBIOS legacy nor requires an AML interpreter
> in OVMF's PEI phase.
> 
> Thanks,
> Laszlo

Something along the lines of what you propose below will do it right?


> Date: Mon, 25 Nov 2013 15:46:58 +0100
> From: Laszlo Ersek <lersek@redhat.com>
> To: "Michael S. Tsirkin" <mst@redhat.com>
> CC: Igor Mammedov <imammedo@redhat.com>
> Subject: Re: changing the pci32 window in qemu's DSDT/SSDT
> Message-ID: <529362E2.1040001@redhat.com>
> In-Reply-To: <20131125132400.GA1327@redhat.com>
> 
> CC'ing Igor
> 
> On 11/25/13 14:24, Michael S. Tsirkin wrote:
> > On Sun, Nov 24, 2013 at 06:43:38PM +0100, Laszlo Ersek wrote:
> >> Hello Michael,
> >>
> >> I didn't completely get your point on IRC regarding this subject. You
> >> were saying that OVMF shouldn't be changed, but the boundaries
> >> exported by qemu should (and that qemu should stop caring about
> >> SeaBIOS tradition in this regard).
> >>
> >> You mentioned MMCONFIG and I don't understand that reference. Can you
> >> please elaborate how you think we should proceed?
> >>
> >> Thanks!
> >> Laszlo
> >
> > I refer to this:
> >     /* Leave enough space for the biggest MCFG BAR */
> >     /* TODO: this matches current bios behaviour, but
> >      * it's not a power of two, which means an MTRR
> >      * can't cover it exactly.
> >      */
> >     s->mch.pci_info.w32.begin = MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT +
> >         MCH_HOST_BRIDGE_PCIEXBAR_MAX;
> >     s->mch.pci_info.w32.end = IO_APIC_DEFAULT_ADDRESS;
> >
> > We must make sure CRS does not include the mmcfg region.
> 
> Sorry but I still don't understand :)
> 
> First, OVMF is currently for i440fx only, and the quoted part is from
> "hw/pci-host/q35.c". I of course agree that we should fix this in a
> uniform manner in qemu -- just saying that for OVMF the following would
> suffice:
> 
>   diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
>   index edc974e..9753fea 100644
>   --- a/hw/pci-host/piix.c
>   +++ b/hw/pci-host/piix.c
>   @@ -345,12 +345,10 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
>        f->ram_memory = ram_memory;
> 
>        i440fx = I440FX_PCI_HOST_BRIDGE(dev);
>   -    /* Set PCI window size the way seabios has always done it. */
>   -    /* Power of 2 so bios can cover it with a single MTRR */
>        if (ram_size <= 0x80000000) {
>            i440fx->pci_info.w32.begin = 0x80000000;
>   -    } else if (ram_size <= 0xc0000000) {
>   -        i440fx->pci_info.w32.begin = 0xc0000000;
>   +    } else if (ram_size <= 0xe0000000) {
>   +        i440fx->pci_info.w32.begin = ram_size;
>        } else {
>            i440fx->pci_info.w32.begin = 0xe0000000;
>        }
> 
> Second, the code that you quoted from "hw/pci-host/q35.c" seems to do
> exactly what you're saying we must do.
> 
>   static Property mch_props[] = {
>       DEFINE_PROP_UINT64(PCIE_HOST_MCFG_BASE, Q35PCIHost, parent_obj.base_addr,
>                           MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT),
> 
> This sets the default mmconfig base address at
> MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT. The code you qouted seems to ensure
> that the range starting at MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT, for a
> length of MCH_HOST_BRIDGE_PCIEXBAR_MAX (ie from 0xb0000000 to
> 0xc0000000) is not included in CRS. In other words, it sets w32.begin to
> right above this BAR (to 0xc0000000), hence the mmcfg region is already
> excluded from CRS.
> 
> So, what do you mean? Do you want to make w32.begin depend on the
> PCIE_HOST_MCFG_BASE property dynamically (ie. to notice when the user
> changes that option on the command line)?
> 
>   diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
>   index c043998..48e6758 100644
>   --- a/hw/pci-host/q35.c
>   +++ b/hw/pci-host/q35.c
>   @@ -181,11 +181,7 @@ static void q35_host_initfn(Object *obj)
>                            NULL, NULL, NULL, NULL);
> 
>        /* Leave enough space for the biggest MCFG BAR */
>   -    /* TODO: this matches current bios behaviour, but
>   -     * it's not a power of two, which means an MTRR
>   -     * can't cover it exactly.
>   -     */
>   -    s->mch.pci_info.w32.begin = MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT +
>   +    s->mch.pci_info.w32.begin = PCIE_HOST_BRIDGE(obj)->base_addr +
>            MCH_HOST_BRIDGE_PCIEXBAR_MAX;
>        s->mch.pci_info.w32.end = IO_APIC_DEFAULT_ADDRESS;
>    }
> 
> Sorry for being dense...
> 
> Thanks
> Laszlo

Re: [Qemu-devel] [PULL for-1.8 1/2] pc: disable pci-info

[Gerd Hoffmann]

[-- Attachment #1: Type: text/plain, Size: 773 bytes --]

On Di, 2013-11-26 at 16:04 +0200, Michael S. Tsirkin wrote:
> On Tue, Nov 26, 2013 at 12:00:51PM +0100, Gerd Hoffmann wrote:
> >   Hi,
> > 
> > > I think it's down to other qemu bugs (such as _CRS not covering
> > > all of PCI memory), we shall just fix them.
> > 
> > i.e. the attached patch should fixup things.
> > 
> > cheers,
> >   Gerd
> > 
> 
> Yes, I think it's a start. Q35 is a bit harder because of the MMIO
> region.

???  Do you mean mmconfig?  That can live inside the window.  So
something like the attached patch should work in theory.  In practice it
hasn't the expected effect for some reason ...

> Do we want to tweak end too? There's all kind of
> stuff there so need to be careful ...

I'd leave the end as-is, at the ioapic address.

cheers,
  Gerd


[-- Attachment #2: Type: text/x-patch, Size: 1630 bytes --]

>From 3d01b6c46fbf655bdb9b4f7ca427f40959b05d31 Mon Sep 17 00:00:00 2001
From: Gerd Hoffmann <kraxel@redhat.com>
Date: Tue, 26 Nov 2013 16:18:04 +0100
Subject: [PATCH] [wip] q35: fix 32bit pci hole

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/pci-host/q35.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
index c043998..8d47bf9 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -179,15 +179,6 @@ static void q35_host_initfn(Object *obj)
     object_property_add(obj, PCIE_HOST_MCFG_SIZE, "int",
                         q35_host_get_mmcfg_size,
                         NULL, NULL, NULL, NULL);
-
-    /* Leave enough space for the biggest MCFG BAR */
-    /* TODO: this matches current bios behaviour, but
-     * it's not a power of two, which means an MTRR
-     * can't cover it exactly.
-     */
-    s->mch.pci_info.w32.begin = MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT +
-        MCH_HOST_BRIDGE_PCIEXBAR_MAX;
-    s->mch.pci_info.w32.end = IO_APIC_DEFAULT_ADDRESS;
 }
 
 static const TypeInfo q35_host_info = {
@@ -365,6 +356,8 @@ static int mch_init(PCIDevice *d)
                              0x100000000ULL - mch->below_4g_mem_size);
     memory_region_add_subregion(mch->system_memory, mch->below_4g_mem_size,
                                 &mch->pci_hole);
+    mch->pci_info.w32.begin = mch->below_4g_mem_size;
+    mch->pci_info.w32.end = IO_APIC_DEFAULT_ADDRESS;
 
     pci_hole64_size = pci_host_get_hole64_size(mch->pci_hole64_size);
     pc_init_pci64_hole(&mch->pci_info, 0x100000000ULL + mch->above_4g_mem_size,
-- 
1.8.3.1

Re: [Qemu-devel] [PULL for-1.8 1/2] pc: disable pci-info

[Michael S. Tsirkin]

On Tue, Nov 26, 2013 at 04:20:58PM +0100, Gerd Hoffmann wrote:
> On Di, 2013-11-26 at 16:04 +0200, Michael S. Tsirkin wrote:
> > On Tue, Nov 26, 2013 at 12:00:51PM +0100, Gerd Hoffmann wrote:
> > >   Hi,
> > > 
> > > > I think it's down to other qemu bugs (such as _CRS not covering
> > > > all of PCI memory), we shall just fix them.
> > > 
> > > i.e. the attached patch should fixup things.
> > > 
> > > cheers,
> > >   Gerd
> > > 
> > 
> > Yes, I think it's a start. Q35 is a bit harder because of the MMIO
> > region.
> 
> ???  Do you mean mmconfig?  That can live inside the window.

Are you sure? When I tried windows crashed but maybe I'm wrong.
Did you try some windows guests?

>  So
> something like the attached patch should work in theory.  In practice it
> hasn't the expected effect for some reason ...
> 
> > Do we want to tweak end too? There's all kind of
> > stuff there so need to be careful ...
> 
> I'd leave the end as-is, at the ioapic address.
> 
> cheers,
>   Gerd
> 

> >From 3d01b6c46fbf655bdb9b4f7ca427f40959b05d31 Mon Sep 17 00:00:00 2001
> From: Gerd Hoffmann <kraxel@redhat.com>
> Date: Tue, 26 Nov 2013 16:18:04 +0100
> Subject: [PATCH] [wip] q35: fix 32bit pci hole
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/pci-host/q35.c | 11 ++---------
>  1 file changed, 2 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> index c043998..8d47bf9 100644
> --- a/hw/pci-host/q35.c
> +++ b/hw/pci-host/q35.c
> @@ -179,15 +179,6 @@ static void q35_host_initfn(Object *obj)
>      object_property_add(obj, PCIE_HOST_MCFG_SIZE, "int",
>                          q35_host_get_mmcfg_size,
>                          NULL, NULL, NULL, NULL);
> -
> -    /* Leave enough space for the biggest MCFG BAR */
> -    /* TODO: this matches current bios behaviour, but
> -     * it's not a power of two, which means an MTRR
> -     * can't cover it exactly.
> -     */
> -    s->mch.pci_info.w32.begin = MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT +
> -        MCH_HOST_BRIDGE_PCIEXBAR_MAX;
> -    s->mch.pci_info.w32.end = IO_APIC_DEFAULT_ADDRESS;
>  }
>  
>  static const TypeInfo q35_host_info = {
> @@ -365,6 +356,8 @@ static int mch_init(PCIDevice *d)
>                               0x100000000ULL - mch->below_4g_mem_size);
>      memory_region_add_subregion(mch->system_memory, mch->below_4g_mem_size,
>                                  &mch->pci_hole);
> +    mch->pci_info.w32.begin = mch->below_4g_mem_size;
> +    mch->pci_info.w32.end = IO_APIC_DEFAULT_ADDRESS;
>  
>      pci_hole64_size = pci_host_get_hole64_size(mch->pci_hole64_size);
>      pc_init_pci64_hole(&mch->pci_info, 0x100000000ULL + mch->above_4g_mem_size,
> -- 
> 1.8.3.1
> 

Re: [Qemu-devel] [PULL for-1.8 1/2] pc: disable pci-info

[Gerd Hoffmann]

  Hi,

> This doesn't clamp the w32.begin value into [0x80000000, 0xe0000000],
> which seems wrong.

Why?  In a 1G guest you can map pci bars at 0x40000000 just fine.

_CRS in acpi should declare the area where you can map pci bars, and
that happens to be end-of-ram -> ioapci base.

Firmware can choose to use a smaller area.  Both seabios and ovmf will
not map anyting below 0x80000000.  That is just fine.  As long as all
pci bars get mapped inside the region declared in _CRS things will work.

While thinking about it:  There is one possible reason to not do it:
Guest bugs.  IIRC windows doesn't like the 64bit pci window being larger
than 2G.  Possibly that is an issue with the 32bit window too.  If that
is the case we should indeed not use w32.begin values smaller than
0x8000000.  Igor, any clue?

> Guys, I'm confused and annoyed out of my brains by the divergence here.
> In my perception Michael, Igor, and Gerd are all proposing different
> things, and my ideas are either rejected or ignored (even though they
> occasionally overlap with ideas from others).

Hmm, as far as I can see there is an agreement that it is qemu's fault,
the acpi tables need fixing, and ovmf should not need changes any
changes.

Mimicing seabios/qemu logic in ovmf can be used as temporary stopgap
while details are sorted on the qemu side.

cheers,
  Gerd

Re: [Qemu-devel] [PULL for-1.8 1/2] pc: disable pci-info

[Michael S. Tsirkin]

On Tue, Nov 26, 2013 at 04:42:16PM +0100, Gerd Hoffmann wrote:
>   Hi,
> 
> > This doesn't clamp the w32.begin value into [0x80000000, 0xe0000000],
> > which seems wrong.
> 
> Why?  In a 1G guest you can map pci bars at 0x40000000 just fine.
> 
> _CRS in acpi should declare the area where you can map pci bars, and
> that happens to be end-of-ram -> ioapci base.
> 
> Firmware can choose to use a smaller area.  Both seabios and ovmf will
> not map anyting below 0x80000000.  That is just fine.  As long as all
> pci bars get mapped inside the region declared in _CRS things will work.
> 
> While thinking about it:  There is one possible reason to not do it:
> Guest bugs.  IIRC windows doesn't like the 64bit pci window being larger
> than 2G.
>  Possibly that is an issue with the 32bit window too.  If that
> is the case we should indeed not use w32.begin values smaller than
> 0x8000000.  Igor, any clue?

I think the issues are all around 64 bit.
It's not hard to test.

> > Guys, I'm confused and annoyed out of my brains by the divergence here.
> > In my perception Michael, Igor, and Gerd are all proposing different
> > things, and my ideas are either rejected or ignored (even though they
> > occasionally overlap with ideas from others).
> 
> Hmm, as far as I can see there is an agreement that it is qemu's fault,
> the acpi tables need fixing, and ovmf should not need changes any
> changes.
> 
> Mimicing seabios/qemu logic in ovmf can be used as temporary stopgap
> while details are sorted on the qemu side.
> 
> cheers,
>   Gerd
> 

Re: [Qemu-devel] [PULL for-1.8 1/2] pc: disable pci-info

[Gerd Hoffmann]

  Hi,

> > > Yes, I think it's a start. Q35 is a bit harder because of the MMIO
> > > region.
> > 
> > ???  Do you mean mmconfig?  That can live inside the window.
> 
> Are you sure? When I tried windows crashed but maybe I'm wrong.
> Did you try some windows guests?

At least it looks that way on my laptop:

nilsson root ~# cat /proc/iomem 
[ ... ]
dfa00000-febfffff : PCI Bus 0000:00
  [ ... ]
  f8000000-fbffffff : PCI MMCONFIG 0000 [bus 00-3f]
    f80f8000-f80f8fff : reserved
fec00000-fec003ff : IOAPIC 0
[ ... ]

The reservation seems to be different from what we are doing though.  We
(aka seabios) are creating a e820 reserved region, on my laptop mmconfig
is declared in acpi:

nilsson root ~# grep f8000000 /var/log/dmesg
[    0.103320] PCI: MMCONFIG for domain 0000 [bus 00-3f] at [mem
0xf8000000-0xfbffffff] (base 0xf8000000)
[    0.126943] PCI: MMCONFIG for domain 0000 [bus 00-3f] at [mem
0xf8000000-0xfbffffff] (base 0xf8000000)
[    0.128697] PCI: MMCONFIG at [mem 0xf8000000-0xfbffffff] reserved in
ACPI motherboard resources
[    0.160979] system 00:01: [mem 0xf8000000-0xfbffffff] could not be
reserved

Maybe this is what windows doesn't like ...

cheers,
  Gerd

Re: [Qemu-devel] [PULL for-1.8 1/2] pc: disable pci-info

[Igor Mammedov]

On Tue, 26 Nov 2013 16:42:16 +0100
Gerd Hoffmann <kraxel@redhat.com> wrote:

>   Hi,
> 
> > This doesn't clamp the w32.begin value into [0x80000000, 0xe0000000],
> > which seems wrong.
> 
> Why?  In a 1G guest you can map pci bars at 0x40000000 just fine.
> 
> _CRS in acpi should declare the area where you can map pci bars, and
> that happens to be end-of-ram -> ioapci base.
> 
> Firmware can choose to use a smaller area.  Both seabios and ovmf will
> not map anyting below 0x80000000.  That is just fine.  As long as all
> pci bars get mapped inside the region declared in _CRS things will work.
> 
> While thinking about it:  There is one possible reason to not do it:
> Guest bugs.  IIRC windows doesn't like the 64bit pci window being larger
> than 2G.  Possibly that is an issue with the 32bit window too.  If that
> is the case we should indeed not use w32.begin values smaller than
> 0x8000000.  Igor, any clue?
I saw windows BSOD with present 64-bit CRS only on WS2003/XP
regardless of OS being 32 or 64 bit or CRS size. The newer versions
worked just fine.
Perhaps we shouldn't care about already broken case for soon to be EOLed OS? 

> 
> > Guys, I'm confused and annoyed out of my brains by the divergence here.
> > In my perception Michael, Igor, and Gerd are all proposing different
> > things, and my ideas are either rejected or ignored (even though they
> > occasionally overlap with ideas from others).
> 
> Hmm, as far as I can see there is an agreement that it is qemu's fault,
> the acpi tables need fixing, and ovmf should not need changes any
> changes.
> 
> Mimicing seabios/qemu logic in ovmf can be used as temporary stopgap
> while details are sorted on the qemu side.
> 
> cheers,
>   Gerd
> 
> 


-- 
Regards,
  Igor

Re: [Qemu-devel] [PULL for-1.8 1/2] pc: disable pci-info

[Igor Mammedov]

On Tue, 26 Nov 2013 15:11:01 +0100
Laszlo Ersek <lersek@redhat.com> wrote:

> On 11/26/13 10:10, Michael S. Tsirkin wrote:
> 
> > seabios manages to enumerate PCI with information exported from qemu
> > so why can't OVMF?
> 
> SeaBIOS and qemu duplicate logic (code) between each other.
> 
> src/fw/pciinit.c grabs RamSize over fw_cfg, and i440fx_mem_addr_setup()
> sets "pcimem_start" to one of three values based on RamSize. (One of the
> three is not explicit there, it's the build default 0xe0000000.)
> 
> The same code is visible in qemu in i440fx_init().
> 
> I duplicated the same logic in OVMF's PEI one week ago, and it solved
> the problem. See the patch attached to
> <http://thread.gmane.org/gmane.comp.bios.tianocore.devel/4881/focus=4995>.
> 
> But code/logic duplication is ugly, which is why I've been looking for a
> better, dynamic solution ever since.
> 
> > I think it's down to other qemu bugs (such as _CRS not covering
> > all of PCI memory), we shall just fix them.
> 
> I don't know how to fix them. I don't know how to enumerate all PCI
> regions in use, plus all unassigned ranges, from below, like with a
> MemoryListener.
> 
> If I understand correctly, Igor suggested to track devices as they map
> their regions, but (again, if I understood correctly) you didn't seem to
> like the idea.
Let me try to summarize what could be done:
 - let all not mapped area fall through to PCI address space (queued for 1.8
   on pci branch)
 - drop pci_info in QEMU completely (including variable/field)
 - during ACPI tables building find all not mapped/reserved regions in e820 map
   (that should take care about all present hardware)
   and create corresponding CRSes.
   there are caveats here:
     * Q35 can add/update mmconfig reservation in e820 table when programmed,
       so scanner won't have to know about it.
     * WS2003/XP case: it doesn't like 64-PCI windows CRS, so corresponding CRS
       shouldn't be created if there isn't any BARs there.
     * take care about explicit/forced creation of 64-bit PCI window CRS for
       PCI hotplug.
     * I don't know how to punch holes in PCI windows for BIOS originated
       reservations. (it's reverse problem, now we need BIOS update QEMU ACPI
       tables). Probably there is no need for it but I don't know SeaBIOS well
       enough.

> Thanks
> Laszlo
> 


-- 
Regards,
  Igor

Re: [Qemu-devel] [PULL for-1.8 1/2] pc: disable pci-info

[Michael S. Tsirkin]

On Tue, Nov 26, 2013 at 07:26:21PM +0100, Igor Mammedov wrote:
> On Tue, 26 Nov 2013 16:42:16 +0100
> Gerd Hoffmann <kraxel@redhat.com> wrote:
> 
> >   Hi,
> > 
> > > This doesn't clamp the w32.begin value into [0x80000000, 0xe0000000],
> > > which seems wrong.
> > 
> > Why?  In a 1G guest you can map pci bars at 0x40000000 just fine.
> > 
> > _CRS in acpi should declare the area where you can map pci bars, and
> > that happens to be end-of-ram -> ioapci base.
> > 
> > Firmware can choose to use a smaller area.  Both seabios and ovmf will
> > not map anyting below 0x80000000.  That is just fine.  As long as all
> > pci bars get mapped inside the region declared in _CRS things will work.
> > 
> > While thinking about it:  There is one possible reason to not do it:
> > Guest bugs.  IIRC windows doesn't like the 64bit pci window being larger
> > than 2G.  Possibly that is an issue with the 32bit window too.  If that
> > is the case we should indeed not use w32.begin values smaller than
> > 0x8000000.  Igor, any clue?
> I saw windows BSOD with present 64-bit CRS only on WS2003/XP
> regardless of OS being 32 or 64 bit or CRS size. The newer versions
> worked just fine.
> Perhaps we shouldn't care about already broken case for soon to be EOLed OS? 

It depends on how large you want to make it, there are some limits
for newer windows versions too.
But I never saw issues with 32 bit CRS.

> > 
> > > Guys, I'm confused and annoyed out of my brains by the divergence here.
> > > In my perception Michael, Igor, and Gerd are all proposing different
> > > things, and my ideas are either rejected or ignored (even though they
> > > occasionally overlap with ideas from others).
> > 
> > Hmm, as far as I can see there is an agreement that it is qemu's fault,
> > the acpi tables need fixing, and ovmf should not need changes any
> > changes.
> > 
> > Mimicing seabios/qemu logic in ovmf can be used as temporary stopgap
> > while details are sorted on the qemu side.
> > 
> > cheers,
> >   Gerd
> > 
> > 
> 
> 
> -- 
> Regards,
>   Igor

Re: [Qemu-devel] [PULL for-1.8 1/2] pc: disable pci-info

[Michael S. Tsirkin]

On Tue, Nov 26, 2013 at 04:59:33PM +0100, Gerd Hoffmann wrote:
> 
>   Hi,
> 
> > > > Yes, I think it's a start. Q35 is a bit harder because of the MMIO
> > > > region.
> > > 
> > > ???  Do you mean mmconfig?  That can live inside the window.
> > 
> > Are you sure? When I tried windows crashed but maybe I'm wrong.
> > Did you try some windows guests?
> 
> At least it looks that way on my laptop:
> 
> nilsson root ~# cat /proc/iomem 
> [ ... ]
> dfa00000-febfffff : PCI Bus 0000:00
>   [ ... ]
>   f8000000-fbffffff : PCI MMCONFIG 0000 [bus 00-3f]
>     f80f8000-f80f8fff : reserved
> fec00000-fec003ff : IOAPIC 0
> [ ... ]


True. Mine also has:
_CRS in DSDT:

                DWordMemory (ResourceProducer, PosDecode, MinFixed, MaxFixed, Cacheable, ReadWrite,
                    0x00000000,         // Granularity
                    0x00100000,         // Range Minimum
                    0xFEBFFFFF,         // Range Maximum
                    0x00000000,         // Translation Offset
                    0xFEB00000,         // Length
                    ,, _Y23, AddressRangeMemory, TypeStatic)

MCFG
[02Ch 0044  8]                 Base Address : 00000000F8000000
[034h 0052  2]         Segment Group Number : 0000
[036h 0054  1]             Start Bus Number : 00
[037h 0055  1]               End Bus Number : 3F
[038h 0056  4]                     Reserved : 00000000

so they do overlap apparently without issues.

Well, this definitely simplifies things: this probably means we can just
do the same thing for piix and q35.



> The reservation seems to be different from what we are doing though.  We
> (aka seabios) are creating a e820 reserved region, on my laptop mmconfig
> is declared in acpi:
> 
> nilsson root ~# grep f8000000 /var/log/dmesg
> [    0.103320] PCI: MMCONFIG for domain 0000 [bus 00-3f] at [mem
> 0xf8000000-0xfbffffff] (base 0xf8000000)
> [    0.126943] PCI: MMCONFIG for domain 0000 [bus 00-3f] at [mem
> 0xf8000000-0xfbffffff] (base 0xf8000000)
> [    0.128697] PCI: MMCONFIG at [mem 0xf8000000-0xfbffffff] reserved in
> ACPI motherboard resources
> [    0.160979] system 00:01: [mem 0xf8000000-0xfbffffff] could not be
> reserved
> 
> Maybe this is what windows doesn't like ...
> 
> cheers,
>   Gerd
> 
>