From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from frasgout.his.huawei.com (frasgout.his.huawei.com [185.176.79.56]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 6329D5467C; Mon, 8 Jan 2024 18:05:57 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=Huawei.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=huawei.com Received: from mail.maildlp.com (unknown [172.18.186.31]) by frasgout.his.huawei.com (SkyGuard) with ESMTP id 4T824q0j0dz6K5kj; Tue, 9 Jan 2024 02:04:11 +0800 (CST) Received: from lhrpeml500005.china.huawei.com (unknown [7.191.163.240]) by mail.maildlp.com (Postfix) with ESMTPS id E2FD61400D9; Tue, 9 Jan 2024 02:05:54 +0800 (CST) Received: from localhost (10.202.227.76) by lhrpeml500005.china.huawei.com (7.191.163.240) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.35; Mon, 8 Jan 2024 18:05:54 +0000 Date: Mon, 8 Jan 2024 18:05:53 +0000 From: Jonathan Cameron To: Ira Weiny CC: fan , Dave Jiang , , , Huai-Cheng Kuo Subject: Re: [PATCH v2 1/2] cxl/cdat: Handle cdat table build errors Message-ID: <20240108180553.00001577@Huawei.com> In-Reply-To: <20240108180042.00005443@Huawei.com> References: <20231117-fix-cdat-cs-v2-0-715399976d4d@intel.com> <20231117-fix-cdat-cs-v2-1-715399976d4d@intel.com> <658346b553a01_34d57294d4@iweiny-mobl.notmuch> <20240108150318.00006eba@Huawei.com> <659c1d888a5fc_8737f29495@iweiny-mobl.notmuch> <20240108180042.00005443@Huawei.com> Organization: Huawei Technologies Research and Development (UK) Ltd. X-Mailer: Claws Mail 4.1.0 (GTK 3.24.33; x86_64-w64-mingw32) Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset="US-ASCII" Content-Transfer-Encoding: 7bit X-ClientProxiedBy: lhrpeml100005.china.huawei.com (7.191.160.25) To lhrpeml500005.china.huawei.com (7.191.163.240) On Mon, 8 Jan 2024 18:00:42 +0000 Jonathan Cameron wrote: > On Mon, 8 Jan 2024 08:06:32 -0800 > Ira Weiny wrote: > > > Jonathan Cameron wrote: > > > On Wed, 20 Dec 2023 11:55:33 -0800 > > > Ira Weiny 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 > > > > > > Reviewed-by: Dave Jiang > > > > > > Signed-off-by: Ira Weiny > > > > > > --- > > > > > > 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) > > Probably needs to first rip out all the -ENOMEM returns that got into > the CXL code in general, then tidy up the return type to be unsigned. > > If you want to do that it would be welcome! Actually. Build_cdat_table() can return errors just not for this reason. host_memory_backend_get_memory() can fail for example. So original patch is good as is, just that the discussion of memory allocation failure threw me off and should be cleaned up separately. Jonathan > > Jonathan > > > > > > Ira > > >