diff mbox series

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

Message ID 08eddfad079afbab1f80f17d9670b40b2a7bdea8.1669781852.git.alison.schofield@intel.com
State Superseded
Headers show
Series cxl: CXL Inject & Clear Poison | expand

Commit Message

Alison Schofield Nov. 30, 2022, 4:34 a.m. UTC 
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>
---
 tools/testing/cxl/test/mem.c | 53 ++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

Comments

Jonathan Cameron Nov. 30, 2022, 2:58 p.m. UTC | #1 
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!

> ---
>  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;
> +}
Alison Schofield Dec. 8, 2022, 4:47 a.m. UTC | #2 
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;
> > +}
>
Jonathan Cameron Dec. 8, 2022, 2:53 p.m. UTC | #3 
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;
> > > +}  
> >   
>
diff mbox series

Patch

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];
+
+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);
+	return false;
+}
+
+static bool mock_poison_found(struct cxl_dev_state *cxlds, u64 dpa)
+{
+	for (int i = 0; i < MOCK_INJECT_POISON_MAX; i++) {
+		if (mock_poison[i].cxlds == cxlds &&
+		    mock_poison[i].dpa == dpa)
+			return true;
+	}
+	return false;
+}
+
+static int mock_inject_poison(struct cxl_dev_state *cxlds,
+			      struct cxl_mbox_cmd *cmd)
+{
+	u64 dpa = le64_to_cpu(*((u64 *)cmd->payload_in));
+
+	if (mock_poison_found(cxlds, dpa)) {
+		/* Not an error to inject poison if already poisoned */
+		dev_dbg(cxlds->dev, "DPA: 0x%llx already poisoned\n", dpa);
+		return 0;
+	}
+	if (!mock_poison_add(cxlds, dpa))
+		return -ENXIO;
+
+	return 0;
+}
+
 static int mock_get_poison(struct cxl_dev_state *cxlds, struct cxl_mbox_cmd *cmd)
 {
 	struct cxl_mbox_poison_payload_in *pi = cmd->payload_in;
@@ -268,6 +318,9 @@  static int cxl_mock_mbox_send(struct cxl_dev_state *cxlds, struct cxl_mbox_cmd *
 	case CXL_MBOX_OP_GET_POISON:
 		rc = mock_get_poison(cxlds, cmd);
 		break;
+	case CXL_MBOX_OP_INJECT_POISON:
+		rc = mock_inject_poison(cxlds, cmd);
+		break;
 	default:
 		break;
 	}