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.came...@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
