Re: [Qemu-devel] [PATCH v4] spapr: add uuid/host details to device tree

[Alexander Graf <agraf@suse.de>]

On 08.07.14 07:00, Nikunj A Dadhania wrote:
> Useful for identifying the guest/host uniquely within the
> guest. Adding following properties to the guest root node.
>
> vm,uuid - uuid of the guest
> host-model - Host model number
> host-serial - Host machine serial number
> hypervisor type - Tells its "kvm"
>
> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
>
> ---
> v4: make uuid as human readable
> v3: rebase to ppcnext
> v2: indentation fixes
> ---
>   hw/ppc/spapr.c       | 25 +++++++++++++++++++++++++
>   target-ppc/kvm.c     | 44 +++++++++++++++++++++++++++++++++++++++++++-
>   target-ppc/kvm_ppc.h | 12 ++++++++++++
>   3 files changed, 80 insertions(+), 1 deletion(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 077ad2d..485ea66 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -318,6 +318,7 @@ static void *spapr_create_fdt_skel(hwaddr initrd_base,
>       QemuOpts *opts = qemu_opts_find(qemu_find_opts("smp-opts"), NULL);
>       unsigned sockets = opts ? qemu_opt_get_number(opts, "sockets", 0) : 0;
>       uint32_t cpus_per_socket = sockets ? (smp_cpus / sockets) : 1;
> +    char char_buf[512];

Can't you just return callee allocated, caller free'd memory?

>   
>       add_str(hypertas, "hcall-pft");
>       add_str(hypertas, "hcall-term");
> @@ -347,6 +348,30 @@ static void *spapr_create_fdt_skel(hwaddr initrd_base,
>       _FDT((fdt_property_string(fdt, "model", "IBM pSeries (emulated by qemu)")));
>       _FDT((fdt_property_string(fdt, "compatible", "qemu,pseries")));
>   
> +    if (kvm_enabled()) {
> +        _FDT((fdt_property_string(fdt, "hypervisor", "kvm")));
> +    }
> +
> +    /*
> +     * Add info to guest to indentify which host is it being run on
> +     * and what is the uuid of the guest
> +     */
> +    memset(char_buf, 0, sizeof(char_buf));
> +    if (!kvmppc_get_host_model(char_buf, sizeof(char_buf))) {
> +        _FDT((fdt_property_string(fdt, "host-model", char_buf)));
> +        memset(char_buf, 0, sizeof(char_buf));
> +    }
> +    if (!kvmppc_get_host_serial(char_buf, sizeof(char_buf))) {
> +        _FDT((fdt_property_string(fdt, "host-serial", char_buf)));
> +    }

Please be aware that all of the above is bogus when you start thinking 
about live migration.

> +
> +    snprintf(char_buf, 37, UUID_FMT, qemu_uuid[0], qemu_uuid[1],

g_strdup_printf()

> +             qemu_uuid[2], qemu_uuid[3], qemu_uuid[4], qemu_uuid[5],
> +             qemu_uuid[6], qemu_uuid[7], qemu_uuid[8], qemu_uuid[9],
> +             qemu_uuid[10], qemu_uuid[11], qemu_uuid[12], qemu_uuid[13],
> +             qemu_uuid[14], qemu_uuid[15]);
> +    _FDT((fdt_property_string(fdt, "vm,uuid", char_buf)));
> +
>       _FDT((fdt_property_cell(fdt, "#address-cells", 0x2)));
>       _FDT((fdt_property_cell(fdt, "#size-cells", 0x2)));
>   
> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> index 2d87108..25091f8 100644
> --- a/target-ppc/kvm.c
> +++ b/target-ppc/kvm.c
> @@ -1369,7 +1369,7 @@ static int read_cpuinfo(const char *field, char *value, int len)
>       }
>   
>       do {
> -        if(!fgets(line, sizeof(line), f)) {
> +        if (!fgets(line, sizeof(line), f)) {
>               break;
>           }
>           if (!strncmp(line, field, field_len)) {
> @@ -1404,6 +1404,48 @@ uint32_t kvmppc_get_tbfreq(void)
>       return retval;
>   }
>   
> +int32_t kvmppc_get_host_serial(char *value, int len)
> +{
> +    FILE *f;
> +    int ret = -1;
> +    char line[512];
> +
> +    memset(line, 0, sizeof(line));
> +    f = fopen("/proc/device-tree/system-id", "r");
> +    if (!f) {
> +        return ret;
> +    }
> +
> +    if (fgets(line, sizeof(line), f)) {
> +        snprintf(value, len, "IBM,%s", line);

Why IBM,<system-id>?

> +        ret = 0;
> +    }
> +    fclose(f);
> +
> +    return ret;

I think it makes sense to extract the "read a full file into a buffer" 
logic into a separate function. For bonus points, find a glib function 
that already does it and use that ;).

> +}
> +
> +int32_t kvmppc_get_host_model(char *value, int len)
> +{
> +    FILE *f;
> +    int ret = -1;
> +    char line[512];
> +
> +    memset(line, 0, sizeof(line));
> +    f = fopen("/proc/device-tree/model", "r");
> +    if (!f) {
> +        return ret;
> +    }
> +
> +    if (fgets(line, sizeof(line), f)) {
> +        snprintf(value, len, "IBM,%s", line);

Same here - wouldn't this be IBM,IBM,foo?


Alex

> +        ret = 0;
> +    }
> +    fclose(f);
> +
> +    return ret;
> +}
> +
>   /* Try to find a device tree node for a CPU with clock-frequency property */
>   static int kvmppc_find_cpu_dt(char *buf, int buf_len)
>   {
> diff --git a/target-ppc/kvm_ppc.h b/target-ppc/kvm_ppc.h
> index 1118122..6fa3314 100644
> --- a/target-ppc/kvm_ppc.h
> +++ b/target-ppc/kvm_ppc.h
> @@ -19,6 +19,8 @@ uint32_t kvmppc_get_tbfreq(void);
>   uint64_t kvmppc_get_clockfreq(void);
>   uint32_t kvmppc_get_vmx(void);
>   uint32_t kvmppc_get_dfp(void);
> +int32_t kvmppc_get_host_model(char *buf, int buf_len);
> +int32_t kvmppc_get_host_serial(char *buf, int buf_len);
>   int kvmppc_get_hasidle(CPUPPCState *env);
>   int kvmppc_get_hypercall(CPUPPCState *env, uint8_t *buf, int buf_len);
>   int kvmppc_set_interrupt(PowerPCCPU *cpu, int irq, int level);
> @@ -60,6 +62,16 @@ static inline uint32_t kvmppc_get_tbfreq(void)
>       return 0;
>   }
>   
> +static inline int32_t kvmppc_get_host_model(char *buf, int buf_len)
> +{
> +    return 0;
> +}
> +
> +static inline int32_t kvmppc_get_host_serial(char *buf, int buf_len)
> +{
> +    return 0;
> +}
> +
>   static inline uint64_t kvmppc_get_clockfreq(void)
>   {
>       return 0;