[libvirt] [PATCH 1/6] s390: Cpu driver support for update and compare

[Jiri Denemark]

On Mon, Nov 21, 2016 at 14:11:51 -0500, Jason J. Herne wrote:
> Implement compare for s390. Required to test the guest against the host for
> guest cpu model runnability checking. We always return IDENTICAL to bypass
> Libvirt's checking. s390 will rely on Qemu to perform the runnability checking.
> 
> Implement update for s390. required to support use of cpu "host-model" mode.
> 
> Signed-off-by: Jason J. Herne <jjherne at linux.vnet.ibm.com>
> ---
>  po/POTFILES.in     |  1 +
>  src/cpu/cpu_s390.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++--
>  2 files changed, 53 insertions(+), 2 deletions(-)
> 
> diff --git a/po/POTFILES.in b/po/POTFILES.in
> index 25867ae..ea70e33 100644
> --- a/po/POTFILES.in
> +++ b/po/POTFILES.in
> @@ -46,6 +46,7 @@ src/cpu/cpu.c
>  src/cpu/cpu_arm.c
>  src/cpu/cpu_map.c
>  src/cpu/cpu_ppc64.c
> +src/cpu/cpu_s390.c
>  src/cpu/cpu_x86.c
>  src/datatypes.c
>  src/driver.c
> diff --git a/src/cpu/cpu_s390.c b/src/cpu/cpu_s390.c
> index 04a6bea..0073565 100644
> --- a/src/cpu/cpu_s390.c
> +++ b/src/cpu/cpu_s390.c
> @@ -71,15 +71,65 @@ s390DataFree(virCPUDataPtr data)
>      VIR_FREE(data);
>  }
>  
> +static virCPUCompareResult
> +virCPUs390Compare(virCPUDefPtr host ATTRIBUTE_UNUSED,
> +                 virCPUDefPtr cpu ATTRIBUTE_UNUSED,
> +                 bool failMessages ATTRIBUTE_UNUSED)

The indentation is a bit off here.

> +{
> +    return VIR_CPU_COMPARE_IDENTICAL;
> +}

I know there is no code to detect host CPU, but this essentially means
that any guest CPU configuration can be started on any s390 host (I'm
talking about KVM, of course). Is this correct or would it make sense to
somehow compare the guest CPU with the host model returned by QEMU?

Which reminds me I don't think I've ever seen any s390 CPU XML. Could
you just add some test cases focused on s390 CPUs to qemuxml2argvtest?

And since this series is largely about QEMU capabilities, it would be
nice if you could add a few .replies and corresponding .xml files to
tests/qemucapabilitiesdata and also use them in domaincapstest?
Generating .replies is easy, just run

    tests/qemucapsprobe /path/to/qemu >caps_2.8.0.s390.replies

I think replies from 2.7.0 and 2.8.0 could be enough (unless
query-cpu-model-expansion was introduced earlier than in 2.8.0).

Note, you may need to regenerate the replies files when
query-cpu-model-expansion starts to be used.

> +
> +static int
> +virCPUs390Update(virCPUDefPtr guest,
> +                 const virCPUDef *host)
> +{
> +     virCPUDefPtr updated = NULL;
> +     int ret = -1;
> +
> +     if (guest->match == VIR_CPU_MATCH_MINIMUM) {
> +         virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                        _("Match mode %s not supported"),

All other error messages in this patch start with lower case so I guess
this could follow the same pattern.

> +                        virCPUMatchTypeToString(guest->match));
> +         goto cleanup;
> +     }
> +

Are you going to support optional features? If so, the function should
update them to require/disable, otherwise I think it should report an
error if the guest CPU definition contains them.

> +     if (guest->mode != VIR_CPU_MODE_HOST_MODEL) {
> +         ret = 0;
> +         goto cleanup;
> +     }
> +
> +     if (!host) {
> +         virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                        _("unknown host CPU model"));
> +         goto cleanup;
> +     }
> +
> +     if (!(updated = virCPUDefCopyWithoutModel(guest)))
> +         goto cleanup;
> +
> +     updated->mode = VIR_CPU_MODE_CUSTOM;
> +     if (virCPUDefCopyModel(updated, host, true) < 0)
> +         goto cleanup;
> +
> +     virCPUDefStealModel(guest, updated, false);
> +     guest->mode = VIR_CPU_MODE_CUSTOM;
> +     guest->match = VIR_CPU_MATCH_EXACT;

Are you going to support additional features for host-model CPUs? In
other words, does the following XML make sense in s390?

    <cpu mode='host-model'>
      <feature name='bla' policy='require'/>
      <feature name='ble' policy='disable'/>
    </cpu>

If so, the code is insufficient. Otherwise, it's unnecessarily
complicated. There's no need to create "updated" CPU model when you
don't need to look at the features specified in the original guest CPU
definition.

> +     ret = 0;
> +
> + cleanup:
> +     virCPUDefFree(updated);
> +     return ret;
> +}
> +
>  struct cpuArchDriver cpuDriverS390 = {
>      .name = "s390",
>      .arch = archs,
>      .narch = ARRAY_CARDINALITY(archs),
> -    .compare    = NULL,
> +    .compare    = virCPUs390Compare,
>      .decode     = s390Decode,
>      .encode     = NULL,
>      .free       = s390DataFree,
>      .nodeData   = s390NodeData,
>      .baseline   = NULL,
> -    .update     = NULL,
> +    .update     = virCPUs390Update,
>  };

Jirka