On Wed, 19 Jul 2023 14:49:07 -0400
Gregory Price <gregory.pr...@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.pr...@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-6eb2e4709...@intel.com/
Jonathan
>
> ~Gregory
