Re: [RFC PATCH 10/17] misc/i2c_mctp_cxl: Initial device emulation

[Gregory Price <gregory.price@memverge.com>]

On Wed, Jul 19, 2023 at 09:19:47AM +0100, Jonathan Cameron wrote:
> On Tue, 18 Jul 2023 17:30:57 -0400
> Gregory Price <gregory.price@memverge.com> wrote:
> 
> > On Mon, Jul 17, 2023 at 06:16:39PM +0100, Jonathan Cameron wrote:
> > > @@ -397,8 +401,9 @@ struct CXLType3Dev {
> > >      AddressSpace hostpmem_as;
> > >      CXLComponentState cxl_cstate;
> > >      CXLDeviceState cxl_dstate;
> > > -    CXLCCI cci;
> > > -    
> > > +    CXLCCI cci; /* Primary PCI mailbox CCI */
> > > +    CXLCCI oob_mctp_cci; /* Initialized only if targetted */
> > > +  
> > 
> > I've been humming and hawing over this on the MHD stuff because I wanted
> > to figure out how to "add a CCI command" to a type-3 device without
> > either having a billion definitions for CCI command sets - or doing
> > something like this.
> > 
> > I don't hate this design pattern, I just want to ask whether your
> > intent is to end up with CXLType3Dev hosting many CXLCCI's based on what
> > wrapper types you have. 
> > 
> > Example: a type-3 device with mctp pass through and the MHD command set
> > 
> > CXLType3Dev {
> >     ...
> >     CXLCCI cci;
> >     CXLCCI oob_mctp_cci;
> >     CXLCCI mhd_cci;
> >     ...
> > }
> 
> Yes - that's what I was thinking.  In some cases a CCI may be accessed by
> tunneling on a different CCI on the same device as well as the option
> of tunneling to different devices.
> 
> So far the set that we'll end up with isn't too large. And if some aren't
> used for a given instantiation that's fine if it keeps the code simple.
> We may end up with other MCTP buses and to keep things consistent each one
> will need it's own target CXLCCI. If we need to rethink and make it dynamic
> to some degree we can look at it later.
> 

Maybe a dangerous suggestion.  Right now the CCI's are static:

static const struct cxl_cmd cxl_cmd_set[256][256]

how difficult might it be to allow these tables to be dynamic instead?
Then we could add an interface like this:

void cxl_add_cmd_set(CXLCCI *cci, CXLCCI *cmd_set, payload_max) {
	copy(cci, cmd_set);
}

This would enable not just adding sub-components piece-meal, but also if
someone wants to model a real device with custom CCI commands, they can
simply define a CCI set and pass it in via

cxl_add_cmd_set(&ct3d->cci, my_cmd_set, payload_max);

Which lets the existing /dev/cxl/memN device dispatch those commands,
and makes modeling real devices an easier endeavor.

Only downside is that this may require changing the command structure to
include a callback type and pointer per cci function. The upside is this
would also allow commands to be written somewhat agnostic to the device
they're being inherited by and allow for device nesting like...

-device cxl-type3, id=ct3d
-device cxl-mhd, target=ct3d
-device my_vendor_cxl_type3, target=ct3d
etc etc

otherwise we're probably going to end up with a cxl-type3 -device line
300 characters long.

Maybe that's over-generalizing a bit much n.n;

~Gregory