Re: [PATCH 2/4] cxl/mailbox: interface to add CCI commands to an existing CCI

[Jonathan Cameron via <qemu-devel@nongnu.org>]

On Fri, 4 Aug 2023 12:41:26 -0400
Gregory Price <gregory.price@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.memverge@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.price@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.memverge@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.price@memverge.com>
> > Link: https://lore.kernel.org/r/20230721163505.1910-3-gregory.price@memverge.com
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@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,  
> >