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 vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id BBA63C433EF for ; Fri, 28 Jan 2022 14:46:36 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1349400AbiA1Oqe convert rfc822-to-8bit (ORCPT ); Fri, 28 Jan 2022 09:46:34 -0500 Received: from frasgout.his.huawei.com ([185.176.79.56]:4547 "EHLO frasgout.his.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1349433AbiA1Oqe (ORCPT ); Fri, 28 Jan 2022 09:46:34 -0500 Received: from fraeml744-chm.china.huawei.com (unknown [172.18.147.200]) by frasgout.his.huawei.com (SkyGuard) with ESMTP id 4JlgHx4v9Zz67Xkw; Fri, 28 Jan 2022 22:46:05 +0800 (CST) Received: from lhreml710-chm.china.huawei.com (10.201.108.61) by fraeml744-chm.china.huawei.com (10.206.15.225) 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:46:32 +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:46:31 +0000 Date: Fri, 28 Jan 2022 14:46:22 +0000 From: Jonathan Cameron 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: <20220128144622.00004b7f@Huawei.com> In-Reply-To: <20220128142251.0000437f@Huawei.com> References: <20220124171705.10432-1-Jonathan.Cameron@huawei.com> <20220124171705.10432-3-Jonathan.Cameron@huawei.com> <87bkzyd3c7.fsf@linaro.org> <20220128142251.0000437f@Huawei.com> 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: 8BIT 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 Precedence: bulk List-ID: X-Mailing-List: linux-cxl@vger.kernel.org On Fri, 28 Jan 2022 14:22:51 +0000 Jonathan Cameron wrote: > On Wed, 26 Jan 2022 12:32:01 +0000 > Alex Bennée wrote: > > > Jonathan Cameron writes: > > > > > 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. See 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 = opaque; > > > + ComponentRegisters *cregs = &cxl_cstate->crb; > > > + > > > + assert(size == 4); > > > > You assert here but bellow: > > > > > + > > > +/* > > > + * 8.2.3 > > > + * The access restrictions specified in Section 8.2.2 also apply to CXL 2.0 > > > + * Component Registers. > > > + * > > > + * 8.2.2 > > > + * • A 32 bit register shall be accessed as a 4 Bytes quantity. Partial > > > + * reads are not permitted. > > > + * • A 64 bit register shall be accessed as a 8 Bytes quantity. Partial > > > + * reads are not permitted. > > > + * > > > + * As of the spec defined today, only 4 byte registers exist. > > > + */ > > > +static const MemoryRegionOps cache_mem_ops = { > > > + .read = cxl_cache_mem_read_reg, > > > + .write = cxl_cache_mem_write_reg, > > > + .endianness = DEVICE_LITTLE_ENDIAN, > > > + .valid = { > > > + .min_access_size = 4, > > > + .max_access_size = 8, > > > + .unaligned = false, > > > + }, > > > + .impl = { > > > + .min_access_size = 4, > > > + .max_access_size = 4, > > > + }, > > > +}; > > > > 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. > > > > 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#L48 > > cap_array = 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. However, after another read of the spec there are link related registers which we currently don't implement emulation for but do advertise as being present (as they are required) are 8 bytes long. Linux doesn't use them yet, but it may come so I'll need to leave max_access_size = 8 enabled and as you suggested LOG_UNIMP. Probably makes sense to implement the link registers in a follow up patch set along with (possibly) some kernel support to expose the information available in those registers. Jonathan > > Jonathan > > > Otherwise the rest looks ok to me: > > > > Reviewed-by: Alex Bennée > > >