On Thu, Apr 24, 2025 at 11:11:31AM +0100, Jonathan Cameron wrote:
> On Mon, 17 Mar 2025 16:31:28 +0000
> anisa.su...@gmail.com wrote:
>
> > From: Anisa Su <anisa...@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.
...
> > @@ -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?
Good catch! It doesn't seem to specify this check in Section 7.6.7.6.3
Set DC Region Configuration (Opcode 5602h) in the 3.2 spec, but it would
make sense to me to fail with the same rc as when the region has not
been cleared, which is CXL_MBOX_UNSUPPORTED.
Will have the fix in v2.
> > 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 {
>
