Re: [PATCH v4 6/6] hw/cxl: Add clear poison mailbox command support.

[Ira Weiny]

Jonathan Cameron wrote:
> Current implementation is very simple so many of the corner
> cases do not exist (e.g. fragmenting larger poison list entries)

One coding style change at the bottom and I'm still hung up on that loop
logic...

> 
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
> v4:
> - Fix off by one on check of edge of vmr (cut and paste from similar
>   but long fixed in the volatile memory series)
> - Drop unnecessary overflow check.
> - Ensure that even in case of overflow we still delete the element
>   replaced (in the hole punching case)
> ---
>  hw/cxl/cxl-mailbox-utils.c  | 77 +++++++++++++++++++++++++++++++++++++
>  hw/mem/cxl_type3.c          | 36 +++++++++++++++++
>  include/hw/cxl/cxl_device.h |  1 +
>  3 files changed, 114 insertions(+)
> 
> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> index 64a3f3c1bf..0b30307fa3 100644
> --- a/hw/cxl/cxl-mailbox-utils.c
> +++ b/hw/cxl/cxl-mailbox-utils.c
> @@ -65,6 +65,7 @@ enum {
>      MEDIA_AND_POISON = 0x43,
>          #define GET_POISON_LIST        0x0
>          #define INJECT_POISON          0x1
> +        #define CLEAR_POISON           0x2
>  };
>  
>  /* 8.2.8.4.5.1 Command Return Codes */
> @@ -511,6 +512,80 @@ static CXLRetCode cmd_media_inject_poison(struct cxl_cmd 
> *cmd,
>      return CXL_MBOX_SUCCESS;
>  }
>  
> +static CXLRetCode cmd_media_clear_poison(struct cxl_cmd *cmd,
> +                                         CXLDeviceState *cxl_dstate,
> +                                         uint16_t *len)
> +{
> +    CXLType3Dev *ct3d = container_of(cxl_dstate, CXLType3Dev, cxl_dstate);
> +    CXLPoisonList *poison_list = &ct3d->poison_list;
> +    CXLType3Class *cvc = CXL_TYPE3_GET_CLASS(ct3d);
> +    struct clear_poison_pl {
> +        uint64_t dpa;
> +        uint8_t data[64];
> +    };
> +    CXLPoison *ent;
> +    uint64_t dpa;
> +
> +    struct clear_poison_pl *in = (void *)cmd->payload;
> +
> +    dpa = ldq_le_p(&in->dpa);
> +    if (dpa + 64 > cxl_dstate->mem_size) {
> +        return CXL_MBOX_INVALID_PA;
> +    }
> +
> +    /* Always exit loop on entry removal so no need for safe variant */

Commenting this is nice but I don't think it is needed.

> +    QLIST_FOREACH(ent, poison_list, node) {
> +        /*
> +         * Test for contained in entry. Simpler than general case
> +         * as clearing 64 bytes and entries 64 byte aligned
> +         */
> +        if ((dpa < ent->start) || (dpa >= ent->start + ent->length)) {
> +            continue;
> +        }
> +        /* Do accounting early as we know one will go away */
> +        ct3d->poison_list_cnt--;

Sorry to get so hung up on this but while I think this code now works I
still think it is odd and will be an issue to maintain.

FWIW I don't think we have to keep 'ent' in the list here...

> +        if (dpa > ent->start) {
> +            CXLPoison *frag;
> +            /* Cannot overflow as replacing existing entry */
> +
> +            frag = g_new0(CXLPoison, 1);
> +
> +            frag->start = ent->start;
> +            frag->length = dpa - ent->start;
> +            frag->type = ent->type;
> +
> +            QLIST_INSERT_HEAD(poison_list, frag, node);
> +            ct3d->poison_list_cnt++;
> +        }
> +        if (dpa + 64 < ent->start + ent->length) {
> +            CXLPoison *frag;
> +
> +            if (ct3d->poison_list_cnt == CXL_POISON_LIST_LIMIT) {
> +                cxl_set_poison_list_overflowed(ct3d);
> +            } else {
> +                frag = g_new0(CXLPoison, 1);
> +
> +                frag->start = dpa + 64;
> +                frag->length = ent->start + ent->length - frag->start;
> +                frag->type = ent->type;
> +                QLIST_INSERT_HEAD(poison_list, frag, node);
> +                ct3d->poison_list_cnt++;
> +            }
> +        }
> +        /* Any fragments have been added, free original entry */
> +        QLIST_REMOVE(ent, node);
> +        g_free(ent);
> +        break;
> +    }

Why not this?

...
    CXLPoison *ent, found = NULL;

...
    QLIST_FOREACH(ent, poison_list, node) {
        /*
         * Test for contained in entry. Simpler than general case
         * as clearing 64 bytes and entries are 64 byte aligned
         */
        if ((dpa >= ent->start) && (dpa < ent->start + ent->length)) {
            found = ent;
            break;
        }
    }

    /*
     * Do we even need 'found'?  Or is ent null if not found?
     * I'm not sure how QLIST's work.
     */
    if (found) {
        CXLPoison *frag;

        QLIST_REMOVE(found, node);
        ct3d->poison_list_cnt--;

        /* If not clearing the start, create new beginning of range */
        if (dpa > found->start) {
            frag = g_new0(CXLPoison, 1);
            frag->start = found->start;
            frag->length = dpa - found->start;
            frag->type = found->type;
            QLIST_INSERT_HEAD(poison_list, frag, node);
            ct3d->poison_list_cnt++;
        }

        /* If needed, and space available, create new end of range */
        if (dpa + 64 < found->start + found->length) {
            if (ct3d->poison_list_cnt == CXL_POISON_LIST_LIMIT) {
                cxl_set_poison_list_overflowed(ct3d);
            } else {
                frag = g_new0(CXLPoison, 1);

                frag->start = dpa + 64;
                frag->length = found->start + found->length - frag->start;
                frag->type = found->type;
                QLIST_INSERT_HEAD(poison_list, frag, node);
                ct3d->poison_list_cnt++;
            }
        }
        g_free(found);
    }
...

> +    /* Clearing a region with no poison is not an error so always do so */
> +    if (cvc->set_cacheline)

For QEMU coding style you still need '{' '}'.

Ira