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 B1A40C433F5 for ; Fri, 28 Jan 2022 14:39:58 +0000 (UTC) Received: from localhost ([::1]:42652 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1nDSPt-0007In-II for qemu-devel@archiver.kernel.org; Fri, 28 Jan 2022 09:39:57 -0500 Received: from eggs.gnu.org ([209.51.188.92]:38378) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1nDS9b-0005bK-0h for qemu-devel@nongnu.org; Fri, 28 Jan 2022 09:23:07 -0500 Received: from frasgout.his.huawei.com ([185.176.79.56]:2217) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1nDS9X-0003Et-Mv for qemu-devel@nongnu.org; Fri, 28 Jan 2022 09:23:06 -0500 Received: from fraeml737-chm.china.huawei.com (unknown [172.18.147.201]) by frasgout.his.huawei.com (SkyGuard) with ESMTP id 4Jlfmp1LHxz67bjw; Fri, 28 Jan 2022 22:22:34 +0800 (CST) Received: from lhreml710-chm.china.huawei.com (10.201.108.61) by fraeml737-chm.china.huawei.com (10.206.15.218) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2308.21; Fri, 28 Jan 2022 15:23:00 +0100 Received: from localhost (10.47.76.156) by lhreml710-chm.china.huawei.com (10.201.108.61) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2308.21; Fri, 28 Jan 2022 14:22:59 +0000 Date: Fri, 28 Jan 2022 14:22:51 +0000 To: Alex =?ISO-8859-1?Q?Benn=E9e?= CC: , Marcel Apfelbaum , "Michael S . Tsirkin" , Igor Mammedov , , Ben Widawsky , "Peter Maydell" , , "Shameerali Kolothum Thodi" , Philippe =?ISO-8859-1?Q?Mathieu-Daud=E9?= , Saransh Gupta1 , Shreyas Shah , Chris Browy , Samarth Saxena , "Dan Williams" Subject: Re: [PATCH v4 02/42] hw/cxl/component: Introduce CXL components (8.1.x, 8.2.5) Message-ID: <20220128142251.0000437f@Huawei.com> In-Reply-To: <87bkzyd3c7.fsf@linaro.org> References: <20220124171705.10432-1-Jonathan.Cameron@huawei.com> <20220124171705.10432-3-Jonathan.Cameron@huawei.com> <87bkzyd3c7.fsf@linaro.org> Organization: Huawei Technologies Research and Development (UK) Ltd. X-Mailer: Claws Mail 4.0.0 (GTK+ 3.24.29; i686-w64-mingw32) MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable X-Originating-IP: [10.47.76.156] X-ClientProxiedBy: lhreml731-chm.china.huawei.com (10.201.108.82) To lhreml710-chm.china.huawei.com (10.201.108.61) X-CFilter-Loop: Reflected 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_H4=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: , Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: "Qemu-devel" Reply-to: Jonathan Cameron From: Jonathan Cameron via On Wed, 26 Jan 2022 12:32:01 +0000 Alex Benn=C3=A9e wrote: > Jonathan Cameron writes: >=20 > > From: Ben Widawsky > > > > A CXL 2.0 component is any entity in the CXL topology. All components > > have a analogous function in PCIe. Except for the CXL host bridge, all > > have a PCIe config space that is accessible via the common PCIe > > mechanisms. CXL components are enumerated via DVSEC fields in the > > extended PCIe header space. CXL components will minimally implement some > > subset of CXL.mem and CXL.cache registers defined in 8.2.5 of the CXL > > 2.0 specification. Two headers and a utility library are introduced to > > support the minimum functionality needed to enumerate components. > > > > The cxl_pci header manages bits associated with PCI, specifically the > > DVSEC and related fields. The cxl_component.h variant has data > > structures and APIs that are useful for drivers implementing any of the > > CXL 2.0 components. The library takes care of making use of the DVSEC > > bits and the CXL.[mem|cache] registers. Per spec, the registers are > > little endian. > > > > None of the mechanisms required to enumerate a CXL capable hostbridge > > are introduced at this point. > > > > Note that the CXL.mem and CXL.cache registers used are always 4B wide. > > It's possible in the future that this constraint will not hold. > > > > Signed-off-by: Ben Widawsky > > Signed-off-by: Jonathan Cameron > > --- > > hw/Kconfig | 1 + > > hw/cxl/Kconfig | 3 + > > hw/cxl/cxl-component-utils.c | 212 +++++++++++++++++++++++++++++++++ > > hw/cxl/meson.build | 3 + > > hw/meson.build | 1 + > > include/hw/cxl/cxl.h | 16 +++ > > include/hw/cxl/cxl_component.h | 196 ++++++++++++++++++++++++++++++ > > include/hw/cxl/cxl_pci.h | 138 +++++++++++++++++++++ > > 8 files changed, 570 insertions(+) > > > > diff --git a/hw/Kconfig b/hw/Kconfig > > index ad20cce0a9..50e0952889 100644 > > --- a/hw/Kconfig > > +++ b/hw/Kconfig > > @@ -6,6 +6,7 @@ source audio/Kconfig > > source block/Kconfig > > source char/Kconfig > > source core/Kconfig > > +source cxl/Kconfig > > source display/Kconfig > > source dma/Kconfig > > source gpio/Kconfig > > diff --git a/hw/cxl/Kconfig b/hw/cxl/Kconfig > > new file mode 100644 > > index 0000000000..8e67519b16 > > --- /dev/null > > +++ b/hw/cxl/Kconfig > > @@ -0,0 +1,3 @@ > > +config CXL > > + bool > > + default y if PCI_EXPRESS > > diff --git a/hw/cxl/cxl-component-utils.c b/hw/cxl/cxl-component-utils.c > > new file mode 100644 > > index 0000000000..5007b29ebb > > --- /dev/null > > +++ b/hw/cxl/cxl-component-utils.c > > @@ -0,0 +1,212 @@ > > +/* > > + * CXL Utility library for components > > + * > > + * Copyright(C) 2020 Intel Corporation. > > + * > > + * This work is licensed under the terms of the GNU GPL, version 2. Se= e the > > + * COPYING file in the top-level directory. > > + */ > > + > > +#include "qemu/osdep.h" > > +#include "qemu/log.h" > > +#include "hw/pci/pci.h" > > +#include "hw/cxl/cxl.h" > > + > > +static uint64_t cxl_cache_mem_read_reg(void *opaque, hwaddr offset, > > + unsigned size) > > +{ > > + CXLComponentState *cxl_cstate =3D opaque; > > + ComponentRegisters *cregs =3D &cxl_cstate->crb; > > + > > + assert(size =3D=3D 4); =20 >=20 > You assert here but bellow: >=20 > > + > > +/* > > + * 8.2.3 > > + * The access restrictions specified in Section 8.2.2 also apply to = CXL 2.0 > > + * Component Registers. > > + * > > + * 8.2.2 > > + * =E2=80=A2 A 32 bit register shall be accessed as a 4 Bytes quanti= ty. Partial > > + * reads are not permitted. > > + * =E2=80=A2 A 64 bit register shall be accessed as a 8 Bytes quanti= ty. Partial > > + * reads are not permitted. > > + * > > + * As of the spec defined today, only 4 byte registers exist. > > + */ > > +static const MemoryRegionOps cache_mem_ops =3D { > > + .read =3D cxl_cache_mem_read_reg, > > + .write =3D cxl_cache_mem_write_reg, > > + .endianness =3D DEVICE_LITTLE_ENDIAN, > > + .valid =3D { > > + .min_access_size =3D 4, > > + .max_access_size =3D 8, > > + .unaligned =3D false, > > + }, > > + .impl =3D { > > + .min_access_size =3D 4, > > + .max_access_size =3D 4, > > + }, > > +}; =20 >=20 > You have constrained the access to 4 so you will only see 4 bytes > accesses. If it is valid for the guest to access 64bit words then it > would be better to no-op that case and maybe LOG_UNIMP the fact. >=20 Ugh. This looks suspiciously like a work around for a kernel bug (possibly an accidental one). If the comment above is correct (I've checked the spec and agree with it).. https://elixir.bootlin.com/linux/v5.17-rc1/source/drivers/cxl/core/regs.c#L= 48 cap_array =3D readq(base + CXL_CM_CAP_HDR_OFFSET); is undefined behavior as CAP_HDR is a 32bit register. I guess the undefined choice on all the hardware people actually have is to service the 8 byte read, but there is no reason to believe future hardware will. ... and this is why we like having emulation in Qemu :) + eagle eyed reviewers! Guess I'd better send a kernel fix sometime soon. Jonathan > Otherwise the rest looks ok to me: >=20 > Reviewed-by: Alex Benn=C3=A9e >=20