Re: [PATCH 3/5] tools/testing/cxl: Mock the Inject Poison mailbox command

[Alison Schofield <alison.schofield@intel.com>]

On Wed, Nov 30, 2022 at 02:58:47PM +0000, Jonathan Cameron wrote:
> On Tue, 29 Nov 2022 20:34:35 -0800
> alison.schofield@intel.com wrote:
> 
> > From: Alison Schofield <alison.schofield@intel.com>
> > 
> > Mock the injection of poison by storing the device:address entry in a
> > cxl_test array: mock_poison[]. Limit the array to 64 entries and fail
> > new inject requests when full.
> > 
> > Introducing the mock_poison[] list here, makes it available for use in
> > the mock of Clear Poison, and the mock of Get Poison List.
> > 
> > Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> 
> Main question I have here is whether we want to mock per device injected poison
> lists, or just one global.  I think we want per device so we can reflect
> the limits as would be retrieved from Identify Memory Device Output Payload.
> Whilst we don't do anything useful with it yet, we should also update
> the mocked response to that command to reflect this.
> 
> Perhaps we should have a sysfs attribute to read how many entries we can
> inject?  Seems like that would be useful for testing flows on real devices,
> particularly if the device only supports a very small number!

Jonathan,

Thanks for the review and helping me see the use case. I see the usefulness of
the per device model but am not clear on the sysfs attr for number of entries.

If user wants to know how many entries can be injected, just inject til I tell
you no more!  Remember that count, run your test w it.

If user wants to dial down that number to something custom, then I can
see RW attr being useful. Mock driver sets it to a max, and user can
lower it.

Is that useful?

Alison

> 
> > ---
> >  tools/testing/cxl/test/mem.c | 53 ++++++++++++++++++++++++++++++++++++
> >  1 file changed, 53 insertions(+)
> > 
> > diff --git a/tools/testing/cxl/test/mem.c b/tools/testing/cxl/test/mem.c
> > index a4f81915ec03..98acb9a644df 100644
> > --- a/tools/testing/cxl/test/mem.c
> > +++ b/tools/testing/cxl/test/mem.c
> > @@ -13,6 +13,7 @@
> >  #define LSA_SIZE SZ_128K
> >  #define DEV_SIZE SZ_2G
> >  #define EFFECT(x) (1U << x)
> > +#define MOCK_INJECT_POISON_MAX 64
> >  
> >  static struct cxl_cel_entry mock_cel[] = {
> >  	{
> > @@ -43,6 +44,10 @@ static struct cxl_cel_entry mock_cel[] = {
> >  		.opcode = cpu_to_le16(CXL_MBOX_OP_GET_POISON),
> >  		.effect = cpu_to_le16(0),
> >  	},
> > +	{
> > +		.opcode = cpu_to_le16(CXL_MBOX_OP_INJECT_POISON),
> > +		.effect = cpu_to_le16(0),
> > +	},
> >  };
> >  
> >  /* See CXL 2.0 Table 181 Get Health Info Output Payload */
> > @@ -210,6 +215,51 @@ static int mock_health_info(struct cxl_dev_state *cxlds,
> >  	return 0;
> >  }
> >  
> > +static struct mock_poison {
> > +	struct cxl_dev_state *cxlds;
> > +	u64 dpa;
> > +} mock_poison[MOCK_INJECT_POISON_MAX];
> 
> Don't we want one of these per device instance?
> 
> > +
> > +static bool mock_poison_add(struct cxl_dev_state *cxlds, u64 dpa)
> > +{
> > +	for (int i = 0; i < MOCK_INJECT_POISON_MAX; i++) {
> > +		if (!mock_poison[i].cxlds) {
> > +			mock_poison[i].cxlds = cxlds;
> > +			mock_poison[i].dpa = dpa;
> > +			return true;
> > +		}
> > +	}
> > +	dev_dbg(cxlds->dev, "Mock poison list full: %d\n",
> > +		MOCK_INJECT_POISON_MAX);
> Slightly nicer maybe to use the text from
> https://elixir.bootlin.com/linux/latest/source/drivers/cxl/cxlmem.h#L128
> and have: "Mock poison injection limit has been reached: %d\n" ..
> 
> > +	return false;
> > +}
>