Re: [PULL 55/63] hw/cxl/mbox: Add support for background operations

[Peter Maydell]

On Tue, 7 Nov 2023 at 10:13, Michael S. Tsirkin <mst@redhat.com> wrote:
>
> From: Davidlohr Bueso <dave@stgolabs.net>
>
> Support background commands in the mailbox, and update
> cmd_infostat_bg_op_sts() accordingly. This patch does not implement mbox
> interrupts upon completion, so the kernel driver must rely on polling to
> know when the operation is done.
>
> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
> Message-Id: <20231023160806.13206-12-Jonathan.Cameron@huawei.com>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

Hi; Coverity points out dead code in this function (CID 1523907):

> +static void bg_timercb(void *opaque)
> +{
> +    CXLCCI *cci = opaque;
> +    CXLDeviceState *cxl_dstate = &CXL_TYPE3(cci->d)->cxl_dstate;
> +    uint64_t bg_status_reg = 0;
> +    uint64_t now = qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL);
> +    uint64_t total_time = cci->bg.starttime + cci->bg.runtime;
> +
> +    assert(cci->bg.runtime > 0);
> +    bg_status_reg = FIELD_DP64(bg_status_reg, CXL_DEV_BG_CMD_STS,
> +                               OP, cci->bg.opcode);
> +
> +    if (now >= total_time) { /* we are done */
> +        uint64_t status_reg;
> +        uint16_t ret = CXL_MBOX_SUCCESS;

Here we set 'ret' to CXL_MBOX_SUCCESS...

> +
> +        cci->bg.complete_pct = 100;
> +        /* Clear bg */
> +        status_reg = FIELD_DP64(0, CXL_DEV_MAILBOX_STS, BG_OP, 0);
> +        cxl_dstate->mbox_reg_state64[R_CXL_DEV_MAILBOX_STS] = status_reg;
> +
> +        bg_status_reg = FIELD_DP64(bg_status_reg, CXL_DEV_BG_CMD_STS,
> +                                   RET_CODE, ret);

...and nothing here changes 'ret'...

> +
> +        /* TODO add ad-hoc cmd succesful completion handling */
> +
> +        qemu_log("Background command %04xh finished: %s\n",
> +                 cci->bg.opcode,
> +                 ret == CXL_MBOX_SUCCESS ? "success" : "aborted");

...but here we check whether ret is CXL_MBOX_SUCCESS or not:
the "aborted" half of this condition is dead code.

A later commit adds an "if (ret == CXL_MBOX_SUCCESS) {" block
where the TODO currently is, and that is an unnecessary check,
because ret cannot be anything else at that point.

> +    } else {
> +        /* estimate only */
> +        cci->bg.complete_pct = 100 * now / total_time;
> +        timer_mod(cci->bg.timer, now + CXL_MBOX_BG_UPDATE_FREQ);
> +    }

What was the intention here ?

thanks
-- PMM