Re: [PATCH 5/6] hw/cxl: Add poison injection via the mailbox.

[Jonathan Cameron]

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
> >   
> 
>