Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 17 Jun 2022 12:32:28 -0700
From:   Dan Williams <dan.j.williams@intel.com>
To:     Alison Schofield <alison.schofield@intel.com>,
        Jonathan Cameron <Jonathan.Cameron@huawei.com>
CC:     "Williams, Dan J" <dan.j.williams@intel.com>,
        "Weiny, Ira" <ira.weiny@intel.com>,
        "Verma, Vishal L" <vishal.l.verma@intel.com>,
        "Ben Widawsky" <bwidawsk@kernel.org>,
        Steven Rostedt <rostedt@goodmis.org>,
        "Ingo Molnar" <mingo@redhat.com>,
        "linux-cxl@vger.kernel.org" <linux-cxl@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 2/3] cxl/mbox: Add GET_POISON_LIST mailbox command support
Message-ID: <62acd6cc72637_844b129463@dwillia2-xfh.notmuch>
References: <cover.1655250669.git.alison.schofield@intel.com>
 <382a9c35ef43e89db85670637d88371f9197b7a2.1655250669.git.alison.schofield@intel.com>
 <20220617150508.0000266a@Huawei.com>
 <20220617162935.GA1532720@alison-desk>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20220617162935.GA1532720@alison-desk>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?N2Cdqrcwzner7Ld3JMvYY8XowxYiGhuW2j+g6P0CVO4rZadsCqVOotnkM/Be?=
 =?us-ascii?Q?TgPXAt9nUT9Qm/xKCwxG+Qh8C12IXVa+gexhFz194zf0CucQT5xZZTDrG4iW?=
 =?us-ascii?Q?/lTYiUF9Ow+AEJMhYY8i6z9SeozkyuG4IZOtK9bxrSqzSbg7yqXW+Ibbp1bV?=
 =?us-ascii?Q?I6yqJ6C8yYJjNpwy8xU0Sny0C2c0GmlJk0HB4iPObHRn3SE47A5ESrHLv7lq?=
 =?us-ascii?Q?HXAFqYqYrZJmeCYVw5dXtFimx0FP/jvom94PGlcii1DH3YNRGZbhOJSY8vA1?=
 =?us-ascii?Q?JsMzQVZggGgPLePnAAnRTMpfCwNLZheYXJqXJNkwgGUWdj75JowCH9qEAFgr?=
 =?us-ascii?Q?qLdpi5JMeENOlXHZxEa6q9bCKc+CTN0oxNJS4flMellsIMSShRmVKfCz3vt0?=
 =?us-ascii?Q?Ak7aV0xhDKc6AmzINFkm9z1zy2/D/5Yl9+lXO3cVzmlrUwq5ZwFU2jV4YFfS?=
 =?us-ascii?Q?VTgFxIlmW9qQixyJae1uP+EP8EUQ5o1q+sZpDpIfEpF37b29Edwcfc6V/6PZ?=
 =?us-ascii?Q?IpNxT8kWZ01cuF93iJoOzIR33y5Av9cLizGQpBhlrz0PotZ8NT1l2loyS1T9?=
 =?us-ascii?Q?J/9yo+2sGU4NxJyWtFCBkDn+H969LesUs56jWIA7cunsdq8dJcIKB0KK09LJ?=
 =?us-ascii?Q?gcWJ3sSnUpL7WUpyQEbjTTWD8afYe01MUNMP6b5fRWQN/icDK4Thgvgwxpto?=
 =?us-ascii?Q?a19NtIdfdSzJilipQ5zWul5FkmHnbV9MEvRkXwLkWQjSwDNVaR1gHBF6p//B?=
 =?us-ascii?Q?h41OWV9KUxWp9ETelLBdRgQct5nWONCJ3IoKDbnFUMAJ+cVgMwGxYIOXR+Fy?=
 =?us-ascii?Q?hVj5c+eC9zvn91fGGDjSCxgshgo3gjXtJ+vM4xtWmayn0Bp8lfgKfyRLz+rD?=
 =?us-ascii?Q?NJi3Q0IUE43qmxaPlneE+GCDsW9tUWLMuvO7QOdDPPaIjnhElDzwIRnjOUz4?=
 =?us-ascii?Q?aaUIrcCIELkTq2d+wipMKinPEuJTcFfEJQ7zEKZb8O4vnyNPmHOMvk91wx+/?=
 =?us-ascii?Q?BmMkfPA6QjhciDvRsYhzzdRopUUFIsFPECkzDkTcCOZXZgOqBj6ipffPxQdS?=
 =?us-ascii?Q?1jB3yl+nCcpRaCFjvF2cSC83V7i+w4mPCaYjAYg6WBR/9zHw6d4Qx6JaIhC2?=
 =?us-ascii?Q?138WQtAuHZNCz9NUUJuGxNUbYqLn3DND294/lxccTyUcE3FUJ6gedWcz+sSK?=
 =?us-ascii?Q?DdaNLuim4ZPIpt42DS9bj4C7a6Wf6HjOtyeu6AnjvcTseFTY9UB6hOkX2fmx?=
 =?us-ascii?Q?FyfAFekEuEdAB5iefQDovly0qX4JYY3XdMGWKND83Jc7m76HpcXO3GSP9xOj?=
 =?us-ascii?Q?73KDMNQVw9VBUqq1yfiqEjOFEir57VUiNfdrDq1CNJjSF5Haj1AocTFxp2xz?=
 =?us-ascii?Q?nts2A0G5NuH076mRg4tNpO/cCta+q73++paBKDtWQTrku0LXXN5UxE8ZTrlR?=
 =?us-ascii?Q?xEHlHxpn8KDtxiOUPaM/ApDZB6obQt4J/VCLFeHW93zPanz/+GNDW9DxrFAU?=
 =?us-ascii?Q?AnCKilJQd9qUAr6AU8TaSAcaIm9S1amnxk7w8S912UE+d9pXPj+uwGjGnlXh?=
 =?us-ascii?Q?c8dWm0k99SlujOx4vtrFDTrxKYe5mFVHDYr4efCCaH2KwYy/TyfFnyBCRO+9?=
 =?us-ascii?Q?16FhrH3xJGu+98eMRJgKUkvrzVnCmwQ6IEbC82zfS5dzTFIiz+9UvhQumeWM?=
 =?us-ascii?Q?9KojM1W6ArIu0nbtEaT6XlUlPawlxwMlait/bG/WMSuMTb4PuHxREr5kOLKM?=
 =?us-ascii?Q?62KBo7+KbnemVbPP0pPh0hNPCtF2t74=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 6e5f4d13-27dd-4f75-f572-08da50981e3d
X-MS-Exchange-CrossTenant-AuthSource: MWHPR1101MB2126.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 17 Jun 2022 19:32:30.0224
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: cdclxWAlWAfSIANMn/qEo+H4VP/mcpbQCYp/gX84+/jta4grupSW9blo82W4+QMxznXlRekXJ+RtT2qsYlVJuKjbo+tBtMKCXyZSe2aWdEA=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN9PR11MB5289
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-5.0 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,
        SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Alison Schofield wrote:
> On Fri, Jun 17, 2022 at 07:05:08AM -0700, Jonathan Cameron wrote:
> > On Tue, 14 Jun 2022 17:10:27 -0700
> > alison.schofield@intel.com wrote:
> > 
> > > From: Alison Schofield <alison.schofield@intel.com>
> > > 
> > > CXL devices that support persistent memory maintain a list of locations
> > > that are poisoned or result in poison if the addresses are accessed by
> > > the host.
> > > 
> > > Per the spec (CXL 2.0 8.2.8.5.4.1), the device returns this Poison
> > > list as a set of  Media Error Records that include the source of the
> > > error, the starting device physical address and length. The length is
> > > the number of adjacent DPAs in the record and is in units of 64 bytes.
> > > 
> > > Retrieve the list and log each Media Error Record as a trace event of
> > > type cxl_poison_list.
> > > 
> > > Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> > 
> > A few more things inline.
> > 
> > Otherwise, can confirm it works with some hack QEMU code.
> > I'll tidy that up and post soon.
> > 
> > > +int cxl_mem_get_poison_list(struct device *dev)
> > > +{
> snip
> > > +
> > > +			trace_cxl_poison_list(dev, source, addr, len);
> > 
> > Need to mask off the lower 6 bits of addr as they contain the source
> > + a few reserved bits.
> > 
> > I was confused how you were geting better than 64 byte precision in your
> > example.
> >
> Ah...got it. Thanks!
> 
> > > +		}
> > > +
> > > +		/* Protect against an uncleared _FLAG_MORE */
> > > +		nr_records = nr_records + le16_to_cpu(po->count);
> > > +		if (nr_records >= cxlds->poison_max)
> > > +			goto out;
> > > +
> > > +	} while (po->flags & CXL_POISON_FLAG_MORE);
> > So.. A conundrum here.  What happens if:
> > 
> > 1. We get an error mid way through a set of multiple reads
> >    (something intermittent - maybe a software issue)
> > 2. We will drop out of here fine and report the error.
> > 3. We run this function again.
> > 
> > It will (I think) currently pick up where we left off, but we have
> > no way of knowing that as there isn't a 'total records' count or
> > any other form of index in the output payload.
> 
> Yes. That is sad. I'm assume it's by design and CXL devices never
> intended to keep any totals.
> 
> > 
> > So, software solutions I think should work (though may warrant a note
> > to be added to the spec).
> > 
> > 1. Read whole thing twice. First time is just to ensure we get
> >    to the end and flush out any prior half done reads.
> > 2. Issue a read for a different region (perhaps length 0) first
> >    and assume everything starts from scratch when we go back to
> >    this region.
> 
> Can you tell me more about 2 ?
> 
> Also, Since posting this I have added protection to this path to ensure
> only one reader of the poison list for this device. Like this:
> 
> if (!completion_done(&cxlds->read_poison_complete);
>               return -EBUSY;
> wait_for_completion_interruptible(&cxlds->read_poison_complete);
> 	...GET ALL THE POISON...
> complete(&cxlds->read_poison_complete);

Since this runs in the context of the requester a completion feels out
of place. What this probably wants is a mutex() protecting the state
machine of the Media Error Record retrieval and the "more" flag.

> 
> And will add the error message on that unexpected _FLAG_MORE too.
> 
> Alison
> > 
> > Jonathan
> > 
> 
> 
> 
> > > +
> > > +out:
> > > +	kvfree(po);
> > > +	return rc;
> > > +}
> > > +EXPORT_SYMBOL_NS_GPL(cxl_mem_get_poison_list, CXL);
> > > +
> > >  struct cxl_dev_state *cxl_dev_state_create(struct device *dev)
> > >  {
> > >  	struct cxl_dev_state *cxlds;
> >