| Message ID | 20230302101710.1652-7-Jonathan.Cameron@huawei.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | hw/cxl: Poison get, inject, clear | expand |
Jonathan Cameron wrote: > Current implementation is very simple so many of the corner > cases do not exist (e.g. fragmenting larger poison list entries) > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > --- > v2: > - Endian fix > --- > hw/cxl/cxl-mailbox-utils.c | 79 +++++++++++++++++++++++++++++++++++++ > hw/mem/cxl_type3.c | 36 +++++++++++++++++ > include/hw/cxl/cxl_device.h | 1 + > 3 files changed, 116 insertions(+) > > diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c > index da8732a547..f2a339bedc 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,82 @@ 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; > + } > + > + QLIST_FOREACH(ent, poison_list, node) { Because you are removing from the list I think this needs to be QLIST_FOREACH_SAFE()? > + /* > + * 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--; > + if (dpa > ent->start) { > + CXLPoison *frag; > + if (ct3d->poison_list_cnt == CXL_POISON_LIST_LIMIT) { I'm still not seeing how this is ever going to be true with the above decrement? > + cxl_set_poison_list_overflowed(ct3d); > + break; > + } > + 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) { Or this one. > + cxl_set_poison_list_overflowed(ct3d); > + break; > + } > + > + 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); I think the decrement needs to happen here. > + g_free(ent); > + break; > + } > + /* Clearing a region with no poison is not an error so always do so */ > + if (cvc->set_cacheline) > + if (!cvc->set_cacheline(ct3d, dpa, in->data)) { > + return CXL_MBOX_INTERNAL_ERROR; > + } > + > + return CXL_MBOX_SUCCESS; > +} > + > #define IMMEDIATE_CONFIG_CHANGE (1 << 1) > #define IMMEDIATE_DATA_CHANGE (1 << 2) > #define IMMEDIATE_POLICY_CHANGE (1 << 3) > @@ -542,6 +619,8 @@ static struct cxl_cmd cxl_cmd_set[256][256] = { > cmd_media_get_poison_list, 16, 0 }, > [MEDIA_AND_POISON][INJECT_POISON] = { "MEDIA_AND_POISON_INJECT_POISON", > cmd_media_inject_poison, 8, 0 }, > + [MEDIA_AND_POISON][CLEAR_POISON] = { "MEDIA_AND_POISON_CLEAR_POISON", > + cmd_media_clear_poison, 72, 0 }, > }; > > void cxl_process_mailbox(CXLDeviceState *cxl_dstate) > diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c > index 21e3a84785..44ffc7d9b0 100644 > --- a/hw/mem/cxl_type3.c > +++ b/hw/mem/cxl_type3.c > @@ -919,6 +919,41 @@ static void set_lsa(CXLType3Dev *ct3d, const void *buf, uint64_t size, > */ > } > > +static bool set_cacheline(CXLType3Dev *ct3d, uint64_t dpa_offset, uint8_t *data) > +{ > + MemoryRegion *vmr = NULL, *pmr = NULL; > + AddressSpace *as; > + > + if (ct3d->hostvmem) { > + vmr = host_memory_backend_get_memory(ct3d->hostvmem); > + } > + if (ct3d->hostpmem) { > + pmr = host_memory_backend_get_memory(ct3d->hostpmem); > + } > + > + if (!vmr && !pmr) { > + return false; > + } > + > + if (dpa_offset + 64 > int128_get64(ct3d->cxl_dstate.mem_size)) { > + return false; > + } > + > + if (vmr) { > + if (dpa_offset <= int128_get64(vmr->size)) { vmr->size - 64? Ira > + as = &ct3d->hostvmem_as; > + } else { > + as = &ct3d->hostpmem_as; > + dpa_offset -= vmr->size; > + } > + } else { > + as = &ct3d->hostpmem_as; > + } > + > + address_space_write(as, dpa_offset, MEMTXATTRS_UNSPECIFIED, &data, 64); > + return true; > +} > + > void cxl_set_poison_list_overflowed(CXLType3Dev *ct3d) > { > ct3d->poison_list_overflowed = true; > @@ -1140,6 +1175,7 @@ static void ct3_class_init(ObjectClass *oc, void *data) > cvc->get_lsa_size = get_lsa_size; > cvc->get_lsa = get_lsa; > cvc->set_lsa = set_lsa; > + cvc->set_cacheline = set_cacheline; > } > > static const TypeInfo ct3d_info = { > diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h > index 32c234ea91..73328a52cf 100644 > --- a/include/hw/cxl/cxl_device.h > +++ b/include/hw/cxl/cxl_device.h > @@ -298,6 +298,7 @@ struct CXLType3Class { > uint64_t offset); > void (*set_lsa)(CXLType3Dev *ct3d, const void *buf, uint64_t size, > uint64_t offset); > + bool (*set_cacheline)(CXLType3Dev *ct3d, uint64_t dpa_offset, uint8_t *data); > }; > > MemTxResult cxl_type3_read(PCIDevice *d, hwaddr host_addr, uint64_t *data, > -- > 2.37.2 >
On Thu, 2 Mar 2023 17:05:22 -0800 Ira Weiny <ira.weiny@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) > > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > --- > > v2: > > - Endian fix > > --- > > hw/cxl/cxl-mailbox-utils.c | 79 +++++++++++++++++++++++++++++++++++++ > > hw/mem/cxl_type3.c | 36 +++++++++++++++++ > > include/hw/cxl/cxl_device.h | 1 + > > 3 files changed, 116 insertions(+) > > > > diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c > > index da8732a547..f2a339bedc 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,82 @@ 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; > > + } > > + > > + QLIST_FOREACH(ent, poison_list, node) { > > Because you are removing from the list I think this needs to be > QLIST_FOREACH_SAFE()? > I don't think we need it because we break immediately after the remove so the list pointers are touched any more. > > + /* > > + * 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--; > > + if (dpa > ent->start) { > > + CXLPoison *frag; > > + if (ct3d->poison_list_cnt == CXL_POISON_LIST_LIMIT) { > > I'm still not seeing how this is ever going to be true with the above > decrement? Ah I'd missed the comment previously. Agreed on this one. > > > + cxl_set_poison_list_overflowed(ct3d); > > + break; > > + } > > + 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) { > > Or this one. This one however is needed but has another bug (sigh) This is the hole punching case. We are removing the middle of an existing long entry. As such we take one away and gain 2. At this stage we've already gained one. Whilst we know we are going to remove one element later, we will still end up overflowing. Not overflow is > CXL_POISON_LIST_LIMIT The bug that is hiding here is that we have added an entry, but not removed any yet. So if we break in this condition we are over by one. The spec is silent on what should happen in this case (I think) so best we can do is add the first new entry in place of the old entry and just overflow for the second new entry. > > > + cxl_set_poison_list_overflowed(ct3d); > > + break; So no break here, and following code in an 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); > > I think the decrement needs to happen here. For the implementation this was meant to be (without the bug above) we are cheating a little. We 'could' take the entry off the list first before working out if we need to add the entries in. Thus ensuring we remain below the list limit. Instead we leave it on the list, add new elements and drop it later. To do that we need to briefly allow the list to be one larger than it will be at the end of the code. That's done by doing the decrement early so that the rest of the code runs with a list that is one bigger than it appears to be. > > > + g_free(ent); > > + break; > > + } > > + /* Clearing a region with no poison is not an error so always do so */ > > + if (cvc->set_cacheline) > > + if (!cvc->set_cacheline(ct3d, dpa, in->data)) { > > + return CXL_MBOX_INTERNAL_ERROR; > > + } > > + > > + return CXL_MBOX_SUCCESS; > > +} > > + > > #define IMMEDIATE_CONFIG_CHANGE (1 << 1) > > #define IMMEDIATE_DATA_CHANGE (1 << 2) > > #define IMMEDIATE_POLICY_CHANGE (1 << 3) > > @@ -542,6 +619,8 @@ static struct cxl_cmd cxl_cmd_set[256][256] = { > > cmd_media_get_poison_list, 16, 0 }, > > [MEDIA_AND_POISON][INJECT_POISON] = { "MEDIA_AND_POISON_INJECT_POISON", > > cmd_media_inject_poison, 8, 0 }, > > + [MEDIA_AND_POISON][CLEAR_POISON] = { "MEDIA_AND_POISON_CLEAR_POISON", > > + cmd_media_clear_poison, 72, 0 }, > > }; > > > > void cxl_process_mailbox(CXLDeviceState *cxl_dstate) > > diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c > > index 21e3a84785..44ffc7d9b0 100644 > > --- a/hw/mem/cxl_type3.c > > +++ b/hw/mem/cxl_type3.c > > @@ -919,6 +919,41 @@ static void set_lsa(CXLType3Dev *ct3d, const void *buf, uint64_t size, > > */ > > } > > > > +static bool set_cacheline(CXLType3Dev *ct3d, uint64_t dpa_offset, uint8_t *data) > > +{ > > + MemoryRegion *vmr = NULL, *pmr = NULL; > > + AddressSpace *as; > > + > > + if (ct3d->hostvmem) { > > + vmr = host_memory_backend_get_memory(ct3d->hostvmem); > > + } > > + if (ct3d->hostpmem) { > > + pmr = host_memory_backend_get_memory(ct3d->hostpmem); > > + } > > + > > + if (!vmr && !pmr) { > > + return false; > > + } > > + > > + if (dpa_offset + 64 > int128_get64(ct3d->cxl_dstate.mem_size)) { > > + return false; > > + } > > + > > + if (vmr) { > > + if (dpa_offset <= int128_get64(vmr->size)) { > > vmr->size - 64? Copy and paste error. Until very recently this was broken in volatile patch series and I'd missed code was duplicated here. Given they are 64 byte aligned we can make this simply < int128_get64(vmr->size) Thanks, Jonathan > Ira
diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c index da8732a547..f2a339bedc 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,82 @@ 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; + } + + 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--; + if (dpa > ent->start) { + CXLPoison *frag; + if (ct3d->poison_list_cnt == CXL_POISON_LIST_LIMIT) { + cxl_set_poison_list_overflowed(ct3d); + break; + } + 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); + break; + } + + 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; + } + /* Clearing a region with no poison is not an error so always do so */ + if (cvc->set_cacheline) + if (!cvc->set_cacheline(ct3d, dpa, in->data)) { + return CXL_MBOX_INTERNAL_ERROR; + } + + return CXL_MBOX_SUCCESS; +} + #define IMMEDIATE_CONFIG_CHANGE (1 << 1) #define IMMEDIATE_DATA_CHANGE (1 << 2) #define IMMEDIATE_POLICY_CHANGE (1 << 3) @@ -542,6 +619,8 @@ static struct cxl_cmd cxl_cmd_set[256][256] = { cmd_media_get_poison_list, 16, 0 }, [MEDIA_AND_POISON][INJECT_POISON] = { "MEDIA_AND_POISON_INJECT_POISON", cmd_media_inject_poison, 8, 0 }, + [MEDIA_AND_POISON][CLEAR_POISON] = { "MEDIA_AND_POISON_CLEAR_POISON", + cmd_media_clear_poison, 72, 0 }, }; void cxl_process_mailbox(CXLDeviceState *cxl_dstate) diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c index 21e3a84785..44ffc7d9b0 100644 --- a/hw/mem/cxl_type3.c +++ b/hw/mem/cxl_type3.c @@ -919,6 +919,41 @@ static void set_lsa(CXLType3Dev *ct3d, const void *buf, uint64_t size, */ } +static bool set_cacheline(CXLType3Dev *ct3d, uint64_t dpa_offset, uint8_t *data) +{ + MemoryRegion *vmr = NULL, *pmr = NULL; + AddressSpace *as; + + if (ct3d->hostvmem) { + vmr = host_memory_backend_get_memory(ct3d->hostvmem); + } + if (ct3d->hostpmem) { + pmr = host_memory_backend_get_memory(ct3d->hostpmem); + } + + if (!vmr && !pmr) { + return false; + } + + if (dpa_offset + 64 > int128_get64(ct3d->cxl_dstate.mem_size)) { + return false; + } + + if (vmr) { + if (dpa_offset <= int128_get64(vmr->size)) { + as = &ct3d->hostvmem_as; + } else { + as = &ct3d->hostpmem_as; + dpa_offset -= vmr->size; + } + } else { + as = &ct3d->hostpmem_as; + } + + address_space_write(as, dpa_offset, MEMTXATTRS_UNSPECIFIED, &data, 64); + return true; +} + void cxl_set_poison_list_overflowed(CXLType3Dev *ct3d) { ct3d->poison_list_overflowed = true; @@ -1140,6 +1175,7 @@ static void ct3_class_init(ObjectClass *oc, void *data) cvc->get_lsa_size = get_lsa_size; cvc->get_lsa = get_lsa; cvc->set_lsa = set_lsa; + cvc->set_cacheline = set_cacheline; } static const TypeInfo ct3d_info = { diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h index 32c234ea91..73328a52cf 100644 --- a/include/hw/cxl/cxl_device.h +++ b/include/hw/cxl/cxl_device.h @@ -298,6 +298,7 @@ struct CXLType3Class { uint64_t offset); void (*set_lsa)(CXLType3Dev *ct3d, const void *buf, uint64_t size, uint64_t offset); + bool (*set_cacheline)(CXLType3Dev *ct3d, uint64_t dpa_offset, uint8_t *data); }; MemTxResult cxl_type3_read(PCIDevice *d, hwaddr host_addr, uint64_t *data,
Current implementation is very simple so many of the corner cases do not exist (e.g. fragmenting larger poison list entries) Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> --- v2: - Endian fix --- hw/cxl/cxl-mailbox-utils.c | 79 +++++++++++++++++++++++++++++++++++++ hw/mem/cxl_type3.c | 36 +++++++++++++++++ include/hw/cxl/cxl_device.h | 1 + 3 files changed, 116 insertions(+)