On Tue, Mar 18, 2025 at 03:56:24PM +0000, Jonathan Cameron wrote:
> On Mon, 17 Mar 2025 16:31:29 +0000
> anisa.su...@gmail.com wrote:
>
> > From: Anisa Su <anisa...@samsung.com>
> >
> > FM DCD Management command 0x5600 implemented per CXL 3.2 Spec Section
> > 7.6.7.6.1
> >
> > Signed-off-by: Anisa Su <anisa...@samsung.com>
> > ---
> > hw/cxl/cxl-mailbox-utils.c | 67 ++++++++++++++++++++++++++++++++++++++
> > hw/cxl/i2c_mctp_cxl.c | 6 +++-
> > 2 files changed, 72 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> > index 1b62d36101..e9991fd1a7 100644
> > --- a/hw/cxl/cxl-mailbox-utils.c
> > +++ b/hw/cxl/cxl-mailbox-utils.c
> > @@ -122,6 +122,8 @@ enum {
> > #define MANAGEMENT_COMMAND 0x0
> > MHD = 0x55,
> > #define GET_MHD_INFO 0x0
> > + FMAPI_DCD_MGMT = 0x56,
> > + #define GET_DCD_INFO 0x0
> > };
> >
> > /* CCI Message Format CXL r3.1 Figure 7-19 */
> > @@ -3341,6 +3343,62 @@ static CXLRetCode cmd_dcd_release_dyn_cap(const
> > struct cxl_cmd *cmd,
> > return CXL_MBOX_SUCCESS;
> > }
> >
> > +/*
> > + * CXL r3.2 section 7.6.7.6.1: Get DCD Info (Opcode 5600h)
> > + */
>
> Single line comment should be fine here.
>
> > +static CXLRetCode cmd_fm_get_dcd_info(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 num_hosts;
> > + uint8_t num_regions_supported;
> > + uint8_t rsvd1[2];
> > + uint16_t add_select_policy_bitmask;
> > + uint8_t rsvd2[2];
> > + uint16_t release_select_policy_bitmask;
> > + uint8_t sanitize_on_release_bitmask;
> > + uint8_t rsvd3;
> > + uint64_t total_dynamic_capacity;
> > + uint64_t region_blk_size_bitmasks[8];
> > + } QEMU_PACKED *out;
> } QEMU_PACKED *out = (void *)payload_out;
> > + CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
> > + CXLDCRegion region;
> > + int i;
> > +
> > + if (ct3d->dc.num_regions == 0) {
> > + return CXL_MBOX_UNSUPPORTED;
> > + }
> > +
> > + out = (void *)payload_out;
> Why not just do this at declaration above?
> It is harmless to set it then even if we exit earlier
> I think.
>
> > +
> > + /* TODO: num hosts set to 1 for now */
>
> Unless this changes later in the set, no need for a todo here.
> This simply denotes what we are emulating. Maybe we will make
> it more flexible in future, maybe not.
>
> > + out->num_hosts = 1;
> > + out->num_regions_supported = ct3d->dc.num_regions;
> > + /* TODO: only prescriptive supported for now */
>
> Likewise, not a todo that needs comment. Just a current setting.
> As long as we never make it nor support this we are fine for
> compatibility etc. The CXL stuff doesn't support migration anyway
> so not problems there.
>
> > + stw_le_p(&out->add_select_policy_bitmask,
> > + CXL_EXTENT_SELECTION_POLICY_PRESCRIPTIVE);
should be
BIT(CXL_EXTENT_SELECTION_POLICY_PRESCRIPTIVE)
> > + stw_le_p(&out->release_select_policy_bitmask,
> > + CXL_EXTENT_REMOVAL_POLICY_PRESCRIPTIVE);
should be
BIT(CXL_EXTENT_REMOVAL_POLICY_PRESCRIPTIVE)
-Anisa
> > + /* TODO: sanitize on release bitmask cleared for now */
>
> As with above, not really a todo, more of a choice made for now.
>
> > + out->sanitize_on_release_bitmask = 0;
> > +
> > + stq_le_p(&out->total_dynamic_capacity,
> > + ct3d->dc.total_capacity / CXL_CAPACITY_MULTIPLIER);
> > +
> > + for (i = 0; i < ct3d->dc.num_regions; i++) {
> > + region = ct3d->dc.regions[i];
> > + memcpy(&out->region_blk_size_bitmasks[i],
> > + ®ion.supported_blk_size_bitmask, 8);
>
> sizeof(out->region_blk_size_bitmasks[i])
>
> > + }
> > +
> > + *len_out = sizeof(*out);
> > + return CXL_MBOX_SUCCESS;
> > +}
> > +
> > static const struct cxl_cmd cxl_cmd_set[256][256] = {
> > [INFOSTAT][BACKGROUND_OPERATION_ABORT] = {
> > "BACKGROUND_OPERATION_ABORT",
> > cmd_infostat_bg_op_abort, 0, 0 },
> > @@ -3462,6 +3520,11 @@ static const struct cxl_cmd cxl_cmd_set_sw[256][256]
> > = {
> > cmd_tunnel_management_cmd, ~0, 0 },
> > };
> >
> > +static const struct cxl_cmd cxl_cmd_set_fm_dcd[256][256] = {
> > + [FMAPI_DCD_MGMT][GET_DCD_INFO] = { "GET_DCD_INFO",
> > + cmd_fm_get_dcd_info, 0, 0},
> > +};
> > +
> > /*
> > * While the command is executing in the background, the device should
> > * update the percentage complete in the Background Command Status Register
> > @@ -3764,7 +3827,11 @@ void cxl_initialize_t3_fm_owned_ld_mctpcci(CXLCCI
> > *cci, DeviceState *d,
> > DeviceState *intf,
> > size_t payload_max)
> > {
> > + CXLType3Dev *ct3d = CXL_TYPE3(d);
> > cxl_copy_cci_commands(cci, cxl_cmd_set_t3_fm_owned_ld_mctp);
> > + if (ct3d->dc.num_regions) {
> > + cxl_copy_cci_commands(cci, cxl_cmd_set_fm_dcd);
> > + }
> > cci->d = d;
> > cci->intf = intf;
> > cxl_init_cci(cci, payload_max);
> > diff --git a/hw/cxl/i2c_mctp_cxl.c b/hw/cxl/i2c_mctp_cxl.c
> > index 7d2cbc3b75..df95182925 100644
> > --- a/hw/cxl/i2c_mctp_cxl.c
> > +++ b/hw/cxl/i2c_mctp_cxl.c
> > @@ -46,6 +46,9 @@
> > /* Implementation choice - may make this configurable */
> > #define MCTP_CXL_MAILBOX_BYTES 512
> >
> > +/* Supported FMAPI Cmds */
> > +#define FMAPI_CMD_MAX_OPCODE 0x57
> > +
> > typedef struct CXLMCTPMessage {
> > /*
> > * DSP0236 (MCTP Base) Integrity Check + Message Type
> > @@ -200,7 +203,8 @@ static void i2c_mctp_cxl_handle_message(MCTPI2CEndpoint
> > *mctp)
> > if (!(msg->message_type == MCTP_MT_CXL_TYPE3 &&
> > msg->command_set < 0x51) &&
> > !(msg->message_type == MCTP_MT_CXL_FMAPI &&
> > - msg->command_set >= 0x51 && msg->command_set < 0x56)) {
> > + msg->command_set >= 0x51 &&
> > + msg->command_set < FMAPI_CMD_MAX_OPCODE)) {
>
> Hmm. There is a visibility problem here we should address but probably not
> by introducing a new define. Maybe we should move the enum from
> cxl-mailbox-utils.c in a precursor patch.
>
> Jonathan
>
>
> > buf->rc = CXL_MBOX_UNSUPPORTED;
> > st24_le_p(buf->pl_length, len_out);
> > s->len = s->pos;
>
