Re: [PATCH v5 04/13] hw/mem/cxl_type3: Add support to create DC regions to type3 memory devices

[Jonathan Cameron]

On Mon,  4 Mar 2024 11:33:59 -0800
nifan.cxl@gmail.com wrote:

> From: Fan Ni <fan.ni@samsung.com>
> 
> 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 like 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 <fan.ni@samsung.com>
Suggested changes are trivial formatting things
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  hw/mem/cxl_type3.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 46 insertions(+)
> 
> diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> index 244d2b5fd5..a191211009 100644
> --- a/hw/mem/cxl_type3.c
> +++ b/hw/mem/cxl_type3.c
> @@ -30,6 +30,7 @@
>  #include "hw/pci/msix.h"
>  
>  #define DWORD_BYTE 4
> +#define CXL_CAPACITY_MULTIPLIER   (256 * MiB)
>  
>  /* Default CDAT entries for a memory region */
>  enum {
> @@ -567,6 +568,45 @@ static void ct3d_reg_write(void *opaque, hwaddr offset, 
> uint64_t value,
>      }
>  }
>  
> +/*
> + * TODO: dc region configuration will be updated once host backend and 
> address
> + * space support is added for DCD.
> + */
> +static bool cxl_create_dc_regions(CXLType3Dev *ct3d, Error **errp)
> +{
> +    int i;
> +    uint64_t region_base = 0;
> +    uint64_t region_len =  2 * GiB;
> +    uint64_t decode_len = 2 * GiB;
> +    uint64_t blk_size = 2 * MiB;
> +    CXLDCRegion *region;
> +    MemoryRegion *mr;
> +
> +    if (ct3d->hostvmem) {
> +        mr = host_memory_backend_get_memory(ct3d->hostvmem);
> +        region_base += memory_region_size(mr);
> +    }
> +    if (ct3d->hostpmem) {
> +        mr = host_memory_backend_get_memory(ct3d->hostpmem);
> +        region_base += memory_region_size(mr);
> +    }
> +    assert(region_base % CXL_CAPACITY_MULTIPLIER == 0);
> +
> +    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;

Maybe make the loop update to do some or all of the variable updating
(perhaps all of them is a bit too complex!)

    for (i = 0, region = &ct3d->dc_regions[0];
         i < ct3d->dc.num_regions;
         i++, region++, region_base += region_len) {
Also, using this style of assignment will avoid lots of repetition of region.
        *region = (CXLDCRegion) {
            .base = region_base,
            .decode_len = decode_len,
            .len = region_len,
            .block_size = blk_size,
            /* dsmad_handle set when creating CDAT table entries */
            .flags = 0,
        };
    }

> +    }
> +
> +    return true;
> +}