On Mon, 14 Jul 2025 15:15:12 +0100
Jonathan Cameron <jonathan.came...@huawei.com> wrote:
> On Mon, 14 Jul 2025 15:02:18 +0100
> Jonathan Cameron <jonathan.came...@huawei.com> wrote:
>
> > On Mon, 14 Jul 2025 05:32:19 -0400
> > "Michael S. Tsirkin" <m...@redhat.com> wrote:
> >
> > > On Wed, Jul 02, 2025 at 05:02:13PM +0100, Jonathan Cameron wrote:
> > > > From: Anisa Su <anisa...@samsung.com>
> > > >
> > > > FM DCD Management command 0x5602 implemented per CXL r3.2 Spec Section
> > > > 7.6.7.6.3
> > > >
> > > > Reviewed-by: Fan Ni <fan...@samsung.com>
> > > > Signed-off-by: Anisa Su <anisa...@samsung.com>
> > > > Signed-off-by: Jonathan Cameron <jonathan.came...@huawei.com>
> >
> >
> > > > diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> > > > index bf1710b251..1fc453f70d 100644
> > > > --- a/hw/cxl/cxl-mailbox-utils.c
> > > > +++ b/hw/cxl/cxl-mailbox-utils.c
> >
> > > > +/* CXL r3.2 section 7.6.7.6.3: Set Host DC Region Configuration
> > > > (Opcode 5602) */
> > > > +static CXLRetCode cmd_fm_set_dc_region_config(const struct cxl_cmd
> > > > *cmd,
> > > > + uint8_t *payload_in,
> > > > + size_t len_in,
> > > > + uint8_t *payload_out,
> > > > + size_t *len_out,
> > > > + CXLCCI *cci)
> > > > +{
> > > > + struct {
> > > > + uint8_t reg_id;
> > > > + uint8_t rsvd[3];
> > > > + uint64_t block_sz;
> > > > + uint8_t flags;
> > > > + uint8_t rsvd2[3];
> > > > + } QEMU_PACKED *in = (void *)payload_in;
> > > > + CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
> > > > + CXLEventDynamicCapacity dcEvent = {};
> > > > + CXLDCRegion *region = &ct3d->dc.regions[in->reg_id];
> > > > +
> > > > + /*
> > > > + * CXL r3.2 7.6.7.6.3: Set DC Region Configuration
> > > > + * This command shall fail with Unsupported when the Sanitize on
> > > > Release
> > > > + * field does not match the region’s configuration... and the
> > > > device
> > > > + * does not support reconfiguration of the Sanitize on Release
> > > > setting.
> > > > + *
> > > > + * Currently not reconfigurable, so always fail if sanitize bit
> > > > (bit 0)
> > > > + * doesn't match.
> > > > + */
> > > > + if ((in->flags & 0x1) != (region->flags & 0x1)) {
> > > > + return CXL_MBOX_UNSUPPORTED;
> > > > + }
> > > > +
> > > > + if (in->reg_id >= DCD_MAX_NUM_REGION) {
> > > > + return CXL_MBOX_UNSUPPORTED;
> > > > + }
> > > > +
> > > > + /* Check that no extents are in the region being reconfigured */
> > > > + if (!bitmap_empty(region->blk_bitmap, region->len /
> > > > region->block_size)) {
> > > > + return CXL_MBOX_UNSUPPORTED;
> > > > + }
> > > > +
> > > > + /* Check that new block size is supported */
> > > > + if (!test_bit(BIT((int) log2(in->block_sz)),
> > > > + ®ion->supported_blk_size_bitmask)) {
> > > > + return CXL_MBOX_INVALID_INPUT;
> > > > + }
> > >
> > > This does not work: test_bit works on unsigned long, while
> > > supported_blk_size_bitmask is uint64_t.
> > >
> > > Why so funky? what is wrong with:
> > >
> > > if (!(BIT_ULL(log2(in->block_sz)) & region->supported_blk_size_bitmask))
> > >
> > > And BTW why cast to int here?
> This became obvious when your suggestion didn't build :(
>
> ./../hw/cxl/cxl-mailbox-utils.c: In function ‘cmd_fm_set_dc_region_config’:
> /home/jic23/src/qemu/include/qemu/bitops.h:25:39: error: invalid operands to
> binary << (have ‘long long unsigned int’ and ‘double’)
> 25 | #define BIT_ULL(nr) (1ULL << (nr))
> | ^~ ~~~~
> ../../hw/cxl/cxl-mailbox-utils.c:3436:11: note: in expansion of macro
> ‘BIT_ULL’
> 3436 | if (!(BIT_ULL(log2(in->block_sz)) &
> region->supported_blk_size_bitmask)) {
> | ^~~~~~~
>
> Now I look again, this is effectively 2**(log_2(x)) or x. So
> if (in->block_sz & region->supporte_blk_size_bitmask)
it (!(in->block_sz & region->supports_blk_size_bitmask))
I mean.
> Should work as long as we know block_size is a power of 2 (which the
> specification
> says it must be).
>
> Anisa?
>
> >
> > Change looks fine to me, so I'll prepare an updated set with this
> > and the missing semi colon. Anisa if you can have a look at this
> > that would be great.
> >
> > Sorry I seem to have missed Anisa off the cc for this!
> >
> > Jonathan
> >
>
