Re: [PATCH 5/5] tools/testing/cxl: Use injected poison for Get Poison List

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

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

> On Wed, Nov 30, 2022 at 03:15:22PM +0000, Jonathan Cameron wrote:
> > On Tue, 29 Nov 2022 20:34:37 -0800
> > alison.schofield@intel.com wrote:
> >   
> > > From: Alison Schofield <alison.schofield@intel.com>
> > > 
> > > Prior to poison inject & clear support, the mock of 'Get Poison List'
> > > returned a list containing a single mocked error record.
> > > 
> > > Now that the mock of Inject and Clear poison maintains a mock_poison[]
> > > list, use that when available for the device. Otherwise, return the
> > > existing default mocked error record.
> > > 
> > > This enables roundtrip userspace testing of the inject, clear, and
> > > poison list support.  
> > 
> > I'm not that keen on having an unclearable record, unless you handle that
> > with the right error flow for clear poison.
> > Plus I think you should always return that as well as the injected poison list
> > so this is consistent.
> > 
> > Whether it matters to model it that well in the mocking code?  Up to you...
> > 
> > Jonathan
> >   
> I think I went off the rails here ;)
> With this mock of Inject and Clear, returning totally fake errors is
> useless. So, I would change my above statement to say:
> 
> "Now that the mock of Inject and Clear poison maintains a mock_poison[]
> ist, use that when available for the device."
> 
> Do you agree with that usage Jonathan?  If the user wants to get errors
> when they read the poison list, they need to inject them.
> I guess I also could flip the order of these patchsets and do away
> with the fake poison list entirely.

Sounds good.

> 
> a bit more below...
> 
> > 
> >   
> > > 
> > > Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> > > ---
> > >  tools/testing/cxl/test/mem.c | 97 +++++++++++++++++++++++++-----------
> > >  1 file changed, 69 insertions(+), 28 deletions(-)
> > > 
> > > diff --git a/tools/testing/cxl/test/mem.c b/tools/testing/cxl/test/mem.c
> > > index 9d794fbe5ee1..31c147cf2d5b 100644
> > > --- a/tools/testing/cxl/test/mem.c
> > > +++ b/tools/testing/cxl/test/mem.c
> > > @@ -224,6 +224,75 @@ static struct mock_poison {
> > >  	u64 dpa;
> > >  } mock_poison[MOCK_INJECT_POISON_MAX];
> > >  
> > > +struct mock_poison_po {
> > > +	struct cxl_mbox_poison_payload_out poison_out;
> > > +	struct cxl_poison_record record[MOCK_INJECT_POISON_MAX];  
> > 
> > Maybe use a variable length final element [] so that you can then
> > create a 1 record version on demand where it is used for
> > the default record.
> >   
> > > +};
> > > +
> > > +/*
> > > + * Use the default poison payload when no injected poison is currently
> > > + * mocked for this device. Mock one poison record with length 64 bytes.
> > > + */
> > > +static struct mock_poison_po default_poison_po = {
> > > +	.poison_out.count = cpu_to_le16(1),
> > > +	.record[0].length = cpu_to_le32(1),
> > > +};
> > > +
> > > +static struct mock_poison_po *cxl_get_injected_po(struct cxl_dev_state *cxlds,
> > > +						  u64 offset, u64 length)
> > > +{
> > > +	struct mock_poison_po *po;
> > > +	int nr_records = 0;
> > > +	u64 dpa;
> > > +
> > > +	po = kzalloc(sizeof(*po), GFP_KERNEL);
> > > +	if (!po)
> > > +		return NULL;
> > > +
> > > +	for (int i = 0; i < MOCK_INJECT_POISON_MAX; i++) {
> > > +		if (mock_poison[i].cxlds != cxlds)
> > > +			continue;
> > > +		if (mock_poison[i].dpa < offset ||
> > > +		    mock_poison[i].dpa > offset + length - 1)
> > > +			continue;
> > > +		dpa = cpu_to_le64(mock_poison[i].dpa |
> > > +				  CXL_POISON_SOURCE_INJECTED);
> > > +		po->record[nr_records].address = dpa;
> > > +		po->record[nr_records].length = cpu_to_le32(1);
> > > +		nr_records++;
> > > +	}
> > > +	if (!nr_records) {
> > > +		kfree(po);
> > > +		return NULL;
> > > +	}
> > > +	po->poison_out.count = cpu_to_le16(nr_records);
> > > +	return po;
> > > +}
> > > +
> > > +static int mock_get_poison(struct cxl_dev_state *cxlds,
> > > +			   struct cxl_mbox_cmd *cmd)
> > > +{  
> > 
> > Why the function move?  If you want to do this, a trivial move only
> > patch first would have bad it slightly easier to review. But given it's small
> > not worth doing now, particularly as most of the function changes anyway.
> >   
> 
> Let me see how this cleans up, including your feedback below, when 
> I get rid of the either injected or default choice.
> 
> >   
> > > +	struct cxl_mbox_poison_payload_in *pi = cmd->payload_in;
> > > +	struct mock_poison_po *po;
> > > +
> > > +	po = cxl_get_injected_po(cxlds, pi->offset, pi->length);
> > > +	if (!po) {
> > > +		po = &default_poison_po;  
> > 
> > Keep the comment from below on what this value is. It is weird enough
> > to be worth a comment.
> > 
> > I'd be tempted to make the default_poison_po const and use a copy
> > of it here to avoid fun races on reads from multiple devices.
> > Or just allocate a 1 element version and fill it here.  However,
> > I think we should really have both the default and injected elements.
> >   
> > > +		po->record[0].address = cpu_to_le64(pi->offset |
> > > +						    CXL_POISON_SOURCE_INTERNAL);
> > > +	}
> > > +	if (cmd->size_out < sizeof(po))
> > > +		return -EINVAL;  
> > 
> > This isn't technically correct, though it's probably fine for how we currently
> > use it. Should really assume full mailbox size and fill on that basis (record
> > count etc) but only copy the size of the target buffer.
> > Not sure we care enough about this being 'right' as opposed to useful.
> >   
> > > +
> > > +	memcpy(cmd->payload_out, po, sizeof(*po));
> > > +	cmd->size_out = sizeof(po);
> > > +
> > > +	if (po != &default_poison_po)
> > > +		kfree(po);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >  static bool mock_poison_add(struct cxl_dev_state *cxlds, u64 dpa)
> > >  {
> > >  	for (int i = 0; i < MOCK_INJECT_POISON_MAX; i++) {
> > > @@ -293,34 +362,6 @@ static int mock_inject_poison(struct cxl_dev_state *cxlds,
> > >  	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;
> > > -
> > > -	struct {
> > > -		struct cxl_mbox_poison_payload_out poison_out;
> > > -		struct cxl_poison_record record;
> > > -	} mock_poison_list = {
> > > -		.poison_out = {
> > > -			.count = cpu_to_le16(1),
> > > -		},
> > > -		/* Mock one poison record at pi.offset for 64 bytes */
> > > -		.record = {
> > > -			/* .address encodes DPA and poison source bits */
> > > -			.address = cpu_to_le64(pi->offset |
> > > -					       CXL_POISON_SOURCE_INTERNAL),
> > > -			.length = cpu_to_le32(1),
> > > -		},
> > > -	};
> > > -
> > > -	if (cmd->size_out < sizeof(mock_poison_list))
> > > -		return -EINVAL;
> > > -
> > > -	memcpy(cmd->payload_out, &mock_poison_list, sizeof(mock_poison_list));
> > > -	cmd->size_out = sizeof(mock_poison_list);
> > > -	return 0;
> > > -}
> > > -
> > >  static int cxl_mock_mbox_send(struct cxl_dev_state *cxlds, struct cxl_mbox_cmd *cmd)
> > >  {
> > >  	struct device *dev = cxlds->dev;  
> >