Re: [PATCH v4 02/42] hw/cxl/component: Introduce CXL components (8.1.x, 8.2.5)

From: "Alex Bennée" <alex.bennee@linaro.org>
To: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: qemu-devel@nongnu.org, "Marcel Apfelbaum" <marcel@redhat.com>,
	"Michael S . Tsirkin" <mst@redhat.com>,
	"Igor Mammedov" <imammedo@redhat.com>,
	linux-cxl@vger.kernel.org,
	"Ben Widawsky" <ben.widawsky@intel.com>,
	"Peter Maydell" <peter.maydell@linaro.org>,
	linuxarm@huawei.com,
	"Shameerali Kolothum Thodi"
	<shameerali.kolothum.thodi@huawei.com>,
	"Philippe Mathieu-Daudé" <f4bug@amsat.org>,
	"Saransh Gupta1" <saransh@ibm.com>,
	"Shreyas Shah" <shreyas.shah@elastics.cloud>,
	"Chris Browy" <cbrowy@avery-design.com>,
	"Samarth Saxena" <samarths@cadence.com>,
	"Dan Williams" <dan.j.williams@intel.com>
Subject: Re: [PATCH v4 02/42] hw/cxl/component: Introduce CXL components (8.1.x, 8.2.5)
Date: Wed, 26 Jan 2022 12:32:01 +0000	[thread overview]
Message-ID: <87bkzyd3c7.fsf@linaro.org> (raw)
In-Reply-To: <20220124171705.10432-3-Jonathan.Cameron@huawei.com>

Jonathan Cameron <Jonathan.Cameron@huawei.com> writes:

> From: Ben Widawsky <ben.widawsky@intel.com>
>
> 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 <ben.widawsky@intel.com>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
>  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.

Otherwise the rest looks ok to me:

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée