[Pdbg] [PATCH 14/17] libpdbg: Expand sbefifo data marshalling and calling chip-op
Alistair Popple
alistair at popple.id.au
Wed Nov 6 10:09:45 AEDT 2019
On Tuesday, 5 November 2019 5:31:05 PM AEDT Amitay Isaacs wrote:
> On Tue, 2019-11-05 at 17:22 +1100, Alistair Popple wrote:
> > On Tuesday, 5 November 2019 3:39:42 PM AEDT Amitay Isaacs wrote:
> > > On Tue, 2019-11-05 at 15:18 +1100, Alistair Popple wrote:
> > > > On Thursday, 31 October 2019 5:34:25 PM AEDT Amitay Isaacs wrote:
> > > > > This will allow to separate sbefifo transport layer to use
> > > > > something other
> > > > > than sbefifo kernel device driver.
> > > > >
> > > > > Signed-off-by: Amitay Isaacs <amitay at ozlabs.org>
> > > > > ---
> > > > > libpdbg/sbefifo.c | 132 ++++++++++++++++++++++++++++--------
> > > > > ----
> > > > > ------
> > > > > 1 file changed, 80 insertions(+), 52 deletions(-)
> > > > >
> > > > > diff --git a/libpdbg/sbefifo.c b/libpdbg/sbefifo.c
> > > > > index 030e6e9..9e3532b 100644
> > > > > --- a/libpdbg/sbefifo.c
> > > > > +++ b/libpdbg/sbefifo.c
> > > > > @@ -30,12 +30,29 @@ static uint32_t sbefifo_op_ffdc_get(struct
> > > > > sbefifo *sbefifo,
> > > > const uint8_t **ffd
> > > > > return sbefifo_ffdc_get(sbefifo->sf_ctx, ffdc,
> > > > > ffdc_len);
> > > > > }
> > > > >
> > > > > -static int sbefifo_op_istep(struct sbefifo *sbefifo,
> > > > > - uint32_t major, uint32_t minor)
> > > > > +static int sbefifo_op_istep(struct sbefifo *sbefifo, uint32_t
> > > > > major, uint32_t
> > > > minor)
> > > > > {
> > > > > + uint8_t *msg, *out;
> > > > > + uint32_t msg_len, out_len;
> > > > > + int rc;
> > > > > +
> > > > > PR_NOTICE("sbefifo: istep %u.%u\n", major, minor);
> > > > >
> > > > > - return sbefifo_istep_execute(sbefifo->sf_ctx, major &
> > > > > 0xff,
> > > > > minor & 0xff);
> > > > > + rc = sbefifo_istep_execute_push(major & 0xff, minor &
> > > > > 0xff,
> > > > > &msg, &msg_len);
> > > > > + if (rc)
> > > > > + return rc;
> > > > > +
> > > > > + out_len = 0;
> > > > > + rc = sbefifo_operation(sbefifo->sf_ctx, msg, msg_len,
> > > > > &out,
> > > > > &out_len);
> > > >
> > > > This code looks pretty much the same as what is in libsbefifo, so
> > > > why
> > > > not just
> > > > call that?
> > > >
> > > > I see that later on in the series you change the
> > > > sbefifo_operation()
> > > > call to
> > > > sbefifo->chipop(), but given you have defined the struct
> > > > sbefifo_context and it's
> > > > still required for calls to libsbefifo why not just define a
> > > > callback
> > > > in the
> > > > struct? That way pdbg can just call sbefifo_op_istep() and the
> > > > libsbefifo
> > > > library can call something like sf_ctx->transport() which does
> > > > the
> > > > right thing
> > > > based on how the context is initialised.
> > >
> > > Well, the whole point was not to embed the sbefifo transport deep
> > > inside libsbefifo. But allow it to be layered by device tree.
> > >
> > > The idea of registering transport was a hack to get cronus
> > > working.
> > > But it's not as elegant as organizing the right targets in device
> > > tree
> > > and the transport then works automatically.
> >
> > This wouldn't change that though, the transport layer would just
> > initialise
> > libsbefifo differently as part of the _probe() function. Really
> > though I think
> > we are just arguing over two equally viable approaches:
> >
> > 1. Have the complete chip-ops (eg. getmem) defined in libsbefifo and
> > have
> > libsbefifo do a context-specific callback to handle the transport.
> >
> > 2. Make libsbefifo purely a request marshaling/formatting and
> > response parsing
> > library and have the complete chip-op implementation defined in
> > libpdbg.
> >
> > At the moment we seem to doing both approaches which is what is
> > leading to the
> > code duplication from libsbefifo in this patch. I think we should
> > either have
> > libsbefifo define just the _push()/_pull() operations and not the
> > overall chip-
> > ops (eg. by removing sbefifo_mem_get and the sbefifo_context struct
> > from
> > libsbefifo) or we should have libsbefifo take a call-back during
> > inititialisation of the sbefifo_context.
>
> The only reason I have not dropped the code from libsbefifo is to be
> able to test libsbefifo standalone. The patches switch pdbg approach
> from (1) to (2).
Right, this is the draw back of approach (2) in that you can't use the library
standalone. So why not just stick with (1)? You could leave the existing
implementation of sbefifo_operation() in libsbefifo and use that to initialise
the sbefifo_context in the standalone case for testing and use
cronus_sbefifo_operation or host_sbefifo_operation or whatever for use from
libpdbg.
> I can split libsbefifo.h into seaprate header files, one used by
> libpdbg and the other maintaining old style api for standalone testing.
This approach seems to end up with code duplication though which also reduces
test coverage. Is there something I'm missing that makes it impossible to do
something like:
rc = sctx->operation(sctx, (uint8_t *)msg, 6 * 4, cmd, &out, &out_len);
instead of:
rc = sbefifo_operation(sctx, (uint8_t *)msg, 6 * 4, cmd, &out, &out_len);
?
It seems that would solve a lot of the code duplication whilst allowing for
the same layered approach we have here using the device-tree.
- Alistair
> >
> > - Alistair
> >
> > > I guess if you see the other patch series, it might become apparent
> > > why
> > > I have taken this approach.
> > >
> > > > - Alistair
> > > >
> > > > > + free(msg);
> > > > > + if (rc)
> > > > > + return rc;
> > > > > +
> > > > > + rc = sbefifo_istep_execute_pull(out, out_len);
> > > > > + if (out)
> > > > > + free(out);
> > > > > +
> > > > > + return rc;
> > > > > }
> > > > >
> > > > > static int sbefifo_op_getmem(struct mem *sbefifo_mem,
> > > > > @@ -43,48 +60,45 @@ static int sbefifo_op_getmem(struct mem
> > > > > *sbefifo_mem,
> > > > > uint8_t block_size, bool ci)
> > > > > {
> > > > > struct sbefifo *sbefifo =
> > > > > target_to_sbefifo(sbefifo_mem-
> > > > > > target.parent);
> > > > > - uint8_t *out;
> > > > > - uint64_t start_addr, end_addr;
> > > > > - uint32_t align, offset, len;
> > > > > + uint8_t *msg, *out;
> > > > > + uint32_t msg_len, out_len, len;
> > > > > uint16_t flags;
> > > > > int rc;
> > > > >
> > > > > - align = 8;
> > > > > -
> > > > > - if (block_size && block_size != 8) {
> > > > > - PR_ERROR("sbefifo: Only 8 byte block sizes are
> > > > > supported\n");
> > > > > - return -1;
> > > > > - };
> > > > > -
> > > > > - start_addr = addr & (~(uint64_t)(align-1));
> > > > > - end_addr = (addr + size + (align-1)) &
> > > > > (~(uint64_t)(align-1));
> > > > > -
> > > > > - if (end_addr - start_addr > UINT32_MAX) {
> > > > > - PR_ERROR("sbefifo: size too large\n");
> > > > > - return -EINVAL;
> > > > > + if (size > UINT32_MAX) {
> > > > > + PR_ERROR("sbefifo: Invalid size for getmem\n");
> > > > > + return EINVAL;
> > > > > }
> > > > >
> > > > > - offset = addr - start_addr;
> > > > > - len = end_addr - start_addr;
> > > > > + len = size & 0xffffffff;
> > > > >
> > > > > - PR_NOTICE("sbefifo: getmem addr=0x%016" PRIx64 ",
> > > > > len=%u\n",
> > > > > - start_addr, len);
> > > > > + PR_NOTICE("sbefifo: getmem addr=0x%016" PRIx64 ",
> > > > > len=%u\n",
> > > > > addr, len);
> > > > >
> > > > > flags = SBEFIFO_MEMORY_FLAG_PROC;
> > > > > if (ci)
> > > > > flags |= SBEFIFO_MEMORY_FLAG_CI;
> > > > >
> > > > > - rc = sbefifo_mem_get(sbefifo->sf_ctx, start_addr, len,
> > > > > flags,
> > > > > &out);
> > > > > -
> > > > > - pdbg_progress_tick(len, len);
> > > > > + rc = sbefifo_mem_get_push(addr, len, flags, &msg,
> > > > > &msg_len);
> > > > > + if (rc)
> > > > > + return rc;
> > > > >
> > > > > + out_len = len + 4;
> > > > > + rc = sbefifo_operation(sbefifo->sf_ctx, msg, msg_len,
> > > > > &out,
> > > > > &out_len);
> > > > > + free(msg);
> > > > > if (rc)
> > > > > return rc;
> > > > >
> > > > > - memcpy(data, out+offset, size);
> > > > > - free(out);
> > > > > + rc = sbefifo_mem_get_pull(out, out_len, addr, size,
> > > > > flags,
> > > > > &msg);
> > > > > + if (out)
> > > > > + free(out);
> > > > >
> > > > > - return 0;
> > > > > + if (!rc) {
> > > > > + memcpy(data, msg, len);
> > > > > + free(msg);
> > > > > + pdbg_progress_tick(len, len);
> > > > > + }
> > > > > +
> > > > > + return rc;
> > > > > }
> > > > >
> > > > > static int sbefifo_op_putmem(struct mem *sbefifo_mem,
> > > > > @@ -92,30 +106,14 @@ static int sbefifo_op_putmem(struct mem
> > > > > *sbefifo_mem,
> > > > > uint8_t block_size, bool ci)
> > > > > {
> > > > > struct sbefifo *sbefifo =
> > > > > target_to_sbefifo(sbefifo_mem-
> > > > > > target.parent);
> > > > > - uint32_t align, len;
> > > > > + uint8_t *msg, *out;
> > > > > + uint32_t msg_len, out_len, len;
> > > > > uint16_t flags;
> > > > > int rc;
> > > > >
> > > > > - align = 8;
> > > > > -
> > > > > - if (block_size && block_size != 8) {
> > > > > - PR_ERROR("sbefifo: Only 8 byte block sizes are
> > > > > supported\n");
> > > > > - return -1;
> > > > > - };
> > > > > -
> > > > > - if (addr & (align-1)) {
> > > > > - PR_ERROR("sbefifo: Address must be aligned to
> > > > > %d
> > > > > bytes\n", align);
> > > > > - return -1;
> > > > > - }
> > > > > -
> > > > > - if (size & (align-1)) {
> > > > > - PR_ERROR("sbefifo: Data must be multiple of %d
> > > > > bytes\n", align);
> > > > > - return -1;
> > > > > - }
> > > > > -
> > > > > if (size > UINT32_MAX) {
> > > > > - PR_ERROR("sbefifo: size too large\n");
> > > > > - return -1;
> > > > > + PR_ERROR("sbefifo: Invalid size for putmem\n");
> > > > > + return EINVAL;
> > > > > }
> > > > >
> > > > > len = size & 0xffffffff;
> > > > > @@ -126,9 +124,22 @@ static int sbefifo_op_putmem(struct mem
> > > > > *sbefifo_mem,
> > > > > if (ci)
> > > > > flags |= SBEFIFO_MEMORY_FLAG_CI;
> > > > >
> > > > > - rc = sbefifo_mem_put(sbefifo->sf_ctx, addr, data, len,
> > > > > flags);
> > > > > + rc = sbefifo_mem_put_push(addr, data, len, flags, &msg,
> > > > > &msg_len);
> > > > > + if (rc)
> > > > > + return rc;
> > > > > +
> > > > > + out_len = 4;
> > > > > + rc = sbefifo_operation(sbefifo->sf_ctx, msg, msg_len,
> > > > > &out,
> > > > > &out_len);
> > > > > + free(msg);
> > > > > + if (rc)
> > > > > + return rc;
> > > > >
> > > > > - pdbg_progress_tick(len, len);
> > > > > + rc = sbefifo_mem_put_pull(out, out_len);
> > > > > + if (out)
> > > > > + free(out);
> > > > > +
> > > > > + if (!rc)
> > > > > + pdbg_progress_tick(len, len);
> > > > >
> > > > > return rc;
> > > > > }
> > > > > @@ -137,6 +148,9 @@ static int sbefifo_op_control(struct
> > > > > sbefifo
> > > > > *sbefifo,
> > > > > uint32_t core_id, uint32_t
> > > > > thread_id,
> > > > > uint32_t oper)
> > > > > {
> > > > > + uint8_t *msg, *out;
> > > > > + uint32_t msg_len, out_len;
> > > > > + int rc;
> > > > > uint8_t mode = 0;
> > > > >
> > > > > /* Enforce special-wakeup for thread stop and sreset */
> > > > > @@ -146,7 +160,21 @@ static int sbefifo_op_control(struct
> > > > > sbefifo
> > > > > *sbefifo,
> > > > >
> > > > > PR_NOTICE("sbefifo: control c:0x%x, t:0x%x, op:%u
> > > > > mode:%u\n",
> > > > > core_id,
> > > > thread_id, oper, mode);
> > > > >
> > > > > - return sbefifo_control_insn(sbefifo->sf_ctx, core_id &
> > > > > 0xff,
> > > > > thread_id &
> > > > 0xff, oper & 0xff, mode);
> > > > > + rc = sbefifo_control_insn_push(core_id & 0xff,
> > > > > thread_id &
> > > > > 0xff, oper & 0xff,
> > > > mode, &msg, &msg_len);
> > > > > + if (rc)
> > > > > + return rc;
> > > > > +
> > > > > + out_len = 0;
> > > > > + rc = sbefifo_operation(sbefifo->sf_ctx, msg, msg_len,
> > > > > &out,
> > > > > &out_len);
> > > > > + free(msg);
> > > > > + if (rc)
> > > > > + return rc;
> > > > > +
> > > > > + rc = sbefifo_control_insn_pull(out, out_len);
> > > > > + if (out)
> > > > > + free(out);
> > > > > +
> > > > > + return rc;
> > > > > }
> > > > >
> > > > > static int sbefifo_op_thread_start(struct sbefifo *sbefifo,
> > > > >
> > > >
> > > >
> > >
> > > Amitay.
> > >
> >
> >
> >
>
> Amitay.
>
More information about the Pdbg
mailing list