Re: [PULL 57/63] hw/cxl: Add support for device sanitation

[Peter Maydell]

On Tue, 7 Nov 2023 at 10:13, Michael S. Tsirkin <mst@redhat.com> wrote:
>
> From: Davidlohr Bueso <dave@stgolabs.net>
>
> Make use of the background operations through the sanitize command, per CXL
> 3.0 specs. Traditionally run times can be rather long, depending on the
> size of the media.
>
> Estimate times based on:
>          https://pmem.io/documents/NVDIMM_DSM_Interface-V1.8.pdf
>

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

> +/*
> + * CXL 3.0 spec section 8.2.9.8.5.1 - Sanitize.
> + *
> + * Once the Sanitize command has started successfully, the device shall be
> + * placed in the media disabled state. If the command fails or is interrupted
> + * by a reset or power failure, it shall remain in the media disabled state
> + * until a successful Sanitize command has been completed. During this state:
> + *
> + * 1. Memory writes to the device will have no effect, and all memory reads
> + * will return random values (no user data returned, even for locations that
> + * the failed Sanitize operation didn’t sanitize yet).
> + *
> + * 2. Mailbox commands shall still be processed in the disabled state, except
> + * that commands that access Sanitized areas shall fail with the Media 
> Disabled
> + * error code.
> + */
> +static CXLRetCode cmd_sanitize_overwrite(const struct cxl_cmd *cmd,
> +                                         uint8_t *payload_in,
> +                                         size_t len_in,
> +                                         uint8_t *payload_out,
> +                                         size_t *len_out,
> +                                         CXLCCI *cci)
> +{
> +    CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
> +    uint64_t total_mem; /* in Mb */
> +    int secs;
> +
> +    total_mem = (ct3d->cxl_dstate.vmem_size + ct3d->cxl_dstate.pmem_size) >> 
> 20;
> +    if (total_mem <= 512) {
> +        secs = 4;
> +    } else if (total_mem <= 1024) {
> +        secs = 8;
> +    } else if (total_mem <= 2 * 1024) {
> +        secs = 15;
> +    } else if (total_mem <= 4 * 1024) {
> +        secs = 30;
> +    } else if (total_mem <= 8 * 1024) {
> +        secs = 60;
> +    } else if (total_mem <= 16 * 1024) {
> +        secs = 2 * 60;
> +    } else if (total_mem <= 32 * 1024) {
> +        secs = 4 * 60;
> +    } else if (total_mem <= 64 * 1024) {
> +        secs = 8 * 60;
> +    } else if (total_mem <= 128 * 1024) {
> +        secs = 15 * 60;
> +    } else if (total_mem <= 256 * 1024) {
> +        secs = 30 * 60;
> +    } else if (total_mem <= 512 * 1024) {
> +        secs = 60 * 60;
> +    } else if (total_mem <= 1024 * 1024) {
> +        secs = 120 * 60;
> +    } else {
> +        secs = 240 * 60; /* max 4 hrs */
> +    }

Here we have an exhaustive if ladder that sets 'secs'. None
of the values we might end up with are less than 4.

> +
> +    /* EBUSY other bg cmds as of now */
> +    cci->bg.runtime = secs * 1000UL;
> +    *len_out = 0;
> +
> +    cxl_dev_disable_media(&ct3d->cxl_dstate);
> +
> +    if (secs > 2) {
> +        /* sanitize when done */
> +        return CXL_MBOX_BG_STARTED;
> +    } else {

...but here we have an else clause for when secs <= 2,
which can never happen.

> +        __do_sanitization(ct3d);
> +        cxl_dev_enable_media(&ct3d->cxl_dstate);
> +
> +        return CXL_MBOX_SUCCESS;
> +    }

What was the intention here ?

> +}

thanks
-- PMM