Re: [libvirt] [PATCH 1/2] virsh: Implement virNodeGetSEVInfo in virsh

[Erik Skultety <eskultet redhat com>]

On Tue, Aug 21, 2018 at 11:20:27AM +0800, Han Han wrote:
> Add sub-command nodesevinfo to get node infomation of AMD SEV feature.
>
> Signed-off-by: Han Han <hhan redhat com>
> ---
>  tools/virsh-host.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++
>  tools/virsh.pod    |  5 ++++
>  2 files changed, 71 insertions(+)
>
> diff --git a/tools/virsh-host.c b/tools/virsh-host.c
> index 16f504bafe..0bcd71a2b8 100644
> --- a/tools/virsh-host.c
> +++ b/tools/virsh-host.c
> @@ -952,6 +952,67 @@ cmdNodeMemStats(vshControl *ctl, const vshCmd *cmd)
>      return ret;
>  }
>
> +/*
> + * "nodesevinfo" command
> + */
> +static const vshCmdInfo info_nodesevinfo[] = {
> +    {.name = "help",
> +     .data = N_("AMD SEV feature information.")

s/feature/platform

Note that this is part of the "node" subsystem, which means it is always going
to be tied to the firmware on a given node, thus "platform data".

> +    },
> +    {.name = "desc",
> +     .data = N_("Returns information of SEV feature about the node.")

Returns SEV platform-specific data.

> +    },
> +    {.name = NULL}
> +};
> +
> +static bool
> +cmdNodesevinfo(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED)

s/sev/SEV/

> +{
> +    virTypedParameterPtr params = NULL;
> +    int nparams = 0;
> +    unsigned int flags = 0;
> +    bool ret = false;
> +    size_t i;
> +    virshControlPtr priv = ctl->privData;
> +
> +    if (nparams == 0) {
> +        /* Get the number of SEV info parameters */
> +        if (virNodeGetSEVInfo(priv->conn, NULL, &nparams, flags) != 0) {

Have you actually tried the patches? Because ^this causes virsh to segfault
because this is not a legacy API where you need to query the number of params
first and then pre-allocate the pointer to store the data. We don't need that
anymore, remote driver is already able to allocate the memory for the caller.

> +            vshError(ctl, "%s",
> +                     _("Unable to get number of SEV info parameters"));
> +            goto cleanup;
> +        }
> +    }
> +
> +    if (nparams == 0) {
> +        ret = true;
> +        goto cleanup;
> +    }
> +
> +    /* Now get all the SEV info parameters */
> +    params = vshCalloc(ctl, nparams, sizeof(params));

no need for ^this...

> +    if (virNodeGetSEVInfo(priv->conn, &params, &nparams, flags) != 0) {
> +        vshError(ctl, "%s", _("Unable to get SEV info parameters"));
> +        goto cleanup;
> +    }
> +
> +    /* XXX: Need to sort the returned params once new parameter
> +     * fields not of shared memory are added.

I'm not sure I see a problem ^here, can you elaborate please?

> +     */
> +    vshPrint(ctl, _("SEV info:\n"));
> +    for (i = 0; i < nparams; i++) {
> +        char *str = vshGetTypedParamValue(ctl, &params[i]);
> +        vshPrint(ctl, "\t%-15s %s\n", params[i].field, str);
> +        VIR_FREE(str);
> +    }
> +
> +    ret = true;
> +
> + cleanup:
> +    virTypedParamsFree(params, nparams);
> +    return ret;
> +}
> +
>  /*
>   * "nodesuspend" command
>   */
> @@ -1900,6 +1961,11 @@ const vshCmdDef hostAndHypervisorCmds[] = {
>       .info = info_nodememstats,
>       .flags = 0
>      },
> +    {.name = "nodesevinfo",
> +     .handler = cmdNodesevinfo,
> +     .info = info_nodesevinfo,
> +     .flags = 0
> +    },
>      {.name = "nodesuspend",
>       .handler = cmdNodeSuspend,
>       .opts = opts_node_suspend,
> diff --git a/tools/virsh.pod b/tools/virsh.pod
> index 4e118851f8..ea513c0acc 100644
> --- a/tools/virsh.pod
> +++ b/tools/virsh.pod
> @@ -317,6 +317,11 @@ of cpu statistics during 1 second.
>  Returns memory stats of the node.
>  If I<cell> is specified, this will print the specified cell statistics only.
>
> +=item B<nodesevinfo>
> +
> +Display AMD's SEV feature of this host, including PDH, cert-chain, cbitpos
> +and reduced-phys-bits.

Returns AMD's SEV platform-specific data. Availability of the fields depends on
the version of the SEV firmware.

Explanation of fields:
    pdh - Platform Diffie-Hellman key
    cert-chain - certificate chain used to verify authenticity of the platform
    cbitpos - C-bit position, i.e. which physical address bit marks protection
              on memory pages
    reduced-phys-bits - how many physical address bits we lost due to memory
                        encryption

Erik