Re: [PATCH qemu 07/11] hw/cxl: mailbox-utils: 0x5602 - FMAPI Set DC Region Config

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

On Mon, 14 Jul 2025 17:21:10 +0000
Anisa Su <anisa.su887@gmail.com> wrote:

> On Mon, Jul 14, 2025 at 06:02:26PM +0100, Jonathan Cameron wrote:
> > On Mon, 14 Jul 2025 09:45:31 -0700
> > Fan Ni <nifan.cxl@gmail.com> wrote:
> >   
> > > On Mon, Jul 14, 2025 at 03:16:38PM +0100, Jonathan Cameron wrote:  
> > > > On Mon, 14 Jul 2025 15:15:12 +0100
> > > > Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:
> > > >     
> > > > > On Mon, 14 Jul 2025 15:02:18 +0100
> > > > > Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:
> > > > >     
> > > > > > On Mon, 14 Jul 2025 05:32:19 -0400
> > > > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > > >       
> > > > > > > On Wed, Jul 02, 2025 at 05:02:13PM +0100, Jonathan Cameron wrote:        
> > > > > > > > From: Anisa Su <anisa.su@samsung.com>
> > > > > > > > 
> > > > > > > > FM DCD Management command 0x5602 implemented per CXL r3.2 Spec Section 7.6.7.6.3
> > > > > > > > 
> > > > > > > > Reviewed-by: Fan Ni <fan.ni@samsung.com>
> > > > > > > > Signed-off-by: Anisa Su <anisa.su@samsung.com>
> > > > > > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@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)),
> > > > > > > > +                  &region->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.    
> > > 
> > > Make sense to me. 
> > > 
> > > The only thing is how to detect the violation if the passed in block_sz
> > > is not power of 2.
> > > Or who will do the check if not in qemu?  
> > Hi Fan,
> > 
> > I checked the spec on this.  There isn't an explicit statement that the device
> > should return an error on this. Looks to be impdef. I'd happily see such
> > a check as a usability improvement though!
> > 
> > I'm just not set up to test this right now so decided to be
> > risk averse and not trying adding one. 
> > 
> > If you want to send a patch on top I'd be happy to add it.
> > 
> > Jonathan
> > 
> >   
> Would something like this work?
> 
> /* Check that new block size is supported */
> if (!is_power_of_2(in->block_sz) ||
>     !(in->block_sz & region->supported_blk_size_bitmask)) {
>     return CXL_MBOX_INVALID_INPUT;
> }
> 
> I did not have to add any extra headers to use this helper function in
> include/qemu/host-utils.h and it compiles for me.
Nice.  I knew I was too tired today!

I'll roll that in as seems safe enough.
V2 coming shortly.

J
> 
> Anisa
> >   
> > > 
> > > Fan
> > >   
> > > > 
> > > >     
> > > > > 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
> > > > > >       
> > > > >     
> > > >     
> > >   
> >   
> 
>