Date:   Fri, 3 Mar 2023 07:57:22 -0800
From:   Ira Weiny <ira.weiny@intel.com>
To:     Jonathan Cameron <Jonathan.Cameron@huawei.com>,
        Dave Jiang <dave.jiang@intel.com>
CC:     Fan Ni <fan.ni@samsung.com>,
        "alison.schofield@intel.com" <alison.schofield@intel.com>,
        "vishal.l.verma@intel.com" <vishal.l.verma@intel.com>,
        "ira.weiny@intel.com" <ira.weiny@intel.com>,
        "bwidawsk@kernel.org" <bwidawsk@kernel.org>,
        "dan.j.williams@intel.com" <dan.j.williams@intel.com>,
        "linux-cxl@vger.kernel.org" <linux-cxl@vger.kernel.org>,
        Adam Manzanares <a.manzanares@samsung.com>,
        "dave@stgolabs.net" <dave@stgolabs.net>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] cxl/hdm: Fix hdm decoder init by adding COMMIT field
 check
Message-ID: <640218e217c80_5a3fc2947@iweiny-mobl.notmuch>
References: <CGME20230228224029uscas1p1e2fb92a8a595f80fa2985b452899d785@uscas1p1.samsung.com>
 <20230228224014.1402545-1-fan.ni@samsung.com>
 <346a8225-609e-0188-ec8a-4abe8d271a09@intel.com>
 <20230302062305.GA1444578@bgt-140510-bm03>
 <e10b60ab-f666-8124-eb8f-6a2da6c0f989@intel.com>
 <20230303143605.0000159a@Huawei.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20230303143605.0000159a@Huawei.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?CqedQLEiVZ9ne+Q0Krxauk1qAci1zC6qXvmKbscrNJgPqk3gT2JWBLmpW7TH?=
 =?us-ascii?Q?iOT1XO8LNacqSNspqoDoHKiRfGsgWemhwFhvIf0n5IlA8yGoGQpxm5R62FX4?=
 =?us-ascii?Q?Ilx3Lp1j/VHqLLNEK3oqL0gJcrc3hW1Z+qglXLVKWJlOsam5gqT9LRE7e4e/?=
 =?us-ascii?Q?Em4ovw1EP82PsL1xQ1SA915D3G2RWyWTHYHYg4CrQGg3DgU18q6VWmu8A1F5?=
 =?us-ascii?Q?vF26DAj1Xh3eMHRiUsPz91eEzbAOR9ML7kA9BYCEgaxaXsgvk3bOQWKfe8ew?=
 =?us-ascii?Q?6DPjqOv3mV4/llb+Sn8OxY5ZbgXNwPzN2yie1jmWjTxu0xOTEqyqt5ZqF3KA?=
 =?us-ascii?Q?4e8V6vVa/t2K7qYQ+ol+cHyXI4dyaZPqYtAEKB/uGjtUrMgH8hG6OlM90+oh?=
 =?us-ascii?Q?e0nyOjjeXNBd/z9wHF5GTZ5cr+urZbrXlcu8Mn7JLYR9CE6tMayanmHlSiC7?=
 =?us-ascii?Q?dAdlYgglAnRMhgvvsEpMPtaqlGxcreCqTHmZhs0TUDXFY5n2CV/N1sVHoEEe?=
 =?us-ascii?Q?wnth7Wtacp1iOZTijv+YerIxxCBzBu8CBIR3HhyhmOBvNiGuEIXP+V0c95sn?=
 =?us-ascii?Q?SqANtj1y4MzHi/HYGB+TDrmptl8LffQDE52gULwHxNS0RqboqztaulP5dUP9?=
 =?us-ascii?Q?JguAErQGC9kRKwBKGQGIUPyCLKeva3qD+3LKiRpiU8JomtB1X+EOTmjj6oMF?=
 =?us-ascii?Q?sD0iJpRya6qOwNrpbR2tHqJkgnOGQ2HreMy9lyop0IVAQ8BEY2ldtV3+/rbD?=
 =?us-ascii?Q?GDv+WiBvvPobBE1Jjm1PFi6p9/v5xNKapgh7E0l8Kdcn3XSjSRT+H20xbZxK?=
 =?us-ascii?Q?fI5aizGbg2naP4nhySuYVDD1K+EUNpJUoKw6H6dFParEBhwIZ0fmDLILce0W?=
 =?us-ascii?Q?vN6v3lrRdGj2zol1dOoSfFgcwQuQg+fVPZPvItIfsU4DOdD7uLigvdWDbk8X?=
 =?us-ascii?Q?Pt+5Z5SjudnZLDOAdajBPKKKX4lv5sNG4WgIm8P/gu/4FW3aAczRzmCDEPN+?=
 =?us-ascii?Q?qNJfMzGUMklNVkuJHag5p2djKX+6SBWaGzEFCLjX8rA90qFq8MH1RRKLFM4h?=
 =?us-ascii?Q?pJzSZ68AKt52LZNsl67UVrXZKjgJ6odsNMabo9g/RzNPMd8+yuiaVUCTkJcJ?=
 =?us-ascii?Q?ptNCLABVkb6c3h2JJYfVOFB0IWanpY59UZkVU8RB2e1kz6tRgeto3WOCVYYW?=
 =?us-ascii?Q?2HsIYFJ4J8i30oYJhb2H4CswCLCbw0xc42hbBmX1zKcgH0iIyQfsxHHicy9i?=
 =?us-ascii?Q?LOeIEfy3KYZuNAFtEjWUpStw+2xcVwg5S6mDX5hCjuKVr+vbEiYCG9pLdlbv?=
 =?us-ascii?Q?VruvXIZTeci83b77UgqqCZnNi9bHL1+eAEejMqK1b/wCcKcBM99Q9O7Qlmia?=
 =?us-ascii?Q?Dz3v9mMtMcFaaTRo1Gdxnm7rGE4w+W+6pk6UHe8+coF6UXmCoaxy+EqCrps5?=
 =?us-ascii?Q?QhDp9hOQ85DjJKu5k6Bb+//L448L6hkiFTsowuYZQstH71sC9Xk4/h33yx9o?=
 =?us-ascii?Q?Vp9zZjIk1y1jVzb9dVL41AC/qFSsVzd6gWBsOrMnhUP0PFbyvY42ndyaTU1N?=
 =?us-ascii?Q?kiFTIILYrh1YuInIUrQ09sfakS+llgOYCXS+LOt9?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 7d547523-b65a-4184-7e84-08db1bfffbd1
X-MS-Exchange-CrossTenant-AuthSource: SA1PR11MB6733.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 03 Mar 2023 15:57:26.1374
 (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: Hzw7aM4yGJHsIva4f0zK24WlIsHoY1fYKfnnUW+o4Wtm54ZHDTY2gC3Z3GA577Oo7zymvW3541Z9Q0anVP4sfA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN0PR11MB5988
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Jonathan Cameron wrote:
> On Thu, 2 Mar 2023 08:36:59 -0700
> Dave Jiang <dave.jiang@intel.com> wrote:
> 
> > On 3/1/23 11:23 PM, Fan Ni wrote:
> > > On Wed, Mar 01, 2023 at 11:54:08AM -0700, Dave Jiang wrote:  
> > >>  
> > > Hi Dave,
> > > Thanks for looking into this.  
> > >>
> > >> On 2/28/23 3:40 PM, Fan Ni wrote:  
> > >>> Add COMMIT field check aside with existing COMMITTED field check during
> > >>> hdm decoder initialization to avoid a system crash during module removal
> > >>> after destroying a region which leaves the COMMIT field being reset while
> > >>> the COMMITTED field still being set.  
> > >>
> > >> Hi Fan. Are you seeing this issue on qemu emulation or hardware? The  
> > > I run into the issue with qemu emulation.  
> > >> situation does not make sense to me. If we clear the COMMIT bit, then the
> > >> COMMITTED bit should be cleared by the hardware shortly after right?  
> > > 
> > >  From the spec, I cannot find any statement saying clearing the COMMIT bit
> > > will automatically clear the COMMITTED. If I have not missed the statement in
> > > the spec, I assume we should not make the assumption that it will be
> > > cleared automatically for real hardware. But you may be right, leaving the
> > > COMMITTED bit set can potentially cause some issue? Need to check more.  
> > 
> > I have not been able to find direct verbiage that indicates this either. 
> > However, logically it would make sense. Otherwise, the COMMITTED field 
> > never clears and prevents reprogramming of the HDM decoders. The current 
> > QEMU implementation is creating a situation where the HDM decoder is 
> > always active after COMMIT bit is set the first time, regardless whether 
> > COMMIT field has been cleared later on during a teardown. It does sound 
> > like a bug with QEMU emulation currently.
> 
> I agree that one sane interpretation is that unsetting commit should result in
> the decoder being deactivated and hence the commit bit dropping.  However
> I'm not sure that's the only sane interpretation.
> 
> There is no verbage that I'm aware of that says the committed bit being
> set means that the current register values are in use.  It simply says that
> when the commit bit was set, the HDM decoder was successfully committed
> (using registers as set at that time).  There is a specific statement about
> not changing the registers whilst checks are in progress, but those checks
> are only required if lock on commit is set, so it doesn't cover this case.
> 
> Wonderfully there isn't actually anything says what a commit transition to 0
> means.  Does that result in the decoder become uncommitted, or does that only
> happen when the next 0 to 1 transition happens?
> 
> The only stuff we have is what happens when lock on commit = 1, which isn't
> the case here.
> 
> So is there another valid implementation? I think yes.
> In some implementations, there will be a complex state machine that is
> triggered when commit is set.  That will then write some entirely invisible
> internal state for decode logic based on the contents of the registers.
> As such, once it's set committed, it typically won't look at the registers
> again until another commit 0->1 transition happens.
> At that point the
> committed bit drops and raised again once the commit state machine finishes
> (given QEMU doesn't emulate that delay the upshot is if you set commit then
> check committed it will be set ;)

I'm only barely following along so I wanted to make sure I understand...

Are you saying that at the instant commit 0->1 happens hardware will clear
commited to 0 so that software can later check for commited vs error not
commited?

Ira

> 
> In that implementation the commit 1->0 transition is an irrelevance and
> it won't change the committed bit state.
> 
> So whilst the QEMU code is doing the less obvious implementation, I think
> the spec still allows it.  I don't mind QEMU changing to the more obvious
> one though if someone wants to send a patch.
> 
> Jonathan
> 

[...]