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

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

On Wed, 19 Jul 2023 14:49:07 -0400
Gregory Price <gregory.price@memverge.com> wrote:

> 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]

That's defined by the ID space for the commands.  There can't be more than
that many currently..

> 
> 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);

Ok.  I'm potentially fine with people adding an interface for this, but
only if they plan to also upstream the QEMU emulation of their actual
device.

> 
> 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;

I'd look to just inherit from a cxl type 3, like Ira did in the PoC for
type 2 support.   We can then easily add a path to replace the commands
set with whatever anyone wants.  I'm not sure we want the command line
to be used to configure such a device as it'll both get very complex and
prove increasingly hard to test more than a small subset of options.

https://lore.kernel.org/all/20230517-rfc-type2-dev-v1-0-6eb2e470981b@intel.com/


Jonathan

> 
> ~Gregory