Re: [RFC PATCH 10/17] misc/i2c_mctp_cxl: Initial device emulation
On Thu, Jul 20, 2023 at 01:18:33PM +0100, Jonathan Cameron wrote:
> On Wed, 19 Jul 2023 14:49:07 -0400
> Gregory Price wrote:
>
> >
> > 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..
>
Meant commands beyond what is defined in the spec, not beyond the 256
limits.
> >
> > 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(>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.
>
Working on it :]
Hoping to show off a fully functional MHSLD with some custom commands
soon, and I think I'm happy with the abstraction that fell out on top of
this CCI work. Previously it required duplicating the type3 device or
hacking directly on it, which is a maintenance nightmare / not
upstreamable.
>
> 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
>
I made an attempt at this at first, but due to the custom commands, i
think everyone would (rightly) scoff at the idea of adding
non-specification defined stuff into the core type 3 device. Once I
shifted to modifying the CCI and overriding entries, the entire vendor
device ended up as mostly the CCI command definitions, which is exactly
how i envisioned doing it in the first place.
I'll post some additional patches to my MHD RFC, the changes were pretty
minor.
Hopefully will be able to tack on a concrete MHSLD following that..
~Gregory
Re: [RFC PATCH 10/17] misc/i2c_mctp_cxl: Initial device emulation
On Wed, 19 Jul 2023 14:49:07 -0400
Gregory Price 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 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(>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
Re: [RFC PATCH 10/17] misc/i2c_mctp_cxl: Initial device emulation
On Wed, Jul 19, 2023 at 09:19:47AM +0100, Jonathan Cameron wrote:
> On Tue, 18 Jul 2023 17:30:57 -0400
> Gregory Price 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(>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
Re: [RFC PATCH 10/17] misc/i2c_mctp_cxl: Initial device emulation
On Tue, 18 Jul 2023 17:30:57 -0400
Gregory Price 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.
>
> Instantiate:
> -device cxl-type3,bus=swport0,memdev=cxl-mem1,id=cxl-pmem1,lsa=cxl-lsa1,sn=3
> -device i2c_mctp_cxl,bus=aspeed.i2c.bus.0,address=5,target=cxl-pmem1
> -device cxl-mhd,target=cxl-pmem1,...whatever else...
Not sure on this - it may be implicit in creating an MHD rather than requiring
a command line to target through. Depends on what the MHD creation code
looks like - but this is definitely a possibility.
>
> where the MHD code is contained within its own type/file, and the type3
> device hosts the CCI for it. Similar to how you've implemented the MTCP
> stuff here.
>
> The reason I ask is because certain CCI's don't necessarily get
> associated with "a bus" so much as "a device". the MHD example - it's
> still part of "the device", but it's optional.
For emulation I don't think we care if it's optional. I think we implement
it whatever and if it is not accessed that is fine.
> So does it make sense
> to create this wrapper without a bus association, or to just pile it on
> top CXLType3Dev and have to duplicate the code across any other
> multi-headed devices that folks may conjur up?
Piling it on top of CXLType3Dev was what I was thinking. We can rethink if
there other multi-headed devices using similar interfaces.
Jonathan
>
> ~Gregory
Re: [RFC PATCH 10/17] misc/i2c_mctp_cxl: Initial device emulation
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;
...
}
Instantiate:
-device cxl-type3,bus=swport0,memdev=cxl-mem1,id=cxl-pmem1,lsa=cxl-lsa1,sn=3
-device i2c_mctp_cxl,bus=aspeed.i2c.bus.0,address=5,target=cxl-pmem1
-device cxl-mhd,target=cxl-pmem1,...whatever else...
where the MHD code is contained within its own type/file, and the type3
device hosts the CCI for it. Similar to how you've implemented the MTCP
stuff here.
The reason I ask is because certain CCI's don't necessarily get
associated with "a bus" so much as "a device". the MHD example - it's
still part of "the device", but it's optional. So does it make sense
to create this wrapper without a bus association, or to just pile it on
top CXLType3Dev and have to duplicate the code across any other
multi-headed devices that folks may conjur up?
~Gregory
