[Pdbg] [PATCH 14/17] libpdbg: Expand sbefifo data marshalling and calling chip-op
Amitay Isaacs
amitay at ozlabs.org
Tue Nov 5 17:31:05 AEDT 2019
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).
I can split libsbefifo.h into seaprate header files, one used by
libpdbg and the other maintaining old style api for standalone testing.
>
> - 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.
--
When you have been wronged, a poor memory is your best response.
More information about the Pdbg
mailing list