Re: [PULL 06/10] Add Hyper-V Dynamic Memory Protocol driver (hv-balloon) hot-add support

[Peter Maydell <peter.maydell@linaro.org>]

On Mon, 6 Nov 2023 at 14:23, Maciej S. Szmigiero
<mail@maciej.szmigiero.name> wrote:
>
> From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>
>
> One of advantages of using this protocol over ACPI-based PC DIMM hotplug is
> that it allows hot-adding memory in much smaller granularity because the
> ACPI DIMM slot limit does not apply.
>
> In order to enable this functionality a new memory backend needs to be
> created and provided to the driver via the "memdev" parameter.
>
> This can be achieved by, for example, adding
> "-object memory-backend-ram,id=mem1,size=32G" to the QEMU command line and
> then instantiating the driver with "memdev=mem1" parameter.
>
> The device will try to use multiple memslots to cover the memory backend in
> order to reduce the size of metadata for the not-yet-hot-added part of the
> memory backend.
>
> Co-developed-by: David Hildenbrand <david@redhat.com>
> Acked-by: David Hildenbrand <david@redhat.com>
> Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>

Hi; I was looking at this code because Coverity reported an
issue in it. I think that's because Coverity has got confused
about the way you're doing memory allocation here. But
in looking at the code I see that you're using alloca() in
this function.

Please could you rewrite this not to do that -- we don't use
alloca() or variable-length-arrays in QEMU except in a few
cases which we're trying to get rid of, so we'd like not to
add new uses to the code base.

> +static void hv_balloon_hot_add_posting(HvBalloon *balloon, StateDesc *stdesc)
> +{
> +    PageRange *hot_add_range = &balloon->hot_add_range;
> +    uint64_t *current_count = &balloon->ha_current_count;
> +    VMBusChannel *chan = hv_balloon_get_channel(balloon);
> +    struct dm_hot_add *ha;
> +    size_t ha_size = sizeof(*ha) + sizeof(ha->range);
> +    union dm_mem_page_range *ha_region;
> +    uint64_t align, chunk_max_size;
> +    ssize_t ret;
> +
> +    assert(balloon->state == S_HOT_ADD_POSTING);
> +    assert(hot_add_range->count > 0);
> +
> +    align = (1 << balloon->caps.cap_bits.hot_add_alignment) *
> +        (MiB / HV_BALLOON_PAGE_SIZE);
> +    if (align >= HV_BALLOON_HA_CHUNK_PAGES) {
> +        /*
> +         * If the required alignment is higher than the chunk size we let it
> +         * override that size.
> +         */
> +        chunk_max_size = align;
> +    } else {
> +        chunk_max_size = QEMU_ALIGN_DOWN(HV_BALLOON_HA_CHUNK_PAGES, align);
> +    }
> +
> +    /*
> +     * hot_add_range->count starts aligned in hv_balloon_hot_add_setup(),
> +     * then it is either reduced by subtracting aligned current_count or
> +     * further hot-adds are prevented by marking the whole remaining our range
> +     * as unusable in hv_balloon_handle_hot_add_response().
> +     */
> +    *current_count = MIN(hot_add_range->count, chunk_max_size);
> +
> +    ha = alloca(ha_size);
> +    ha_region = &(&ha->range)[1];
> +    memset(ha, 0, ha_size);
> +    ha->hdr.type = DM_MEM_HOT_ADD_REQUEST;
> +    ha->hdr.size = ha_size;
> +    ha->hdr.trans_id = balloon->trans_id;
> +
> +    ha->range.finfo.start_page = hot_add_range->start;
> +    ha->range.finfo.page_cnt = *current_count;
> +    ha_region->finfo.start_page = hot_add_range->start;
> +    ha_region->finfo.page_cnt = ha->range.finfo.page_cnt;
> +
> +    trace_hv_balloon_outgoing_hot_add(ha->hdr.trans_id,
> +                                      *current_count, hot_add_range->start);
> +
> +    ret = vmbus_channel_send(chan, VMBUS_PACKET_DATA_INBAND,
> +                             NULL, 0, ha, ha_size, false,
> +                             ha->hdr.trans_id);
> +    if (ret <= 0) {
> +        error_report("error %zd when posting hot add msg, expect problems",
> +                     ret);
> +    }
> +
> +    HV_BALLOON_STATE_DESC_SET(stdesc, S_HOT_ADD_REPLY_WAIT);
> +}

thanks
-- PMM