Re: [PULL 04/11] Add support for RAPL MSRs in KVM/Qemu

[Peter Maydell]

On Tue, 23 Jul 2024 at 15:17, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> From: Anthony Harivel <aharivel@redhat.com>
>
> Starting with the "Sandy Bridge" generation, Intel CPUs provide a RAPL
> interface (Running Average Power Limit) for advertising the accumulated
> energy consumption of various power domains (e.g. CPU packages, DRAM,
> etc.).
>
> The consumption is reported via MSRs (model specific registers) like
> MSR_PKG_ENERGY_STATUS for the CPU package power domain. These MSRs are
> 64 bits registers that represent the accumulated energy consumption in
> micro Joules. They are updated by microcode every ~1ms.

Hi; Coverity points out a couple of issues with this commit
(details below).


> +static void *kvm_msr_energy_thread(void *data)
> +{
> +    KVMState *s = data;
> +    struct KVMMsrEnergy *vmsr = &s->msr_energy;
> +
> +    g_autofree vmsr_package_energy_stat *pkg_stat = NULL;
> +    g_autofree vmsr_thread_stat *thd_stat = NULL;
> +    g_autofree CPUState *cpu = NULL;
> +    g_autofree unsigned int *vpkgs_energy_stat = NULL;
> +    unsigned int num_threads = 0;
> +
> +    X86CPUTopoIDs topo_ids;
> +
> +    rcu_register_thread();
> +
> +    /* Allocate memory for each package energy status */
> +    pkg_stat = g_new0(vmsr_package_energy_stat, vmsr->host_topo.maxpkgs);
> +
> +    /* Allocate memory for thread stats */
> +    thd_stat = g_new0(vmsr_thread_stat, 1);
> +
> +    /* Allocate memory for holding virtual package energy counter */
> +    vpkgs_energy_stat = g_new0(unsigned int, vmsr->guest_vsockets);
> +
> +    /* Populate the max tick of each packages */
> +    for (int i = 0; i < vmsr->host_topo.maxpkgs; i++) {
> +        /*
> +         * Max numbers of ticks per package
> +         * Time in second * Number of ticks/second * Number of cores/package
> +         * ex: 100 ticks/second/CPU, 12 CPUs per Package gives 1200 ticks max
> +         */
> +        vmsr->host_topo.maxticks[i] = (MSR_ENERGY_THREAD_SLEEP_US / 1000000)
> +                        * sysconf(_SC_CLK_TCK)
> +                        * vmsr->host_topo.pkg_cpu_count[i];
> +    }
> +
> +    while (true) {
> +        /* Get all qemu threads id */
> +        g_autofree pid_t *thread_ids =
> +            thread_ids = vmsr_get_thread_ids(vmsr->pid, &num_threads);

Here you are assigning the same value to the thread_ids variable
twice. What was the intention here ? (CID 1558553)

> +
> +        if (thread_ids == NULL) {
> +            goto clean;
> +        }



> +/* Read the scheduled time for a given thread of a give pid */
> +void vmsr_read_thread_stat(pid_t pid,
> +                      unsigned int thread_id,
> +                      unsigned long long *utime,
> +                      unsigned long long *stime,
> +                      unsigned int *cpu_id)
> +{
> +    g_autofree char *path = NULL;
> +    g_autofree char *path_name = NULL;
> +
> +    path_name = g_strdup_printf("/proc/%u/task/%d/stat", pid, thread_id);
> +
> +    path = g_build_filename(path_name, NULL);
> +
> +    FILE *file = fopen(path, "r");
> +    if (file == NULL) {
> +        pid = -1;
> +        return;
> +    }
> +
> +    if (fscanf(file, "%*d (%*[^)]) %*c %*d %*d %*d %*d %*d %*u %*u %*u %*u 
> %*u"
> +        " %llu %llu %*d %*d %*d %*d %*d %*d %*u %*u %*d %*u %*u"
> +        " %*u %*u %*u %*u %*u %*u %*u %*u %*u %*d %*u %*u %u",
> +           utime, stime, cpu_id) != 3)
> +    {
> +        pid = -1;
> +        return;

In this error-exit path we leak 'file', because we opened it but
didn't close it. (CID 1558557)

Also, both here and in the previous error-exit check we set pid = -1,
which is pointless because it's a function-local variable and nothing
is going to see the new value.

> +    }
> +
> +    fclose(file);
> +    return;
> +}

thanks
-- PMM