Received-SPF: pass (google.com: domain of linux-kernel+bounces-19817-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Mon, 8 Jan 2024 08:06:32 -0800
From: Ira Weiny <ira.weiny@intel.com>
To: Jonathan Cameron <Jonathan.Cameron@huawei.com>, Ira Weiny
	<ira.weiny@intel.com>
CC: fan <nifan.cxl@gmail.com>, Dave Jiang <dave.jiang@intel.com>,
	<linux-cxl@vger.kernel.org>, <linux-kernel@vger.kernel.org>, Huai-Cheng Kuo
	<hchkuo@avery-design.com.tw>
Subject: Re: [PATCH v2 1/2] cxl/cdat: Handle cdat table build errors
Message-ID: <659c1d888a5fc_8737f29495@iweiny-mobl.notmuch>
References: <20231117-fix-cdat-cs-v2-0-715399976d4d@intel.com>
 <20231117-fix-cdat-cs-v2-1-715399976d4d@intel.com>
 <ZYHWZjsnQwcBEDpd@debian>
 <658346b553a01_34d57294d4@iweiny-mobl.notmuch>
 <20240108150318.00006eba@Huawei.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20240108150318.00006eba@Huawei.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?UWUbbJm2LctzpnCw9fNhmOafqgySorE7PD1zszdZ+NqlpUBnpKVFgOK6pQDi?=
 =?us-ascii?Q?uWC+s9FW9ZY2dtSi1Rpl4NKj4kOGTqDB3nBcSO1GqX4+l5IBSNzQSUid4thx?=
 =?us-ascii?Q?cAUEghe7thoW1CfrKc9Rx6BN+09V1J7C4ZEw/YA34Oejbrr0hV0xSDk6G8Gz?=
 =?us-ascii?Q?R+ftD7Kmpb61I8OF5ZJnTapBdXdIcWYV+dQF4xWbcfoVqzt1O/J5ereGtrVi?=
 =?us-ascii?Q?Gi6UbWo4kJUMjTLnELXQygBVDTBAI5tKVlp2QoQEclCUIwafBZdFejYxXTTO?=
 =?us-ascii?Q?yeFe2froZezxLpL6YSmfqYW9fhXl3+9/me4I4f3vinBMWgI8Bkw9GQQvu4m4?=
 =?us-ascii?Q?A3Cm0ivzX/dFnFApY/EejC4xwuPhpNe7ypxeFK1SyIvBONiD25Uyr2wtzz68?=
 =?us-ascii?Q?bafZISETijP8wPO0OGNou75/dQBPLHknlYURxjKeWmsApjHZFVVjJjNUiFHK?=
 =?us-ascii?Q?SWoaOptuhgNOc6o9iAreBpXZ3bwwWUA3Qi+KD+08O4byM0jKA6/T0bEpHjOW?=
 =?us-ascii?Q?m+PRiuyyFBLUJFEMZczBMoDdVO3+xvPaV4+mDrAGMH44mAWOhRYEMlFHPGhL?=
 =?us-ascii?Q?8sf+0xYSsy0kMkiUthOczOBy+g6H+n6KNO69B3Yp4LZTXuU0woqVNbFSQqWn?=
 =?us-ascii?Q?xp+OYKNEniSl7abFcFhDPNSdqkPE3tKEUKMm4JBU5jDK4BVZztODDtXwwREX?=
 =?us-ascii?Q?EihhKT/xPAaumRKgEtlARBudO9T49PzIZEVq4HqHDke0qmJz64QdVaQuBdjx?=
 =?us-ascii?Q?jlcw9sRjHCrA+fz1+3eFKn22Kq6WlSspETd/Z0cp6NqafC8iiJXWvbcahJkF?=
 =?us-ascii?Q?f95RZaDtMQ9b3jHunGcYqYfojH30rIP/JRj8Y27Gtv1aHx4fb1F3wS0Mfy2M?=
 =?us-ascii?Q?AnKTvCeYs7jTXxDY5gXfmzBm50GDE63uDDK6pjESFp1RVTNG73bzw0Gwc59M?=
 =?us-ascii?Q?17xUM4f6u3NbCa9zUHTx+atxvQyIjarUVxRJgkMEIChO6t6Db+I2KrKnq+eZ?=
 =?us-ascii?Q?eRLkPS2SrtgT1/Gm7FGLFq7n4fi/kbUEArNiarM+t4S0+G8/TIBfys+bJs2n?=
 =?us-ascii?Q?1swtrr42gq45zbJu3oNhnRKQRaVil3F/Na/0DhsrJV5T4/Nzj9o2zpV3luLn?=
 =?us-ascii?Q?7gkFZ9bWabgdccRFX85c+bSKDILp3RgX0bGTYP+o+y14yduYyMBHMVlxhGuX?=
 =?us-ascii?Q?vacOzPHx4xNyrNp0uraAJ2eMgynbrnlljNKexocA6knKNHqI4mhCBFVRIqMX?=
 =?us-ascii?Q?MgfPz/CeKYEYyha3ZhWwOgpvQK2N1y9Tc0h2zEYNn6+bR0o6ufrdaSmbGgzy?=
 =?us-ascii?Q?ywOWT+DkKOzEhVH8ttmbwBxdtXTDLSzax68rIQEjHQ1SsRr3RsDioU+38aLc?=
 =?us-ascii?Q?LHn9sKA07mkGwqXTywBeVas3me4Qo2H5MgXlrpE4bKeUqwvd8P0p2SoC3Yjh?=
 =?us-ascii?Q?Bx0vcRTFmnlqnOt7/NXSka3Pm8Uxg7tHJ9urIIKK3p/X1nRJUCmDhy5Ff6ob?=
 =?us-ascii?Q?PphZzlJkKPtX7TOL/Zz84oYamQ+hzYswiSzhhMfV0LlzEgOVxM7kFt6BlEZz?=
 =?us-ascii?Q?xRa1FhN6aLFS7t+/hpq4FG7oO+LLKcFwJfUYeEGd?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 2dcacd78-ec27-4353-c07b-08dc1063c99e
X-MS-Exchange-CrossTenant-AuthSource: SA1PR11MB6733.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 08 Jan 2024 16:06:35.3106
 (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: 0aFwXFDs/AYsSS4jpTWmFDBWZPmrZL/4sfYb3jnz3HzgNnGJ8JPrc8s1e6CQRB8xTjGwKAQPIA2g3fEQ445dZw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA0PR11MB4671
X-OriginatorOrg: intel.com

Jonathan Cameron wrote:
> On Wed, 20 Dec 2023 11:55:33 -0800
> Ira Weiny <ira.weiny@intel.com> wrote:
> 
> > fan wrote:
> > > On Wed, Nov 29, 2023 at 05:33:03PM -0800, Ira Weiny wrote:  
> > > > The callback for building CDAT tables may return negative error codes.
> > > > This was previously unhandled and will result in potentially huge
> > > > allocations later on in ct3_build_cdat()
> > > > 
> > > > Detect the negative error code and defer cdat building.
> > > > 
> > > > Fixes: f5ee7413d592 ("hw/mem/cxl-type3: Add CXL CDAT Data Object Exchange")
> > > > Cc: Huai-Cheng Kuo <hchkuo@avery-design.com.tw>
> > > > Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> > > > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> > > > ---
> > > >  hw/cxl/cxl-cdat.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/hw/cxl/cxl-cdat.c b/hw/cxl/cxl-cdat.c
> > > > index 639a2db3e17b..24829cf2428d 100644
> > > > --- a/hw/cxl/cxl-cdat.c
> > > > +++ b/hw/cxl/cxl-cdat.c
> > > > @@ -63,7 +63,7 @@ static void ct3_build_cdat(CDATObject *cdat, Error **errp)
> > > >      cdat->built_buf_len = cdat->build_cdat_table(&cdat->built_buf,
> > > >                                                   cdat->private);
> > > >  
> > > > -    if (!cdat->built_buf_len) {
> > > > +    if (cdat->built_buf_len <= 0) {
> > > >          /* Build later as not all data available yet */
> > > >          cdat->to_update = true;
> > > >          return;
> > > >   
> > > 
> > > The fix looks good to me. Just curious how to really build cdat table
> > > again when an error occurs, for example, the memory allocation fails.  
> > 
> > I did not go that far as I am unsure as well.
> Memory allocations in qemu don't fail (well if they do it crashes)
> Side effect of using glib which makes for simpler cases.
> https://docs.gtk.org/glib/func.malloc.html
> 
> There shouldn't even be any checks :(  I'll fix that up at somepoint
> across all the CXL emulation.  Sometimes reviewers noticed and
> we dropped it at earlier stages, but clearly didn't catch them all.
> 
> Which come to think of it is why this error condition is in practice
> not actually buggy as the code won't ever manage to return -ENOMEM and
> I don't think there are other error codes.

Ah.  Ok but in that case I would say that build_cdat_table() should never
return < 0 to be clear at this level what can happen.

Would you like a patch for that?  (/me assumes you dropped this patch)

Ira