| Message ID | 20141210171359.GA3301@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Wed, 10 Dec 2014 19:13:59 +0200 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Wed, Dec 10, 2014 at 05:45:58PM +0100, Igor Mammedov wrote: > > On Wed, 10 Dec 2014 18:24:24 +0200 > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > On Wed, Dec 10, 2014 at 04:06:56PM +0000, Igor Mammedov wrote: > > > > linker and RSDP tables are build only once, so if later > > > > during rebuild sizes of other ACPI tables change > > > > pointers will be patched incorrectly due to wrong > > > > offsets. > > > > > > > > To fix it rebuild linker and RSDP tables along with > > > > the rest of ACPI tables so that they would have correct > > > > offsets. > > > > > > > > Here is a simple reproducer: > > > > 1: hotplug bridge using command: > > > > device_add pci-bridge,chassis_nr=1 > > > > 2: reset system from monitor: > > > > system_reset > > > > > > > > As result pointers to ACPI tables are not correct > > > > and guest can't read/parse ACPI tables. > > > > Windows guests just refuse to boot and > > > > Linux guests are more resilient and try to boot without > > > > ACPI, sometimes successfully. > > > > > > > > PS: > > > > keep brokenness in 2.2 and older machine types for the sake > > > > of migration > > > > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > > > > I'd like to fix 2.2 as well. > > that's easy, > > has_imutable_rsdp = true > > should be moved into > > pc_compat_2_1() > > > > > > > > Why does adding bridge in this way cause tables to > > > be resized? > > > I think this is a root-cause that should be fixed. > > That's because there wasn't any bridge described in tables > > on startup, but after hotplug and reset tables are rebuild > > which pickups hotplugged bridge and its secondary PCI bus. > > > > This behavior seems sane, I'd expect bare-metal behave > > the same way. > > For a simple fix, we can skip bridges added by hotplug - > treat them as regular devices. > > It defintely seems nicer - at least for old machine types - > than "keep brokenness". > > I think the following should do the trcik, but it's completely > untested. > Could you pls try? > > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > index b37a397..039776d 100644 > --- a/hw/i386/acpi-build.c > +++ b/hw/i386/acpi-build.c > @@ -844,7 +844,8 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state) > * Skip bridge subtree creation if bridge hotplug is disabled > * to make acpi tables compatible with legacy machine types. > */ > - if (!child->pcihp_bridge_en && bus->parent_dev) { > + if (bus->parent_dev && (!child->pcihp_bridge_en || > + !DEVICE(bus->parent_dev)->hotplugged)) { > return; > } What will happen if hotplug may bridges in QEMU-fixed^^ -M 2.1 migrate to target: QEMU-2.1 -M 2.1 + hotplugged on sourcebridges on CLI My guess it may fail due to ACPI blob size difference.
On Wed, Dec 10, 2014 at 06:39:49PM +0100, Igor Mammedov wrote: > On Wed, 10 Dec 2014 19:13:59 +0200 > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > On Wed, Dec 10, 2014 at 05:45:58PM +0100, Igor Mammedov wrote: > > > On Wed, 10 Dec 2014 18:24:24 +0200 > > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > > > On Wed, Dec 10, 2014 at 04:06:56PM +0000, Igor Mammedov wrote: > > > > > linker and RSDP tables are build only once, so if later > > > > > during rebuild sizes of other ACPI tables change > > > > > pointers will be patched incorrectly due to wrong > > > > > offsets. > > > > > > > > > > To fix it rebuild linker and RSDP tables along with > > > > > the rest of ACPI tables so that they would have correct > > > > > offsets. > > > > > > > > > > Here is a simple reproducer: > > > > > 1: hotplug bridge using command: > > > > > device_add pci-bridge,chassis_nr=1 > > > > > 2: reset system from monitor: > > > > > system_reset > > > > > > > > > > As result pointers to ACPI tables are not correct > > > > > and guest can't read/parse ACPI tables. > > > > > Windows guests just refuse to boot and > > > > > Linux guests are more resilient and try to boot without > > > > > ACPI, sometimes successfully. > > > > > > > > > > PS: > > > > > keep brokenness in 2.2 and older machine types for the sake > > > > > of migration > > > > > > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > > > > > > I'd like to fix 2.2 as well. > > > that's easy, > > > has_imutable_rsdp = true > > > should be moved into > > > pc_compat_2_1() > > > > > > > > > > > Why does adding bridge in this way cause tables to > > > > be resized? > > > > I think this is a root-cause that should be fixed. > > > That's because there wasn't any bridge described in tables > > > on startup, but after hotplug and reset tables are rebuild > > > which pickups hotplugged bridge and its secondary PCI bus. > > > > > > This behavior seems sane, I'd expect bare-metal behave > > > the same way. > > > > For a simple fix, we can skip bridges added by hotplug - > > treat them as regular devices. > > > > It defintely seems nicer - at least for old machine types - > > than "keep brokenness". > > > > I think the following should do the trcik, but it's completely > > untested. > > Could you pls try? > > > > > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > > index b37a397..039776d 100644 > > --- a/hw/i386/acpi-build.c > > +++ b/hw/i386/acpi-build.c > > @@ -844,7 +844,8 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state) > > * Skip bridge subtree creation if bridge hotplug is disabled > > * to make acpi tables compatible with legacy machine types. > > */ > > - if (!child->pcihp_bridge_en && bus->parent_dev) { > > + if (bus->parent_dev && (!child->pcihp_bridge_en || > > + !DEVICE(bus->parent_dev)->hotplugged)) { > > return; > > } > What will happen if > hotplug may bridges in > QEMU-fixed^^ -M 2.1 > migrate to target: > QEMU-2.1 -M 2.1 + hotplugged on sourcebridges on CLI > > My guess it may fail due to ACPI blob size difference. Yes it might if we are unlucky and cross a 64 k boundary. The robust fix is still to resize ROMs on receive. We'll also put this patch on a stable branch.
On Wed, 10 Dec 2014 20:28:01 +0200 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Wed, Dec 10, 2014 at 06:39:49PM +0100, Igor Mammedov wrote: > > On Wed, 10 Dec 2014 19:13:59 +0200 > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > On Wed, Dec 10, 2014 at 05:45:58PM +0100, Igor Mammedov wrote: > > > > On Wed, 10 Dec 2014 18:24:24 +0200 > > > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > > > > > On Wed, Dec 10, 2014 at 04:06:56PM +0000, Igor Mammedov wrote: > > > > > > linker and RSDP tables are build only once, so if later > > > > > > during rebuild sizes of other ACPI tables change > > > > > > pointers will be patched incorrectly due to wrong > > > > > > offsets. > > > > > > > > > > > > To fix it rebuild linker and RSDP tables along with > > > > > > the rest of ACPI tables so that they would have correct > > > > > > offsets. > > > > > > > > > > > > Here is a simple reproducer: > > > > > > 1: hotplug bridge using command: > > > > > > device_add pci-bridge,chassis_nr=1 > > > > > > 2: reset system from monitor: > > > > > > system_reset > > > > > > > > > > > > As result pointers to ACPI tables are not correct > > > > > > and guest can't read/parse ACPI tables. > > > > > > Windows guests just refuse to boot and > > > > > > Linux guests are more resilient and try to boot without > > > > > > ACPI, sometimes successfully. > > > > > > > > > > > > PS: > > > > > > keep brokenness in 2.2 and older machine types for the sake > > > > > > of migration > > > > > > > > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > > > > > > > > I'd like to fix 2.2 as well. > > > > that's easy, > > > > has_imutable_rsdp = true > > > > should be moved into > > > > pc_compat_2_1() > > > > > > > > > > > > > > Why does adding bridge in this way cause tables to > > > > > be resized? > > > > > I think this is a root-cause that should be fixed. > > > > That's because there wasn't any bridge described in tables > > > > on startup, but after hotplug and reset tables are rebuild > > > > which pickups hotplugged bridge and its secondary PCI bus. > > > > > > > > This behavior seems sane, I'd expect bare-metal behave > > > > the same way. > > > > > > For a simple fix, we can skip bridges added by hotplug - > > > treat them as regular devices. > > > > > > It defintely seems nicer - at least for old machine types - > > > than "keep brokenness". > > > > > > I think the following should do the trcik, but it's completely > > > untested. > > > Could you pls try? > > > > > > > > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > > > index b37a397..039776d 100644 > > > --- a/hw/i386/acpi-build.c > > > +++ b/hw/i386/acpi-build.c > > > @@ -844,7 +844,8 @@ static void build_pci_bus_end(PCIBus *bus, > > > void *bus_state) > > > * Skip bridge subtree creation if bridge hotplug is disabled > > > * to make acpi tables compatible with legacy machine types. > > > */ > > > - if (!child->pcihp_bridge_en && bus->parent_dev) { > > > + if (bus->parent_dev && (!child->pcihp_bridge_en || > > > + !DEVICE(bus->parent_dev)->hotplugged)) > > > { return; > > > } > > What will happen if > > hotplug may bridges in > > QEMU-fixed^^ -M 2.1 > > migrate to target: > > QEMU-2.1 -M 2.1 + hotplugged on sourcebridges on CLI > > > > My guess it may fail due to ACPI blob size difference. > > Yes it might if we are unlucky and cross a 64 k boundary. > The robust fix is still to resize ROMs on receive. > We'll also put this patch on a stable branch. > Well, in my opinion though above snippet is sipmlier but it's papering over the bug in table linker and RSDP, instead of fixing it. In this case SSDT size change is quite legetimate and in future there could be other cases when it changes. So it would be better fix the root problem which is corrupting tables due to changed SSDT size on reboot instead of pretending that issue doesn't exist.
On Wed, Dec 10, 2014 at 08:43:55PM +0100, Igor Mammedov wrote: > On Wed, 10 Dec 2014 20:28:01 +0200 > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > On Wed, Dec 10, 2014 at 06:39:49PM +0100, Igor Mammedov wrote: > > > On Wed, 10 Dec 2014 19:13:59 +0200 > > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > > > On Wed, Dec 10, 2014 at 05:45:58PM +0100, Igor Mammedov wrote: > > > > > On Wed, 10 Dec 2014 18:24:24 +0200 > > > > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > > > > > > > On Wed, Dec 10, 2014 at 04:06:56PM +0000, Igor Mammedov wrote: > > > > > > > linker and RSDP tables are build only once, so if later > > > > > > > during rebuild sizes of other ACPI tables change > > > > > > > pointers will be patched incorrectly due to wrong > > > > > > > offsets. > > > > > > > > > > > > > > To fix it rebuild linker and RSDP tables along with > > > > > > > the rest of ACPI tables so that they would have correct > > > > > > > offsets. > > > > > > > > > > > > > > Here is a simple reproducer: > > > > > > > 1: hotplug bridge using command: > > > > > > > device_add pci-bridge,chassis_nr=1 > > > > > > > 2: reset system from monitor: > > > > > > > system_reset > > > > > > > > > > > > > > As result pointers to ACPI tables are not correct > > > > > > > and guest can't read/parse ACPI tables. > > > > > > > Windows guests just refuse to boot and > > > > > > > Linux guests are more resilient and try to boot without > > > > > > > ACPI, sometimes successfully. > > > > > > > > > > > > > > PS: > > > > > > > keep brokenness in 2.2 and older machine types for the sake > > > > > > > of migration > > > > > > > > > > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > > > > > > > > > > I'd like to fix 2.2 as well. > > > > > that's easy, > > > > > has_imutable_rsdp = true > > > > > should be moved into > > > > > pc_compat_2_1() > > > > > > > > > > > > > > > > > Why does adding bridge in this way cause tables to > > > > > > be resized? > > > > > > I think this is a root-cause that should be fixed. > > > > > That's because there wasn't any bridge described in tables > > > > > on startup, but after hotplug and reset tables are rebuild > > > > > which pickups hotplugged bridge and its secondary PCI bus. > > > > > > > > > > This behavior seems sane, I'd expect bare-metal behave > > > > > the same way. > > > > > > > > For a simple fix, we can skip bridges added by hotplug - > > > > treat them as regular devices. > > > > > > > > It defintely seems nicer - at least for old machine types - > > > > than "keep brokenness". > > > > > > > > I think the following should do the trcik, but it's completely > > > > untested. > > > > Could you pls try? > > > > > > > > > > > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > > > > index b37a397..039776d 100644 > > > > --- a/hw/i386/acpi-build.c > > > > +++ b/hw/i386/acpi-build.c > > > > @@ -844,7 +844,8 @@ static void build_pci_bus_end(PCIBus *bus, > > > > void *bus_state) > > > > * Skip bridge subtree creation if bridge hotplug is disabled > > > > * to make acpi tables compatible with legacy machine types. > > > > */ > > > > - if (!child->pcihp_bridge_en && bus->parent_dev) { > > > > + if (bus->parent_dev && (!child->pcihp_bridge_en || > > > > + !DEVICE(bus->parent_dev)->hotplugged)) > > > > { return; > > > > } > > > What will happen if > > > hotplug may bridges in > > > QEMU-fixed^^ -M 2.1 > > > migrate to target: > > > QEMU-2.1 -M 2.1 + hotplugged on sourcebridges on CLI > > > > > > My guess it may fail due to ACPI blob size difference. > > > > Yes it might if we are unlucky and cross a 64 k boundary. > > The robust fix is still to resize ROMs on receive. > > We'll also put this patch on a stable branch. > > > > Well, in my opinion though above snippet is sipmlier but it's papering > over the bug in table linker and RSDP, instead of fixing it. I'm not against making the change for new machine types, but I'm also looking for a fix for 2.2.1 > In this case SSDT size change is quite legetimate and in future there > could be other cases when it changes. So it would be better fix the root > problem which is corrupting tables due to changed SSDT size on reboot > instead of pretending that issue doesn't exist. For -M 2.3 and up, I'm fine with this approach generally. Of course it all only works on the assumption we make ROM regions resizeable. If not your patch creates migration problems: -hot-add bridge on source -hot-add on destination Size now depends on whether source rebooted, destination has no way to know that.
On Wed, 10 Dec 2014 22:18:53 +0200 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Wed, Dec 10, 2014 at 08:43:55PM +0100, Igor Mammedov wrote: > > On Wed, 10 Dec 2014 20:28:01 +0200 > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > On Wed, Dec 10, 2014 at 06:39:49PM +0100, Igor Mammedov wrote: > > > > On Wed, 10 Dec 2014 19:13:59 +0200 > > > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > > > > > On Wed, Dec 10, 2014 at 05:45:58PM +0100, Igor Mammedov wrote: > > > > > > On Wed, 10 Dec 2014 18:24:24 +0200 > > > > > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > > > > > > > > > On Wed, Dec 10, 2014 at 04:06:56PM +0000, Igor Mammedov wrote: > > > > > > > > linker and RSDP tables are build only once, so if later > > > > > > > > during rebuild sizes of other ACPI tables change > > > > > > > > pointers will be patched incorrectly due to wrong > > > > > > > > offsets. > > > > > > > > > > > > > > > > To fix it rebuild linker and RSDP tables along with > > > > > > > > the rest of ACPI tables so that they would have correct > > > > > > > > offsets. > > > > > > > > > > > > > > > > Here is a simple reproducer: > > > > > > > > 1: hotplug bridge using command: > > > > > > > > device_add pci-bridge,chassis_nr=1 > > > > > > > > 2: reset system from monitor: > > > > > > > > system_reset > > > > > > > > > > > > > > > > As result pointers to ACPI tables are not correct > > > > > > > > and guest can't read/parse ACPI tables. > > > > > > > > Windows guests just refuse to boot and > > > > > > > > Linux guests are more resilient and try to boot without > > > > > > > > ACPI, sometimes successfully. > > > > > > > > > > > > > > > > PS: > > > > > > > > keep brokenness in 2.2 and older machine types for the sake > > > > > > > > of migration > > > > > > > > > > > > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > > > > > > > > > > > > I'd like to fix 2.2 as well. > > > > > > that's easy, > > > > > > has_imutable_rsdp = true > > > > > > should be moved into > > > > > > pc_compat_2_1() > > > > > > > > > > > > > > > > > > > > Why does adding bridge in this way cause tables to > > > > > > > be resized? > > > > > > > I think this is a root-cause that should be fixed. > > > > > > That's because there wasn't any bridge described in tables > > > > > > on startup, but after hotplug and reset tables are rebuild > > > > > > which pickups hotplugged bridge and its secondary PCI bus. > > > > > > > > > > > > This behavior seems sane, I'd expect bare-metal behave > > > > > > the same way. > > > > > > > > > > For a simple fix, we can skip bridges added by hotplug - > > > > > treat them as regular devices. > > > > > > > > > > It defintely seems nicer - at least for old machine types - > > > > > than "keep brokenness". > > > > > > > > > > I think the following should do the trcik, but it's completely > > > > > untested. > > > > > Could you pls try? > > > > > > > > > > > > > > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > > > > > index b37a397..039776d 100644 > > > > > --- a/hw/i386/acpi-build.c > > > > > +++ b/hw/i386/acpi-build.c > > > > > @@ -844,7 +844,8 @@ static void build_pci_bus_end(PCIBus *bus, > > > > > void *bus_state) > > > > > * Skip bridge subtree creation if bridge hotplug is disabled > > > > > * to make acpi tables compatible with legacy machine types. > > > > > */ > > > > > - if (!child->pcihp_bridge_en && bus->parent_dev) { > > > > > + if (bus->parent_dev && (!child->pcihp_bridge_en || > > > > > + !DEVICE(bus->parent_dev)->hotplugged)) > > > > > { return; > > > > > } > > > > What will happen if > > > > hotplug may bridges in > > > > QEMU-fixed^^ -M 2.1 > > > > migrate to target: > > > > QEMU-2.1 -M 2.1 + hotplugged on sourcebridges on CLI > > > > > > > > My guess it may fail due to ACPI blob size difference. > > > > > > Yes it might if we are unlucky and cross a 64 k boundary. > > > The robust fix is still to resize ROMs on receive. > > > We'll also put this patch on a stable branch. > > > > > > > Well, in my opinion though above snippet is sipmlier but it's papering > > over the bug in table linker and RSDP, instead of fixing it. > > I'm not against making the change for new machine types, > but I'm also looking for a fix for 2.2.1 we can ignore 2.2.0, like Paolo did with yesterday's migration bugs and fix 2.2.1 with this patch. > > > > In this case SSDT size change is quite legetimate and in future there > > could be other cases when it changes. So it would be better fix the root > > problem which is corrupting tables due to changed SSDT size on reboot > > instead of pretending that issue doesn't exist. > > For -M 2.3 and up, I'm fine with this approach generally. > Of course it all only works on the assumption we make > ROM regions resizeable. > If not your patch creates migration problems: > -hot-add bridge on source > -hot-add on destination > > Size now depends on whether source rebooted, destination > has no way to know that. Patch of cause doesn't fix migration issue (and hasn't been meant to), it just highlights already existing migration issue more prominently. And it's one more argument in favor of resize-able ROMs on migration or if it's not acceptable then moving blob into DeviceState where we can freely adjust size on target.
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c index b37a397..039776d 100644 --- a/hw/i386/acpi-build.c +++ b/hw/i386/acpi-build.c @@ -844,7 +844,8 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state) * Skip bridge subtree creation if bridge hotplug is disabled * to make acpi tables compatible with legacy machine types. */ - if (!child->pcihp_bridge_en && bus->parent_dev) { + if (bus->parent_dev && (!child->pcihp_bridge_en || + !DEVICE(bus->parent_dev)->hotplugged)) { return; }