On Fri, 3 Mar 2023 22:47:20 -0800
Ira Weiny <ira.we...@intel.com> wrote:
> 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?
Fair enough. I think this is a case of code evolving to a state
that is non optimal in the end so I'll refactor it to something like
you have suggested.
I think we caan simplify it further by dragging the cacheline clear
up to before the list manipulation.
>
> ...
> 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 '{' '}'.
Gah.
>
> Ira
Thanks,
Jonathan
