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

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Wed, 7 Dec 2022 20:47:27 -0800
Alison Schofield <alison.schofield@intel.com> wrote:

> 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?
At this stage - no idea ;)

Probably a wait and see question. If no one asks for it, don't bother
providing the support.

Jonathan

> 
> 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;
> > > +}  
> >   
>