Re: [PULL v2 29/86] hw/cxl/host: Add support for CXL Fixed Memory Windows.

[Peter Maydell]

On Mon, 16 May 2022 at 21:52, Michael S. Tsirkin <mst@redhat.com> wrote:
>
> From: Jonathan Cameron <jonathan.cameron@huawei.com>
>
> The concept of these is introduced in [1] in terms of the
> description the CEDT ACPI table. The principal is more general.
> Unlike once traffic hits the CXL root bridges, the host system
> memory address routing is implementation defined and effectively
> static once observable by standard / generic system software.
> Each CXL Fixed Memory Windows (CFMW) is a region of PA space
> which has fixed system dependent routing configured so that
> accesses can be routed to the CXL devices below a set of target
> root bridges. The accesses may be interleaved across multiple
> root bridges.

Hi; Coverity points out a memory leak in this function
(CID 1488872):

> +void cxl_fixed_memory_window_config(MachineState *ms,
> +                                    CXLFixedMemoryWindowOptions *object,
> +                                    Error **errp)
> +{
> +    CXLFixedWindow *fw = g_malloc0(sizeof(*fw));

Here we allocate memory...

> +    strList *target;
> +    int i;
> +
> +    for (target = object->targets; target; target = target->next) {
> +        fw->num_targets++;
> +    }
> +
> +    fw->enc_int_ways = cxl_interleave_ways_enc(fw->num_targets, errp);
> +    if (*errp) {
> +        return;

...but in the error returns here..

> +    }
> +
> +    fw->targets = g_malloc0_n(fw->num_targets, sizeof(*fw->targets));
> +    for (i = 0, target = object->targets; target; i++, target = 
> target->next) {
> +        /* This link cannot be resolved yet, so stash the name for now */
> +        fw->targets[i] = g_strdup(target->value);
> +    }
> +
> +    if (object->size % (256 * MiB)) {
> +        error_setg(errp,
> +                   "Size of a CXL fixed memory window must my a multiple of 
> 256MiB");
> +        return;

...here..

> +    }
> +    fw->size = object->size;
> +
> +    if (object->has_interleave_granularity) {
> +        fw->enc_int_gran =
> +            cxl_interleave_granularity_enc(object->interleave_granularity,
> +                                           errp);
> +        if (*errp) {
> +            return;

...and here we fail to free the memory we allocated.

> +        }
> +    } else {
> +        /* Default to 256 byte interleave */
> +        fw->enc_int_gran = 0;
> +    }
> +
> +    ms->cxl_devices_state->fixed_windows =
> +        g_list_append(ms->cxl_devices_state->fixed_windows, fw);
> +
> +    return;
> +}

Would you mind sending a patch to fix this bug ?
The neatest approach probably uses g_autofree and g_steal_pointer().

thanks
-- PMM