[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v2 15/21] conf: Add target type and model for spapr-vty



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 redhat com>
> ---
>  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>&lt;address/&gt;</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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]