| Message ID | 20230217181812.26995-6-Jonathan.Cameron@huawei.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | hw/cxl: Poison get, inject, clear | expand |
Jonathan Cameron wrote: > Very simple implementation to allow testing of corresponding > kernel code. Note that for now we track each 64 byte section > independently. Whilst a valid implementation choice, it may > make sense to fuse entries so as to prove out more complex > corners of the kernel code. > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > --- > hw/cxl/cxl-mailbox-utils.c | 40 ++++++++++++++++++++++++++++++++++++++ > 1 file changed, 40 insertions(+) > > diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c > index cf3cfb10a1..7d3f7bcd3a 100644 > --- a/hw/cxl/cxl-mailbox-utils.c > +++ b/hw/cxl/cxl-mailbox-utils.c > @@ -64,6 +64,7 @@ enum { > #define SET_LSA 0x3 > MEDIA_AND_POISON = 0x43, > #define GET_POISON_LIST 0x0 > + #define INJECT_POISON 0x1 > }; > > struct cxl_cmd; > @@ -436,6 +437,43 @@ static CXLRetCode cmd_media_get_poison_list(struct cxl_cmd *cmd, > return CXL_MBOX_SUCCESS; > } > > +static CXLRetCode cmd_media_inject_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; > + CXLPoison *ent; > + struct inject_poison_pl { > + uint64_t dpa; > + }; > + struct inject_poison_pl *in = (void *)cmd->payload; > + CXLPoison *p; > + > + QLIST_FOREACH(ent, poison_list, node) { > + if (ent->start == in->dpa && ent->length == 64) { How does this interact with the QMP inject poison? Should this be checking the range of the entries? Ira > + return CXL_MBOX_SUCCESS; > + } > + } > + > + if (ct3d->poison_list_cnt == CXL_POISON_LIST_LIMIT) { > + return CXL_MBOX_INJECT_POISON_LIMIT; > + } > + p = g_new0(CXLPoison, 1); > + > + p->length = 64; > + p->start = in->dpa; > + p->type = CXL_POISON_TYPE_INJECTED; > + > + /* > + * Possible todo: Merge with existing entry if next to it and if same type > + */ > + QLIST_INSERT_HEAD(poison_list, p, node); > + ct3d->poison_list_cnt++; > + > + return CXL_MBOX_SUCCESS; > +} > + > #define IMMEDIATE_CONFIG_CHANGE (1 << 1) > #define IMMEDIATE_DATA_CHANGE (1 << 2) > #define IMMEDIATE_POLICY_CHANGE (1 << 3) > @@ -465,6 +503,8 @@ static struct cxl_cmd cxl_cmd_set[256][256] = { > ~0, IMMEDIATE_CONFIG_CHANGE | IMMEDIATE_DATA_CHANGE }, > [MEDIA_AND_POISON][GET_POISON_LIST] = { "MEDIA_AND_POISON_GET_POISON_LIST", > cmd_media_get_poison_list, 16, 0 }, > + [MEDIA_AND_POISON][INJECT_POISON] = { "MEDIA_AND_POISON_INJECT_POISON", > + cmd_media_inject_poison, 8, 0 }, > }; > > void cxl_process_mailbox(CXLDeviceState *cxl_dstate) > -- > 2.37.2 >
On Tue, 21 Feb 2023 17:18:01 -0800 Ira Weiny <ira.weiny@intel.com> wrote: > Jonathan Cameron wrote: > > Very simple implementation to allow testing of corresponding > > kernel code. Note that for now we track each 64 byte section > > independently. Whilst a valid implementation choice, it may > > make sense to fuse entries so as to prove out more complex > > corners of the kernel code. > > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > --- > > hw/cxl/cxl-mailbox-utils.c | 40 ++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 40 insertions(+) > > > > diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c > > index cf3cfb10a1..7d3f7bcd3a 100644 > > --- a/hw/cxl/cxl-mailbox-utils.c > > +++ b/hw/cxl/cxl-mailbox-utils.c > > @@ -64,6 +64,7 @@ enum { > > #define SET_LSA 0x3 > > MEDIA_AND_POISON = 0x43, > > #define GET_POISON_LIST 0x0 > > + #define INJECT_POISON 0x1 > > }; > > > > struct cxl_cmd; > > @@ -436,6 +437,43 @@ static CXLRetCode cmd_media_get_poison_list(struct cxl_cmd *cmd, > > return CXL_MBOX_SUCCESS; > > } > > > > +static CXLRetCode cmd_media_inject_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; > > + CXLPoison *ent; > > + struct inject_poison_pl { > > + uint64_t dpa; > > + }; > > + struct inject_poison_pl *in = (void *)cmd->payload; > > + CXLPoison *p; > > + > > + QLIST_FOREACH(ent, poison_list, node) { > > + if (ent->start == in->dpa && ent->length == 64) { > > How does this interact with the QMP inject poison? Should this be > checking the range of the entries? Good question and this implementation is definitely not right. Having looked at the spec I'm not entirely sure what happens wrt to the poison list if there is overlap. It leaves things less sharply defined than I'd like. The inject poison command calls out that it is not an error to inject poison into a DPA that already has poison present - so a range match would make more sense than what is here - I'll fix that. It also calls out that the device "shall add a the new physical address to the device's poison list and error source shall be set to an injected event". What it doesn't say is what should it do if there is already an entry for a different poison type. Should we update the type? That's nasty as it could lead to list overflow by turning one region into 2 or 3. I guess no one really cares that much on the precise detail of poison injection hence the spec is a little vague. Anyhow, for now I'm thinking that if a range contains matches leave the type alone is easy and I can't find anything to say that's not a valid implementation choice. As a side note, we don't yet have events support (that series is later in the tree) so that fact injecting poison should add an entry to that isn't happening. I don't propose doing that until after the generic event injection is done though as it will otherwise make that series more complex and I doubt this is the only case we need to cover of these various error reporting paths interacting. Jonathan > > Ira > > > + return CXL_MBOX_SUCCESS; > > + } > > + } > > + > > + if (ct3d->poison_list_cnt == CXL_POISON_LIST_LIMIT) { > > + return CXL_MBOX_INJECT_POISON_LIMIT; > > + } > > + p = g_new0(CXLPoison, 1); > > + > > + p->length = 64; > > + p->start = in->dpa; > > + p->type = CXL_POISON_TYPE_INJECTED; > > + > > + /* > > + * Possible todo: Merge with existing entry if next to it and if same type > > + */ > > + QLIST_INSERT_HEAD(poison_list, p, node); > > + ct3d->poison_list_cnt++; > > + > > + return CXL_MBOX_SUCCESS; > > +} > > + > > #define IMMEDIATE_CONFIG_CHANGE (1 << 1) > > #define IMMEDIATE_DATA_CHANGE (1 << 2) > > #define IMMEDIATE_POLICY_CHANGE (1 << 3) > > @@ -465,6 +503,8 @@ static struct cxl_cmd cxl_cmd_set[256][256] = { > > ~0, IMMEDIATE_CONFIG_CHANGE | IMMEDIATE_DATA_CHANGE }, > > [MEDIA_AND_POISON][GET_POISON_LIST] = { "MEDIA_AND_POISON_GET_POISON_LIST", > > cmd_media_get_poison_list, 16, 0 }, > > + [MEDIA_AND_POISON][INJECT_POISON] = { "MEDIA_AND_POISON_INJECT_POISON", > > + cmd_media_inject_poison, 8, 0 }, > > }; > > > > void cxl_process_mailbox(CXLDeviceState *cxl_dstate) > > -- > > 2.37.2 > > > >
diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c index cf3cfb10a1..7d3f7bcd3a 100644 --- a/hw/cxl/cxl-mailbox-utils.c +++ b/hw/cxl/cxl-mailbox-utils.c @@ -64,6 +64,7 @@ enum { #define SET_LSA 0x3 MEDIA_AND_POISON = 0x43, #define GET_POISON_LIST 0x0 + #define INJECT_POISON 0x1 }; struct cxl_cmd; @@ -436,6 +437,43 @@ static CXLRetCode cmd_media_get_poison_list(struct cxl_cmd *cmd, return CXL_MBOX_SUCCESS; } +static CXLRetCode cmd_media_inject_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; + CXLPoison *ent; + struct inject_poison_pl { + uint64_t dpa; + }; + struct inject_poison_pl *in = (void *)cmd->payload; + CXLPoison *p; + + QLIST_FOREACH(ent, poison_list, node) { + if (ent->start == in->dpa && ent->length == 64) { + return CXL_MBOX_SUCCESS; + } + } + + if (ct3d->poison_list_cnt == CXL_POISON_LIST_LIMIT) { + return CXL_MBOX_INJECT_POISON_LIMIT; + } + p = g_new0(CXLPoison, 1); + + p->length = 64; + p->start = in->dpa; + p->type = CXL_POISON_TYPE_INJECTED; + + /* + * Possible todo: Merge with existing entry if next to it and if same type + */ + QLIST_INSERT_HEAD(poison_list, p, node); + ct3d->poison_list_cnt++; + + return CXL_MBOX_SUCCESS; +} + #define IMMEDIATE_CONFIG_CHANGE (1 << 1) #define IMMEDIATE_DATA_CHANGE (1 << 2) #define IMMEDIATE_POLICY_CHANGE (1 << 3) @@ -465,6 +503,8 @@ static struct cxl_cmd cxl_cmd_set[256][256] = { ~0, IMMEDIATE_CONFIG_CHANGE | IMMEDIATE_DATA_CHANGE }, [MEDIA_AND_POISON][GET_POISON_LIST] = { "MEDIA_AND_POISON_GET_POISON_LIST", cmd_media_get_poison_list, 16, 0 }, + [MEDIA_AND_POISON][INJECT_POISON] = { "MEDIA_AND_POISON_INJECT_POISON", + cmd_media_inject_poison, 8, 0 }, }; void cxl_process_mailbox(CXLDeviceState *cxl_dstate)
Very simple implementation to allow testing of corresponding kernel code. Note that for now we track each 64 byte section independently. Whilst a valid implementation choice, it may make sense to fuse entries so as to prove out more complex corners of the kernel code. Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> --- hw/cxl/cxl-mailbox-utils.c | 40 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+)