On Tue, Nov 21, 2017 at 05:42:25PM +0100, Andrea Bolognani wrote: > We can finally introduce a specific target model for the spapr-vty > device used by pSeries guests, which means isa-serial will no longer > show up to confuse users. > > We make sure migration works in both directions by interpreting the > isa-serial target type, or the lack of target type, appropriately > when parsing the guest XML, and skipping the newly-introduced type > when formatting if for migration. We also verify that spapr-vty is > not used for non-pSeries guests and add a bunch of test cases. > > This commit is best viewed with 'git diff -w'. s/diff/show/ since the change would be most likely committed :). > > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1511421 > > Signed-off-by: Andrea Bolognani <abologna@xxxxxxxxxx> > --- > docs/formatdomain.html.in | 11 ++-- > docs/schemas/domaincommon.rng | 2 + > src/conf/domain_conf.c | 4 ++ > src/conf/domain_conf.h | 2 + > src/qemu/qemu_command.c | 74 ++++++++++------------ > src/qemu/qemu_domain.c | 74 ++++++++++++++++++---- > src/qemu/qemu_domain_address.c | 1 + > .../qemuxml2argv-pseries-basic.args | 2 +- > .../qemuxml2argv-pseries-console-native.args | 1 + > .../qemuxml2argv-pseries-console-native.xml | 17 +++++ > ...gs => qemuxml2argv-pseries-console-virtio.args} | 10 +-- > .../qemuxml2argv-pseries-console-virtio.xml | 19 ++++++ > .../qemuxml2argv-pseries-cpu-compat-power9.args | 2 +- > .../qemuxml2argv-pseries-cpu-compat.args | 2 +- > .../qemuxml2argv-pseries-cpu-exact.args | 2 +- > .../qemuxml2argv-pseries-cpu-le.args | 2 +- > .../qemuxml2argv-pseries-panic-missing.args | 2 +- > .../qemuxml2argv-pseries-panic-no-address.args | 2 +- > ...qemuxml2argv-pseries-serial+console-native.args | 1 + > .../qemuxml2argv-pseries-serial+console-native.xml | 18 ++++++ > .../qemuxml2argv-pseries-serial-compat.args | 1 + > .../qemuxml2argv-pseries-serial-compat.xml | 19 ++++++ > ...qemuxml2argv-pseries-serial-invalid-machine.xml | 19 ++++++ > ...rgs => qemuxml2argv-pseries-serial-native.args} | 7 +- > .../qemuxml2argv-pseries-serial-native.xml | 16 +++++ > .../qemuxml2argv-pseries-usb-default.args | 2 +- > .../qemuxml2argv-pseries-usb-kbd.args | 2 +- > .../qemuxml2argv-pseries-usb-multi.args | 2 +- > .../qemuxml2argv-pseries-vio-user-assigned.args | 4 +- > .../qemuxml2argvdata/qemuxml2argv-pseries-vio.args | 4 +- > tests/qemuxml2argvtest.c | 16 +++++ > .../qemuxml2xmlout-panic-pseries.xml | 4 +- > .../qemuxml2xmlout-pseries-console-native.xml | 1 + > ...l => qemuxml2xmlout-pseries-console-virtio.xml} | 18 ++---- > .../qemuxml2xmlout-pseries-cpu-compat-power9.xml | 4 +- > .../qemuxml2xmlout-pseries-cpu-compat.xml | 4 +- > .../qemuxml2xmlout-pseries-cpu-exact.xml | 4 +- > .../qemuxml2xmlout-pseries-panic-missing.xml | 4 +- > .../qemuxml2xmlout-pseries-panic-no-address.xml | 4 +- > ...emuxml2xmlout-pseries-serial+console-native.xml | 1 + > .../qemuxml2xmlout-pseries-serial-compat.xml | 1 + > ...ml => qemuxml2xmlout-pseries-serial-native.xml} | 10 ++- > tests/qemuxml2xmltest.c | 15 +++++ > 43 files changed, 301 insertions(+), 109 deletions(-) > create mode 120000 tests/qemuxml2argvdata/qemuxml2argv-pseries-console-native.args > create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pseries-console-native.xml > copy tests/qemuxml2argvdata/{qemuxml2argv-pseries-basic.args => qemuxml2argv-pseries-console-virtio.args} (59%) > create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pseries-console-virtio.xml > create mode 120000 tests/qemuxml2argvdata/qemuxml2argv-pseries-serial+console-native.args > create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pseries-serial+console-native.xml > create mode 120000 tests/qemuxml2argvdata/qemuxml2argv-pseries-serial-compat.args > create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pseries-serial-compat.xml > create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pseries-serial-invalid-machine.xml > copy tests/qemuxml2argvdata/{qemuxml2argv-pseries-basic.args => qemuxml2argv-pseries-serial-native.args} (70%) > create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pseries-serial-native.xml > create mode 120000 tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-console-native.xml > copy tests/qemuxml2xmloutdata/{qemuxml2xmlout-panic-pseries.xml => qemuxml2xmlout-pseries-console-virtio.xml} (71%) > create mode 120000 tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-serial+console-native.xml > create mode 120000 tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-serial-compat.xml > copy tests/qemuxml2xmloutdata/{qemuxml2xmlout-panic-pseries.xml => qemuxml2xmlout-pseries-serial-native.xml} (79%) > > diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in > index 2edc61a01..92622d031 100644 > --- a/docs/formatdomain.html.in > +++ b/docs/formatdomain.html.in > @@ -6530,10 +6530,13 @@ qemu-kvm -net nic,model=? /dev/null > specifies the port number. Ports are numbered starting from 0. There are > usually 0, 1 or 2 serial ports. There is also an optional > <code>type</code> attribute <span class="since">since 1.0.2</span> > - which has three choices for its value, one is <code>isa</code>, > - then <code>usb</code> and last one is <code>pci</code>. > - If <code>type</code> is missing, <code>isa</code> will be used by > - default. For <code>usb</code> an optional sub-element > + which can be used to pick between <code>isa</code>, <code>usb</code>, > + <code>pci</code> and, <span class="since">since 3.10.0</span>, > + <code>spapr-vio</code>. > + Some values are not compatible with all architecture and machine types; > + if the value is missing altogether, libvirt will try to pick an > + appropriate default. > + For <code>usb</code> an optional sub-element > <code><address/></code> with <code>type='usb'</code> can tie the > device to a particular controller, <a href="#elementsAddress">documented above</a>. > Similarly, <code>pci</code> can be used to attach the device to > diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng > index 93beabc5e..b7a13660d 100644 > --- a/docs/schemas/domaincommon.rng > +++ b/docs/schemas/domaincommon.rng > @@ -3585,6 +3585,7 @@ > <value>isa</value> > <value>usb</value> > <value>pci</value> > + <value>spapr-vio</value> Since we cannot reduce isa-serial into isa, I guest having spapr-vio in addition to other ${BUS}-serial would be weird, so how about spapr-vio-serial or even only spapr-serial? > <!-- Legacy values, for backwards compatibility --> > <value>isa-serial</value> > <value>usb-serial</value> > @@ -3600,6 +3601,7 @@ > <value>isa-serial</value> > <value>usb-serial</value> > <value>pci-serial</value> > + <value>spapr-vty</value> > </choice> > </attribute> > </element> > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c > index 0d8c88db9..d90acd31e 100644 > --- a/src/conf/domain_conf.c > +++ b/src/conf/domain_conf.c > @@ -452,6 +452,7 @@ VIR_ENUM_IMPL(virDomainChrSerialTarget, > "isa", > "usb", > "pci", > + "spapr-vio", > ); > > VIR_ENUM_IMPL(virDomainChrChannelTarget, > @@ -479,6 +480,7 @@ VIR_ENUM_IMPL(virDomainChrSerialTargetModel, > "isa-serial", > "usb-serial", > "pci-serial", > + "spapr-vty", > ); > > VIR_ENUM_IMPL(virDomainChrDevice, VIR_DOMAIN_CHR_DEVICE_TYPE_LAST, > @@ -4051,6 +4053,7 @@ virDomainDefAddConsoleCompat(virDomainDefPtr def) > > switch ((virDomainChrSerialTargetType) def->serials[0]->targetType) { > case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_ISA: > + case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_SPAPR_VIO: > case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_NONE: { > > /* Create a stub console to match the serial port. > @@ -24091,6 +24094,7 @@ virDomainChrTargetDefFormat(virBufferPtr buf, > case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_PCI: > virBufferAddLit(buf, "-serial"); > break; > + case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_SPAPR_VIO: > case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_NONE: > case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_LAST: > /* No conversion necessary */ > diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h > index 3e74c635b..dc376de49 100644 > --- a/src/conf/domain_conf.h > +++ b/src/conf/domain_conf.h > @@ -1085,6 +1085,7 @@ typedef enum { > VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_ISA, > VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_USB, > VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_PCI, > + VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_SPAPR_VIO, > > VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_LAST > } virDomainChrSerialTargetType; > @@ -1117,6 +1118,7 @@ typedef enum { > VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_ISA_SERIAL, > VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_USB_SERIAL, > VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_PCI_SERIAL, > + VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_SPAPR_VTY, > > VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_LAST > } virDomainChrSerialTargetModel; > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c > index d49183931..d1dd60d8f 100644 > --- a/src/qemu/qemu_command.c > +++ b/src/qemu/qemu_command.c > @@ -10269,55 +10269,49 @@ qemuBuildSerialChrDeviceStr(char **deviceStr, > { > virBuffer cmd = VIR_BUFFER_INITIALIZER; > > - if (qemuDomainIsPSeries(def)) { > - if (serial->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL && > - serial->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO) { > - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_SPAPR_VTY)) { > - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > - _("spapr-vty not supported in this QEMU binary")); > - goto error; > - } > - > - virBufferAsprintf(&cmd, "spapr-vty,chardev=char%s", > - serial->info.alias); > + switch ((virDomainChrSerialTargetModel) serial->targetModel) { > + case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_USB_SERIAL: > + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_USB_SERIAL)) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > + _("usb-serial is not supported in this QEMU binary")); > + goto error; > } > - } else { > - switch ((virDomainChrSerialTargetModel) serial->targetModel) { > - case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_USB_SERIAL: > - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_USB_SERIAL)) { > - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > - _("usb-serial is not supported in this QEMU binary")); > - goto error; > - } > - break; > + break; > > - case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_ISA_SERIAL: > - break; > + case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_ISA_SERIAL: > + break; > > - case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_PCI_SERIAL: > - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PCI_SERIAL)) { > - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > - _("pci-serial is not supported with this QEMU binary")); > - goto error; > - } > - break; > + case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_PCI_SERIAL: > + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PCI_SERIAL)) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > + _("pci-serial is not supported with this QEMU binary")); > + goto error; > + } > + break; > > - case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_NONE: > - case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_LAST: > - /* Except from _LAST, which is just a guard value and will never > - * be used, all of the above are platform devices, which means > - * qemuBuildSerialCommandLine() will have taken the appropriate > - * branch and we will not have ended up here. */ > - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > - _("Invalid target type for serial device")); > + case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_SPAPR_VTY: > + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_SPAPR_VTY)) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > + _("spapr-vty not supported in this QEMU binary")); > goto error; > } > + break; > > - virBufferAsprintf(&cmd, "%s,chardev=char%s,id=%s", > - virDomainChrSerialTargetModelTypeToString(serial->targetModel), > - serial->info.alias, serial->info.alias); > + case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_NONE: > + case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_LAST: > + /* Except from _LAST, which is just a guard value and will never > + * be used, all of the above are platform devices, which means > + * qemuBuildSerialCommandLine() will have taken the appropriate > + * branch and we will not have ended up here. */ > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("Invalid target type for serial device")); > + goto error; > } > > + virBufferAsprintf(&cmd, "%s,chardev=char%s,id=%s", > + virDomainChrSerialTargetModelTypeToString(serial->targetModel), > + serial->info.alias, serial->info.alias); > + > if (qemuBuildDeviceAddressStr(&cmd, def, &serial->info, qemuCaps) < 0) > goto error; > > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c > index 06ce382fa..785a93207 100644 > --- a/src/qemu/qemu_domain.c > +++ b/src/qemu/qemu_domain.c > @@ -3491,8 +3491,7 @@ qemuDomainChrSourceDefValidate(const virDomainChrSourceDef *def) > > > static int > -qemuDomainChrTargetDefValidate(const virDomainDef *def, > - const virDomainChrDef *chr) > +qemuDomainChrTargetDefValidate(const virDomainChrDef *chr) > { > switch ((virDomainChrDeviceType) chr->deviceType) { > case VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL: > @@ -3500,11 +3499,6 @@ qemuDomainChrTargetDefValidate(const virDomainDef *def, > /* Validate target type */ > switch ((virDomainChrSerialTargetType) chr->targetType) { > case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_ISA: > - /* Hack required until we have a proper type for pSeries > - * serial consoles */ > - if (qemuDomainIsPSeries(def)) > - return 0; > - > if (chr->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE && > chr->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_ISA) { > virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > @@ -3534,6 +3528,16 @@ qemuDomainChrTargetDefValidate(const virDomainDef *def, > } > break; > > + case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_SPAPR_VIO: > + if (chr->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE && > + chr->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > + _("Target type 'spapr-vio' requires address " > + "of type 'spapr-vio'")); > + return -1; > + } > + break; > + > case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_NONE: > case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_LAST: > break; > @@ -3571,6 +3575,16 @@ qemuDomainChrTargetDefValidate(const virDomainDef *def, > } > break; > > + case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_SPAPR_VTY: > + if (chr->targetType != VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_SPAPR_VIO) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > + _("Target model '%s' requires " > + "target type 'spapr-vio'"), > + virDomainChrSerialTargetModelTypeToString(chr->targetModel)); > + return -1; > + } > + break; > + > case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_NONE: > case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_LAST: > break; > @@ -3596,7 +3610,7 @@ qemuDomainChrDefValidate(const virDomainChrDef *dev, > if (qemuDomainChrSourceDefValidate(dev->source) < 0) > return -1; > > - if (qemuDomainChrTargetDefValidate(def, dev) < 0) > + if (qemuDomainChrTargetDefValidate(dev) < 0) > return -1; > > if (dev->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_PARALLEL && > @@ -3606,6 +3620,17 @@ qemuDomainChrDefValidate(const virDomainChrDef *dev, > return -1; > } > > + if (dev->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL && > + (dev->targetType == VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_SPAPR_VIO || > + dev->targetModel == VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_SPAPR_VTY) && > + !qemuDomainIsPSeries(def)) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > + _("Serial devices with target type 'spapr-vio' and " > + "target model 'spapr-vty' are only supported on " > + "pSeries guests")); I know that it's unlikely to happen but what if there will be new model for spapr-vio? I would suggest to split this condition: if (serial && sparp_vio && !pseries) error("spapr-vio is supported only on pSeries machine") if (serial && sparp_vty && !pseries) error("spapr-vio is supported only on pSeries machine") Otherwise the patch looks good. Pavel
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
