On Fri, 4 Aug 2023 12:41:26 -0400 Gregory Price <gregory.pr...@memverge.com> wrote:
> On Fri, Aug 04, 2023 at 04:14:14PM +0100, Jonathan Cameron wrote: > > On Fri, 21 Jul 2023 12:35:06 -0400 > > Gregory Price <gourry.memve...@gmail.com> wrote: > > > > > This enables wrapper devices to customize the base device's CCI > > > (for example, with custom commands outside the specification) > > > without the need to change the base device. > > > > > > The also enabled the base device to dispatch those commands without > > > requiring additional driver support. > > > > > > Signed-off-by: Gregory Price <gregory.pr...@memverge.com> > > > --- > > > hw/cxl/cxl-mailbox-utils.c | 19 +++++++++++++++++++ > > > include/hw/cxl/cxl_device.h | 2 ++ > > > 2 files changed, 21 insertions(+) > > > > > > diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c > > > index ddee3f1718..cad0cd0adb 100644 > > > --- a/hw/cxl/cxl-mailbox-utils.c > > > +++ b/hw/cxl/cxl-mailbox-utils.c > > > @@ -1383,6 +1383,25 @@ static void cxl_copy_cci_commands(CXLCCI *cci, > > > const struct cxl_cmd (*cxl_cmds)[ > > > } > > > } > > > > > > +void cxl_add_cci_commands(CXLCCI *cci, const struct cxl_cmd > > > (*cxl_cmd_set)[256], size_t payload_max) > > > +{ > > > + cci->payload_max = payload_max > cci->payload_max ? payload_max : > > > cci->payload_max; > > > + for (int set = 0; set < 256; set++) { > > > + for (int cmd = 0; cmd < 256; cmd++) { > > > + if (cxl_cmd_set[set][cmd].handler) { > > > + const struct cxl_cmd *c = &cxl_cmd_set[set][cmd]; > > > + cci->cxl_cmd_set[set][cmd] = *c; > > Don't interleave definitions and code. > > > > > + struct cel_log *log = > > > + &cci->cel_log[cci->cel_size]; > > > + > > > + log->opcode = (set << 8) | cmd; > > > + log->effect = c->effect; > > > + cci->cel_size++; > > > > So my gut feeling on this is based on the large amount of overlapping code. > > I might queue it > > as it stands, but I'd like to see this refactored. > > > > 1) Single copy routine used in all places that copie in any new entries to > > cci->cxl_cmd_set[][] > > 2) A single cel_log builder function to be called in normal path and after > > an update. Just rebuild > > the whole thing rather than trying to append to it I think. > > > > Something like (so far untested but I'll poke it with Fan's code in a few > > mins) > > > > However this is all proving rather costly in space so maybe we need a better > > representation for the sparse nature of cxl comamnds - a job for another > > day. > > I'd certainly considered the issue of space, but it seemed better to > blow up the size in one commit and then come back around and change the > structure out from under the work this unblocks. What we save in space > we sacrifice in complexity, but the structure seems simple enough that a > change shouldn't take a ton of scrutiny to get right. Makes sense. > > One downside of the approach here is what happens when there's an > overlap and custom devices build up over time. As in - if i steal the > 0xFF command group for my custom emulated MHMLD DCD Everything Super Device, > what happens if the spec finally comes around to defining 0xFF as a real > command set? We don't support your command unless it's in the vendor defined space. Opcodes c000h-ffffh are all yours to do what you like with :) > > tl;dr: Should the copy function error on overlap detections? Maybe.. If it's easy we definitely could do that even if it's considered a bug if it occurs. > > Quick read-back through the spec, I don't see explicit carve-outs for > reserved command regions for custom sets, might be worth a discussion. 8.2.9 Component Command Interface "Opcodes C000h-FFFFh describe vendor specific commands." Jonathan > > For now it shouldn't be an issue. > > > > > > > From 8ab48adfb2b481be0702b84a0d172a4f142b0df6 Mon Sep 17 00:00:00 2001 > > From: Gregory Price <gourry.memve...@gmail.com> > > Date: Fri, 21 Jul 2023 12:35:06 -0400 > > Subject: [PATCH] cxl/mailbox: interface to add CCI commands to an existing > > CCI > > > > This enables wrapper devices to customize the base device's CCI > > (for example, with custom commands outside the specification) > > without the need to change the base device. > > > > The also enabled the base device to dispatch those commands without > > requiring additional driver support. > > > > Signed-off-by: Gregory Price <gregory.pr...@memverge.com> > > Link: > > https://lore.kernel.org/r/20230721163505.1910-3-gregory.pr...@memverge.com > > Signed-off-by: Jonathan Cameron <jonathan.came...@huawei.com> > > > > -- > > Heavily edited by Jonathan Cameron to increase code reuse > > --- > > include/hw/cxl/cxl_device.h | 2 ++ > > hw/cxl/cxl-mailbox-utils.c | 21 +++++++++++++++++++-- > > 2 files changed, 21 insertions(+), 2 deletions(-) > > > > diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h > > index 0c9254dff9..2a3050fbad 100644 > > --- a/include/hw/cxl/cxl_device.h > > +++ b/include/hw/cxl/cxl_device.h > > @@ -297,6 +297,8 @@ void cxl_initialize_mailbox_t3(CXLCCI *cci, DeviceState > > *d, size_t payload_max); > > void cxl_initialize_mailbox_swcci(CXLCCI *cci, DeviceState *intf, > > DeviceState *d, size_t payload_max); > > void cxl_init_cci(CXLCCI *cci, size_t payload_max); > > +void cxl_add_cci_commands(CXLCCI *cci, const struct cxl_cmd > > (*cxl_cmd_set)[256], > > + size_t payload_max); > > int cxl_process_cci_message(CXLCCI *cci, uint8_t set, uint8_t cmd, > > size_t len_in, uint8_t *pl_in, > > size_t *len_out, uint8_t *pl_out, > > diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c > > index 37703f254f..852e5a046b 100644 > > --- a/hw/cxl/cxl-mailbox-utils.c > > +++ b/hw/cxl/cxl-mailbox-utils.c > > @@ -1353,9 +1353,9 @@ static void bg_timercb(void *opaque) > > } > > } > > > > -void cxl_init_cci(CXLCCI *cci, size_t payload_max) > > +static void cxl_rebuild_cel(CXLCCI *cci) > > { > > - cci->payload_max = payload_max; > > + cci->cel_size = 0; /* Reset for a fresh build */ > > for (int set = 0; set < 256; set++) { > > for (int cmd = 0; cmd < 256; cmd++) { > > if (cci->cxl_cmd_set[set][cmd].handler) { > > @@ -1369,6 +1369,13 @@ void cxl_init_cci(CXLCCI *cci, size_t payload_max) > > } > > } > > } > > +} > > + > > +void cxl_init_cci(CXLCCI *cci, size_t payload_max) > > +{ > > + cci->payload_max = payload_max; > > + cxl_rebuild_cel(cci); > > + > > cci->bg.complete_pct = 0; > > cci->bg.starttime = 0; > > cci->bg.runtime = 0; > > @@ -1387,10 +1394,19 @@ static void cxl_copy_cci_commands(CXLCCI *cci, > > const struct cxl_cmd (*cxl_cmds)[ > > } > > } > > > > +void cxl_add_cci_commands(CXLCCI *cci, const struct cxl_cmd > > (*cxl_cmd_set)[256], > > + size_t payload_max) > > +{ > > + cci->payload_max = payload_max > cci->payload_max ? payload_max : > > cci->payload_max; > > + cxl_copy_cci_commands(cci, cxl_cmd_set); > > + cxl_rebuild_cel(cci); > > +} > > + > > void cxl_initialize_mailbox_swcci(CXLCCI *cci, DeviceState *intf, > > DeviceState *d, size_t payload_max) > > { > > cxl_copy_cci_commands(cci, cxl_cmd_set_sw); > > + cxl_rebuild_cel(cci); > > cci->d = d; > > cci->intf = intf; > > cxl_init_cci(cci, payload_max); > > @@ -1399,6 +1415,7 @@ void cxl_initialize_mailbox_swcci(CXLCCI *cci, > > DeviceState *intf, > > void cxl_initialize_mailbox_t3(CXLCCI *cci, DeviceState *d, size_t > > payload_max) > > { > > cxl_copy_cci_commands(cci, cxl_cmd_set); > > + cxl_rebuild_cel(cci); > > cci->d = d; > > > > /* No separation for PCI MB as protocol handled in PCI device */ > > -- > > 2.39.2 > > > > > > > > > + } > > > + } > > > + } > > > +} > > > + > > > void cxl_initialize_mailbox_swcci(CXLCCI *cci, DeviceState *intf, > > > DeviceState *d, size_t payload_max) > > > { > > > cxl_copy_cci_commands(cci, cxl_cmd_set_sw); > > > diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h > > > index 9a3c8b2dfa..abc8405cc5 100644 > > > --- a/include/hw/cxl/cxl_device.h > > > +++ b/include/hw/cxl/cxl_device.h > > > @@ -297,6 +297,8 @@ void cxl_initialize_mailbox_t3(CXLCCI *cci, > > > DeviceState *d, size_t payload_max); > > > void cxl_initialize_mailbox_swcci(CXLCCI *cci, DeviceState *intf, > > > DeviceState *d, size_t payload_max); > > > void cxl_init_cci(CXLCCI *cci, size_t payload_max); > > > +void cxl_add_cci_commands(CXLCCI *cci, const struct cxl_cmd > > > (*cxl_cmd_set)[256], > > > + size_t payload_max); > > > int cxl_process_cci_message(CXLCCI *cci, uint8_t set, uint8_t cmd, > > > size_t len_in, uint8_t *pl_in, > > > size_t *len_out, uint8_t *pl_out, > >