|
From: | Philippe Mathieu-Daudé |
Subject: | Re: [PATCH for-7.1] hw/mips/malta: turn off x86 specific features of PIIX4_PM |
Date: | Tue, 2 Aug 2022 08:34:42 +0200 |
User-agent: | Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0) Gecko/20100101 Thunderbird/91.11.0 |
On 28/7/22 15:16, Igor Mammedov wrote:
On Thu, 28 Jul 2022 13:29:07 +0100 Peter Maydell <peter.maydell@linaro.org> wrote:On Thu, 28 Jul 2022 at 12:50, Igor Mammedov <imammedo@redhat.com> wrote:QEMU crashes trying to save VMSTATE when only MIPS target are compiled in $ qemu-system-mips -monitor stdio (qemu) migrate "exec:gzip -c > STATEFILE.gz" Segmentation fault (core dumped) It happens due to PIIX4_PM trying to parse hotplug vmstate structures which are valid only for x86 and not for MIPS (as it requires ACPI tables support which is not existent for ithe later)
We already discussed this Frankenstein PIIX4 problem 2 and 4 years ago: https://lore.kernel.org/qemu-devel/4d42697e-ba84-e5af-3a17-a2cc52cf0dbc@redhat.com/ https://lore.kernel.org/qemu-devel/20190304210359-mutt-send-email-mst@kernel.org/
Issue was probably exposed by trying to cleanup/compile out unused ACPI bits from MIPS target (but forgetting about migration bits). Disable compiled out features using compat properties as the least risky way to deal with issue.
So now MIPS is forced to use meaningless compat[] to satisfy X86. Am I wrong seeing this as a dirty hack creeping in, yet another technical debt that will hit (me...) back in a close future? Are we sure there are no better solution (probably more time consuming and involving refactors) we could do instead? Thanks, Phil.
Signed-off-by: Igor Mammedov <imammedo@redhat.com>Resolves: https://gitlab.com/qemu-project/qemu/-/issues/995--- PS: another approach could be setting defaults to disabled state and enabling them using compat props on PC machines (which is more code to deal with => more risky) or continue with PIIX4_PM refactoring to split x86-shism out (which I'm not really interested in due to risk of regressions for not much of benefit) --- hw/mips/malta.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/hw/mips/malta.c b/hw/mips/malta.c index 7a0ec513b0..0e932988e0 100644 --- a/hw/mips/malta.c +++ b/hw/mips/malta.c @@ -1442,6 +1442,14 @@ static const TypeInfo mips_malta_device = { .instance_init = mips_malta_instance_init, }; +GlobalProperty malta_compat[] = { + { "PIIX4_PM", "memory-hotplug-support", "off" }, + { "PIIX4_PM", "acpi-pci-hotplug-with-bridge-support", "off" }, + { "PIIX4_PM", "acpi-root-pci-hotplug", "off" }, + { "PIIX4_PM", "x-not-migrate-acpi-index", "true" }, +};Is there an easy way to assert in hw/acpi/piix4.c that if CONFIG_ACPI_PCIHP was not set then the board has initialized all these properties to the don't-use-hotplug state ? That would be a guard against similar bugs (though I suppose we probably aren't likely to add new piix4 boards...)unfortunately new features still creep in 'pc' machine ex: "acpi-root-pci-hotplug"), and I don't see an easy way to compile that nor enforce that in the future. Far from easy would be split piix4_pm on base/enhanced classes so we wouldn't need x86 specific hacks in 'base' variant (assuming 'enhanced' could maintain the current VMSTATE to keep cross-version migration working).+const size_t malta_compat_len = G_N_ELEMENTS(malta_compat); + static void mips_malta_machine_init(MachineClass *mc) { mc->desc = "MIPS Malta Core LV"; @@ -1455,6 +1463,7 @@ static void mips_malta_machine_init(MachineClass *mc) mc->default_cpu_type = MIPS_CPU_TYPE_NAME("24Kf"); #endif mc->default_ram_id = "mips_malta.ram"; + compat_props_add(mc->compat_props, malta_compat, malta_compat_len); } DEFINE_MACHINE("malta", mips_malta_machine_init) -- 2.31.1thanks -- PMM
[Prev in Thread] | Current Thread | [Next in Thread] |