Re: [PATCH 1/9] cxl/type3: Add supported block sizes bitmask to CXLDCRegion struct

[Jonathan Cameron <Jonathan.Cameron@huawei.com>]

On Mon, 17 Mar 2025 16:31:28 +0000
anisa.su887@gmail.com wrote:

> From: Anisa Su <anisa.su@samsung.com>
> 
> Add supported_blk_size field to CXLDCRegion struct in preparation for
> next patch. It is needed by command 0x5600 Get DC Region Config.
Hi Anisa,

Sorry it took me so long to your series!

Squash this with patch 2.  It isn't complex enough addition to justify
as separate patch where we need to explain it is there for that patch
in the cover letter.

> 
> Signed-off-by: Anisa Su <anisa.su@samsung.com>
> ---
>  hw/mem/cxl_type3.c          | 3 +++
>  include/hw/cxl/cxl_device.h | 1 +
>  2 files changed, 4 insertions(+)
> 
> diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> index 11c38a9292..731497ebda 100644
> --- a/hw/mem/cxl_type3.c
> +++ b/hw/mem/cxl_type3.c
> @@ -8,6 +8,7 @@
>   *
>   * SPDX-License-Identifier: GPL-v2-only
>   */
> +#include <math.h>
>  
>  #include "qemu/osdep.h"
>  #include "qemu/units.h"
> @@ -766,6 +767,7 @@ static bool cxl_create_dc_regions(CXLType3Dev *ct3d, Error **errp)
>      uint64_t region_len;
>      uint64_t decode_len;
>      uint64_t blk_size = 2 * MiB;
> +    uint64_t supported_blk_size_bitmask = BIT((int) log2(blk_size));

Isn't this going in circles?  I guess it sort of acts as documentation that it
is a bitmap but then the name is already doing that. 
Maybe set it to blk_size and add a comment that for now only a fixed block size
is supported?

I'm a little confused on what this is for given you don't check it in patch 6
which is changing the block size?

>      CXLDCRegion *region;
>      MemoryRegion *mr;
>      uint64_t dc_size;
> @@ -811,6 +813,7 @@ static bool cxl_create_dc_regions(CXLType3Dev *ct3d, Error **errp)
>              .block_size = blk_size,
>              /* dsmad_handle set when creating CDAT table entries */
>              .flags = 0,
> +            .supported_blk_size_bitmask = supported_blk_size_bitmask,
>          };
>          ct3d->dc.total_capacity += region->len;
>          region->blk_bitmap = bitmap_new(region->len / region->block_size);
> diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
> index ca515cab13..bebed04085 100644
> --- a/include/hw/cxl/cxl_device.h
> +++ b/include/hw/cxl/cxl_device.h
> @@ -608,6 +608,7 @@ typedef struct CXLDCRegion {
>      uint32_t dsmadhandle;
>      uint8_t flags;
>      unsigned long *blk_bitmap;
> +    uint64_t supported_blk_size_bitmask;
>  } CXLDCRegion;
>  
>  typedef struct CXLSetFeatureInfo {