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

[Alexander Graf]

On 08.07.14 13:04, Nikunj A Dadhania wrote:
> Alexander Graf <address@hidden> writes:
>
> > 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 <address@hidden>
> > >
> > > ---
> > > 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?
> Tried doing it more in line of read_cpuinfo in target-ppc/kvm.c
> I could do it either ways.

Yeah, feel free to convert that one too if you like ;).

> > >    
> > >        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.
> Yes, there are tools that look at these. Is there a way to update these
> on migration?

As Ben already mentioned ;).

> > > +
> > > +    snprintf(char_buf, 37, UUID_FMT, qemu_uuid[0], qemu_uuid[1],
> > g_strdup_printf()
> Ok.
>
> > > +             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>?
> There were userspace tools that looking at lparcfg, and were encoded
> similarly.

I don't think we own the IBM namespace, so I find this slightly bogus. 
Also why would a host machine have to be made by IBM?

> > > +        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 ;).
> Let me search.
>
> > > +}
> > > +
> > > +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?
> No, it will be IBM,<model>

Hrm, I just tried to compare this with a pHyp system and can't seem to 
find any /proc/device-tree/host* files. What am I missing?


Alex