From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 165E2C46CD2 for ; Wed, 24 Jan 2024 15:24:30 +0000 (UTC) Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1rSf69-0000Xn-4B; Wed, 24 Jan 2024 10:23:30 -0500 Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1rSf67-0000Xe-07 for qemu-devel@nongnu.org; Wed, 24 Jan 2024 10:23:27 -0500 Received: from frasgout.his.huawei.com ([185.176.79.56]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1rSf63-0005cQ-VS for qemu-devel@nongnu.org; Wed, 24 Jan 2024 10:23:26 -0500 Received: from mail.maildlp.com (unknown [172.18.186.216]) by frasgout.his.huawei.com (SkyGuard) with ESMTP id 4TKnhL0xfFz6J9jy; Wed, 24 Jan 2024 23:20:18 +0800 (CST) Received: from lhrpeml500005.china.huawei.com (unknown [7.191.163.240]) by mail.maildlp.com (Postfix) with ESMTPS id 59A84140D1A; Wed, 24 Jan 2024 23:23:18 +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; Wed, 24 Jan 2024 15:23:17 +0000 Date: Wed, 24 Jan 2024 15:23:16 +0000 To: CC: , , , , , , , , , "Fan Ni" Subject: Re: [PATCH v3 4/9] hw/mem/cxl_type3: Add support to create DC regions to type3 memory devices Message-ID: <20240124152316.0000281c@Huawei.com> In-Reply-To: <20231107180907.553451-5-nifan.cxl@gmail.com> References: <20231107180907.553451-1-nifan.cxl@gmail.com> <20231107180907.553451-5-nifan.cxl@gmail.com> Organization: Huawei Technologies Research and Development (UK) Ltd. X-Mailer: Claws Mail 4.1.0 (GTK 3.24.33; x86_64-w64-mingw32) MIME-Version: 1.0 Content-Type: text/plain; charset="US-ASCII" Content-Transfer-Encoding: 7bit X-Originating-IP: [10.202.227.76] X-ClientProxiedBy: lhrpeml500004.china.huawei.com (7.191.163.9) To lhrpeml500005.china.huawei.com (7.191.163.240) Received-SPF: pass client-ip=185.176.79.56; envelope-from=jonathan.cameron@huawei.com; helo=frasgout.his.huawei.com X-Spam_score_int: -41 X-Spam_score: -4.2 X-Spam_bar: ---- X-Spam_report: (-4.2 / 5.0 requ) BAYES_00=-1.9, RCVD_IN_DNSWL_MED=-2.3, RCVD_IN_MSPIKE_H5=0.001, RCVD_IN_MSPIKE_WL=0.001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, T_SCC_BODY_TEXT_LINE=-0.01 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-to: Jonathan Cameron From: Jonathan Cameron via Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org On Tue, 7 Nov 2023 10:07:08 -0800 nifan.cxl@gmail.com wrote: > From: Fan Ni > > With the change, when setting up memory for type3 memory device, we can > create DC regions. > A property 'num-dc-regions' is added to ct3_props to allow users to pass the > number of DC regions to create. To make it easier, other region parameters > like region base, length, and block size are hard coded. If needed, > these parameters can be added easily. > > With the change, we can create DC regions with proper kernel side > support as below: > > region=$(cat /sys/bus/cxl/devices/decoder0.0/create_dc_region) > echo $region> /sys/bus/cxl/devices/decoder0.0/create_dc_region > echo 256 > /sys/bus/cxl/devices/$region/interleave_granularity > echo 1 > /sys/bus/cxl/devices/$region/interleave_ways > > echo "dc0" >/sys/bus/cxl/devices/decoder2.0/mode > echo 0x40000000 >/sys/bus/cxl/devices/decoder2.0/dpa_size > > echo 0x40000000 > /sys/bus/cxl/devices/$region/size > echo "decoder2.0" > /sys/bus/cxl/devices/$region/target0 > echo 1 > /sys/bus/cxl/devices/$region/commit > echo $region > /sys/bus/cxl/drivers/cxl_region/bind > > Signed-off-by: Fan Ni Hi Fan, a few comments inline. Jonathan > --- > hw/mem/cxl_type3.c | 35 +++++++++++++++++++++++++++++++++++ > 1 file changed, 35 insertions(+) > > diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c > index 754c885cd1..2d67d2015c 100644 > --- a/hw/mem/cxl_type3.c > +++ b/hw/mem/cxl_type3.c > @@ -721,6 +721,36 @@ static void ct3d_reg_write(void *opaque, hwaddr offset, uint64_t value, > } > } > > +static int cxl_create_dc_regions(CXLType3Dev *ct3d) > +{ > + int i; > + uint64_t region_base = 0; > + uint64_t region_len = 2 * GiB; > + uint64_t decode_len = 8; /* 8*256MB */ If decode len is going to be div 256MiB then we need a name for that field that makes it clear that it is. decode_len_256mbytes or something like that and maybe region_len_bytes to keep things consistent. Why the spec didn't make our life easier and define decode length in bytes with some bits that must be zero is beyond me... I think we need to make this at least optionally configurable or based in some fashion on the provided memory backend (divide that up by number of regions with appropriate rounding perhaps?) > + uint64_t blk_size = 2 * MiB; > + CXLDCDRegion *region; > + > + if (ct3d->hostvmem) { > + region_base += ct3d->hostvmem->size; > + } > + if (ct3d->hostpmem) { > + region_base += ct3d->hostpmem->size; > + } > + for (i = 0; i < ct3d->dc.num_regions; i++) { > + region = &ct3d->dc.regions[i]; > + region->base = region_base; > + region->decode_len = decode_len; > + region->len = region_len; > + region->block_size = blk_size; > + /* dsmad_handle is set when creating cdat table entries */ > + region->flags = 0; > + > + region_base += region->len; > + } > + > + return 0; Given it doesn't fail (even after the rest of this series is applied), why return anything? Make it void and we can drop the checks below.. > +} > + > static bool cxl_setup_memory(CXLType3Dev *ct3d, Error **errp) > { > DeviceState *ds = DEVICE(ct3d); > @@ -789,6 +819,10 @@ static bool cxl_setup_memory(CXLType3Dev *ct3d, Error **errp) > g_free(p_name); > } > > + if (cxl_create_dc_regions(ct3d)) { > + return false; > + } > + > return true; > } > > @@ -1108,6 +1142,7 @@ static Property ct3_props[] = { > DEFINE_PROP_UINT64("sn", CXLType3Dev, sn, UI64_NULL), > DEFINE_PROP_STRING("cdat", CXLType3Dev, cxl_cstate.cdat.filename), > DEFINE_PROP_UINT16("spdm", CXLType3Dev, spdm_port, 0), > + DEFINE_PROP_UINT8("num-dc-regions", CXLType3Dev, dc.num_regions, 0), > DEFINE_PROP_END_OF_LIST(), > }; >