On Fri, Aug 02, 2024 at 09:34:54 +0200, Boris Fiuczynski wrote: > On 8/1/24 3:52 PM, Peter Krempa wrote: > > The s390(x) machines never supported ACPI. That didn't stop users > > enabling ACPI in their config. As of libvirt-9.2 (98c4e3d073) with new > > enough qemu we reject configs which require ACPI, but qemu can't satisfy > > it. > > > > This breaks migration of existing VMs with the old wrong configs to new > > libvirt installations. > > > > To address this introduce a post-parse fixup removing the ACPI flag > > specifically for s390 machines which do enable it in the definition. > > > > The advantage of doing it in post-parse, rather than simply relaxing the > > ABI stability check to allow users providing an fixed XML when migrating > > (allowing change of the ACPI flag for s390 in ABI stability check, as it > > doesn't impact ABI), is that only the destination installation needs to > > be patched in order to preserve migration. > > > > To mitigate the disadvantage of simply stripping it from all s390(x) > > configs the hack is not applied when defining or starting a new domain > > from the XML, to preserve the error about unsupported configuration. > > > > Resolves: https://issues.redhat.com/browse/RHEL-49516 > > Signed-off-by: Peter Krempa <pkrempa@xxxxxxxxxx> > > --- > > src/qemu/qemu_domain.c | 47 ++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 47 insertions(+) > > > > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c > > index 198ab99aef..a7a34e66e2 100644 > > --- a/src/qemu/qemu_domain.c > > +++ b/src/qemu/qemu_domain.c > > @@ -5020,6 +5020,51 @@ qemuDomainDefPostParseBasic(virDomainDef *def, > > } > > > > > > +/** > > + * qemuDomainDefACPIPostParse: > > + * @def: domain definition > > + * @qemuCaps: qemu capabilities object > > + * > > + * Fixup the use of ACPI flag on certain architectures that never supported it > > + * and users for some reason used it, which would break migration to newer > > + * libvirt versions which check whether given machine type supports ACPI. > > + * > > + * The fixup is done in post-parse as it's hard to update the ABI stability > > + * check on source of the migration. > > + */ > > +static void > > +qemuDomainDefACPIPostParse(virDomainDef *def, > > + virQEMUCaps *qemuCaps, > > + unsigned int parseFlags) > > +{ > > + /* Only cases when ACPI is enabled need to be fixed up */ > > + if (def->features[VIR_DOMAIN_FEATURE_ACPI] != VIR_TRISTATE_SWITCH_ON) > > + return; > > + > > + /* This fixup is applicable _only_ on architectures which were present as of > > + * libvirt-9.2 and *never* supported ACPI. The fixup is currently done only > > + * for existing users of s390(x) to fix migration for configs which had > > + * <acpi/> despite being ignored. > > + */ > > + if (def->os.arch != VIR_ARCH_S390 && > > + def->os.arch != VIR_ARCH_S390X) > > + return; > > In v1 your code has been generic but now all code below this point is S390 > only. > Therefor I suggest to move this check below the parseFlags check and also to > inverse the arch check and do the stripping inside the if block. Yeah, definitely changing the order of the conditions makes sense. I'll reconsider also the arch check, but I'm inclined to retain the 'return early' semantics here. > > + > > + /* Strip the <acpi/> feature only for non-fresh configs, in order to still > > + * produce an error if the feature is present in a newly defined one. > > + * > > + * The use of the VIR_DOMAIN_DEF_PARSE_ABI_UPDATE looks counter-intuitive, > > + * but it's used only in qemuDomainCreateXML/qemuDomainDefineXMLFlags APIs > > + * */ > > + if (parseFlags & VIR_DOMAIN_DEF_PARSE_ABI_UPDATE) > > + return; > > Scoping this to migration only potentially enforces managements tools using > libvirt to change their previously working code creating domains. As this Well yes, that was the intention. I've originally outlined this intention in v1 where I've stated that the drawback of the patch is that it does not retain such check. Andrea prodded me a bit to look into the use of _ABI_UPDATE which I didn't originally consider as feasible. > has not been working since 9.2.0 I guess the risc is low and enforcing this > to get it changed has to be done at some point anyhow. Therefor I am fine > with this. > > > + > > + /* To be sure, we only strip ACPI if given machine type doesn't support it */ > > + if (virQEMUCapsMachineSupportsACPI(qemuCaps, def->virtType, def->os.machine) == VIR_TRISTATE_BOOL_NO) > > + def->features[VIR_DOMAIN_FEATURE_ACPI] = VIR_TRISTATE_SWITCH_ABSENT; > > +} > > + > > + > > static int > > qemuDomainDefPostParse(virDomainDef *def, > > unsigned int parseFlags, > > @@ -5040,6 +5085,8 @@ qemuDomainDefPostParse(virDomainDef *def, > > if (qemuDomainDefMachinePostParse(def, qemuCaps) < 0) > > return -1; > > > > + qemuDomainDefACPIPostParse(def, qemuCaps, parseFlags); > > + > > if (qemuDomainDefBootPostParse(def, driver, parseFlags) < 0) > > return -1; > > > > Anyway > Reviewed-by: Boris Fiuczynski <fiuczy@xxxxxxxxxxxxx> > > -- > Mit freundlichen Grüßen/Kind regards > Boris Fiuczynski > > IBM Deutschland Research & Development GmbH > Vorsitzender des Aufsichtsrats: Wolfgang Wendt > Geschäftsführung: David Faller > Sitz der Gesellschaft: Böblingen > Registergericht: Amtsgericht Stuttgart, HRB 243294 >
