Re: [Qemu-devel] [PULL for-1.8 1/2] pc: disable pci-info
On 11/18/13 12:53, Michael S. Tsirkin wrote: From: Igor Mammedov imamm...@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 imamm...@redhat.com Reviewed-by: Gerd Hoffmann kra...@redhat.com Reviewed-by: Michael S. Tsirkin m...@redhat.com Reviewed-by: Eduardo Habkost ehabk...@redhat.com Signed-off-by: Michael S. Tsirkin m...@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.comforum_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
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 imamm...@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 imamm...@redhat.com Reviewed-by: Gerd Hoffmann kra...@redhat.com Reviewed-by: Michael S. Tsirkin m...@redhat.com Reviewed-by: Eduardo Habkost ehabk...@redhat.com Signed-off-by: Michael S. Tsirkin m...@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.comforum_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
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
From a81b8d66e24fd298ce7654d424a378337e6cf132 Mon Sep 17 00:00:00 2001
From: Gerd Hoffmann kra...@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 kra...@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 = 0x8000) {
-i440fx-pci_info.w32.begin = 0x8000;
-} else if (ram_size = 0xc000) {
-i440fx-pci_info.w32.begin = 0xc000;
-} else {
-i440fx-pci_info.w32.begin = 0xe000;
-}
+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
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 kra...@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 kra...@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 = 0x8000) {
-i440fx-pci_info.w32.begin = 0x8000;
-} else if (ram_size = 0xc000) {
-i440fx-pci_info.w32.begin = 0xc000;
-} else {
-i440fx-pci_info.w32.begin = 0xe000;
-}
+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
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 0xe000.) 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
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 kra...@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 kra...@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 = 0x8000) {
-i440fx-pci_info.w32.begin = 0x8000;
-} else if (ram_size = 0xc000) {
-i440fx-pci_info.w32.begin = 0xc000;
-} else {
-i440fx-pci_info.w32.begin = 0xe000;
-}
+i440fx-pci_info.w32.begin = ram_size;
This doesn't clamp the w32.begin value into [0x8000, 0xe000],
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
---BeginMessage---
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 = 0x8000) {
i440fx-pci_info.w32.begin = 0x8000;
-} else if (ram_size = 0xc000) {
-i440fx-pci_info.w32.begin = 0xc000;
+} else if (ram_size = 0xe000) {
+i440fx-pci_info.w32.begin = ram_size;
} else {
i440fx-pci_info.w32.begin = 0xe000;
}
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
Re: [Qemu-devel] [PULL for-1.8 1/2] pc: disable pci-info
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 kra...@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 kra...@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 = 0x8000) {
-i440fx-pci_info.w32.begin = 0x8000;
-} else if (ram_size = 0xc000) {
-i440fx-pci_info.w32.begin = 0xc000;
-} else {
-i440fx-pci_info.w32.begin = 0xe000;
-}
+i440fx-pci_info.w32.begin = ram_size;
This doesn't clamp the w32.begin value into [0x8000, 0xe000],
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 ler...@redhat.com
To: Michael S. Tsirkin m...@redhat.com
CC: Igor Mammedov imamm...@redhat.com
Subject: Re: changing the pci32 window in qemu's DSDT/SSDT
Message-ID: 529362e2.1040...@redhat.com
In-Reply-To: 20131125132400.ga1...@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 =
Re: [Qemu-devel] [PULL for-1.8 1/2] pc: disable pci-info
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
From 3d01b6c46fbf655bdb9b4f7ca427f40959b05d31 Mon Sep 17 00:00:00 2001
From: Gerd Hoffmann kra...@redhat.com
Date: Tue, 26 Nov 2013 16:18:04 +0100
Subject: [PATCH] [wip] q35: fix 32bit pci hole
Signed-off-by: Gerd Hoffmann kra...@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)
0x1ULL - 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, 0x1ULL + mch-above_4g_mem_size,
--
1.8.3.1
Re: [Qemu-devel] [PULL for-1.8 1/2] pc: disable pci-info
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 kra...@redhat.com
Date: Tue, 26 Nov 2013 16:18:04 +0100
Subject: [PATCH] [wip] q35: fix 32bit pci hole
Signed-off-by: Gerd Hoffmann kra...@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)
0x1ULL - 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, 0x1ULL +
mch-above_4g_mem_size,
--
1.8.3.1
Re: [Qemu-devel] [PULL for-1.8 1/2] pc: disable pci-info
Hi, This doesn't clamp the w32.begin value into [0x8000, 0xe000], which seems wrong. Why? In a 1G guest you can map pci bars at 0x4000 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 0x8000. 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 0x800. 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
On Tue, Nov 26, 2013 at 04:42:16PM +0100, Gerd Hoffmann wrote: Hi, This doesn't clamp the w32.begin value into [0x8000, 0xe000], which seems wrong. Why? In a 1G guest you can map pci bars at 0x4000 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 0x8000. 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 0x800. 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
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 [ ... ] dfa0-febf : PCI Bus :00 [ ... ] f800-fbff : PCI MMCONFIG [bus 00-3f] f80f8000-f80f8fff : reserved fec0-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 f800 /var/log/dmesg [0.103320] PCI: MMCONFIG for domain [bus 00-3f] at [mem 0xf800-0xfbff] (base 0xf800) [0.126943] PCI: MMCONFIG for domain [bus 00-3f] at [mem 0xf800-0xfbff] (base 0xf800) [0.128697] PCI: MMCONFIG at [mem 0xf800-0xfbff] reserved in ACPI motherboard resources [0.160979] system 00:01: [mem 0xf800-0xfbff] 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
On Tue, 26 Nov 2013 16:42:16 +0100 Gerd Hoffmann kra...@redhat.com wrote: Hi, This doesn't clamp the w32.begin value into [0x8000, 0xe000], which seems wrong. Why? In a 1G guest you can map pci bars at 0x4000 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 0x8000. 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 0x800. 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
On Tue, 26 Nov 2013 15:11:01 +0100 Laszlo Ersek ler...@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 0xe000.) 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
On Tue, Nov 26, 2013 at 07:26:21PM +0100, Igor Mammedov wrote: On Tue, 26 Nov 2013 16:42:16 +0100 Gerd Hoffmann kra...@redhat.com wrote: Hi, This doesn't clamp the w32.begin value into [0x8000, 0xe000], which seems wrong. Why? In a 1G guest you can map pci bars at 0x4000 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 0x8000. 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 0x800. 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
[Qemu-devel] [PULL for-1.8 1/2] pc: disable pci-info
From: Igor Mammedov imamm...@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 imamm...@redhat.com
Reviewed-by: Gerd Hoffmann kra...@redhat.com
Reviewed-by: Michael S. Tsirkin m...@redhat.com
Reviewed-by: Eduardo Habkost ehabk...@redhat.com
Signed-off-by: Michael S. Tsirkin m...@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
