On Mon, 31 Mar 2025 19:38:45 +0000
Anisa Su <anisa.su...@gmail.com> wrote:
> 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>
> > > --- 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
> Thanks for the feedback and review Jonathan.
>
> According to the comment above this condition, "Any command forming part
> of the CXL FM-API command set... is valid only with the CXL Fabric
> Manager API over MCTP binding (DSP0234)."
>
> From my understanding, this check is to ensure that any message
> sent from the FM API command set (0x51 - 0x59) has the MCTP_MT_CXL_FMAPI
> binding and all other commands (opcode < 0x51) are are sent with the
> MCTP_MT_CXL_TYPE3 binding.
Yes. That is the intent. Why the spec requires this distinction is a long
story we won't go into here...
>
> Although I see from r3.2 Table 8-230 CXL Defined FM API Command Opcodes
> that commands from sets 0x57-0x59 are prohibited from being implemented
> in the MCTP CCI, would it be more correct to change the condition for
> FMAPI commands to msg->command_set < 0x59? Then if/when commands from sets
> 0x57-0x59 are implemented, if they are implemented according to the spec, they
> should not be added to the FM MCTP CCI.
Agreed. For this check, if we are changing it we should update it to
incorporate the additional FM_API commands so < 0x59 to include the
various CXL fabrics things that have been added.
>
> Please correct my understanding if this is incorrect.
>
> Regarding the visibility problem, I intend to move the enum defining all the
> opcodes in cxl-mailbox.utils.c to cxl-mailbox.h and including cxl-mailbox.h
> in i2c_mctp_cxl.c
Ok. I was going to suggest a separate header to avoid info about mailboxes
that wasn't relevant bleeding over into this mctp bridge device but that
header has very little in it so we should be fine. We can reorganizing things
in some future set if that header gains lots of other stuff.
Thanks
Jonathan
>
> Let me know if that is what you intended.
>
> Other than that, I have removed the extraneous TO-DO's from the other
> patches and plan to send out v2 with relevant corrections soon.
> Hopefully that makes the remaining patches easier for you to review.
>
> Thanks,
> Anisa
>
>
> >
> >
> > > buf->rc = CXL_MBOX_UNSUPPORTED;
> > > st24_le_p(buf->pl_length, len_out);
> > > s->len = s->pos;
> >
