qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v2 1/2] pl330: initial version


From: Peter Maydell
Subject: Re: [Qemu-devel] [PATCH v2 1/2] pl330: initial version
Date: Tue, 10 Apr 2012 18:50:44 +0100

On 30 March 2012 08:58, Peter A. G. Crosthwaite
<address@hidden> wrote:
> Device model for Primecell PL330 dma controller.
>
> Signed-off-by: Peter A. G. Crosthwaite <address@hidden>
> Signed-off-by: Kirill Batuzov <address@hidden>
> ---
> changed from v1:
> GPLv2 license
> some code formatting fixes
>
>  MAINTAINERS     |    1 +
>  Makefile.target |    1 +
>  hw/pl330.c      | 1403 
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 1405 insertions(+), 0 deletions(-)
>  create mode 100644 hw/pl330.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index f83d07c2..4bdf93a 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -257,6 +257,7 @@ S: Maintained
>  F: hw/xilinx_zynq.c
>  F: hw/zynq_slcr.c
>  F: hw/cadence_*
> +F: hw/pl330.c

I think this sort of generic component is probably best not listed
in the MAINTAINERS section for an individual board.

>  CRIS Machines
>  -------------
> diff --git a/Makefile.target b/Makefile.target
> index 44b2e83..2191103 100644
> --- a/Makefile.target
> +++ b/Makefile.target
> @@ -362,6 +362,7 @@ obj-arm-y += cadence_uart.o
>  obj-arm-y += cadence_ttc.o
>  obj-arm-y += cadence_gem.o
>  obj-arm-y += xilinx_zynq.o zynq_slcr.o
> +obj-arm-y += pl330.o
>  obj-arm-y += realview_gic.o realview.o arm_sysctl.o arm11mpcore.o a9mpcore.o
>  obj-arm-y += exynos4210_gic.o exynos4210_combiner.o exynos4210.o
>  obj-arm-y += exynos4_boards.o exynos4210_uart.o exynos4210_pwm.o
> diff --git a/hw/pl330.c b/hw/pl330.c
> new file mode 100644
> index 0000000..0ea8bb6
> --- /dev/null
> +++ b/hw/pl330.c
> @@ -0,0 +1,1403 @@
> +/*
> + * ARM PrimeCell PL330 DMA Controller
> + *
> + * Copyright (c) 2009 Samsung Electronics.
> + * Contributed by Kirill Batuzov <address@hidden>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; version 2.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include "sysbus.h"
> +#include "qemu-timer.h"
> +
> +#ifdef PL330_ERR_DEBUG
> +#define DB_PRINT(...) do { \
> +    fprintf(stderr,  ": %s: ", __func__); \
> +    fprintf(stderr, ## __VA_ARGS__); \
> +    } while (0);
> +#else
> +    #define DB_PRINT(...)
> +#endif
> +
> +#define PL330_CHAN_NUM              8
> +#define PL330_PERIPH_NUM            32
> +#define PL330_MAX_BURST_LEN         128
> +#define PL330_INSN_MAXSIZE          6
> +
> +#define PL330FIFO_OK               0
> +#define PL330FIFO_STALL            1
> +#define PL330FIFO_ERR              (-1)

Extra '_' between PL330 and FIFO, please.

> +
> +#define PL330_FAULT_UNDEF_INSTR     (1 <<  0)
> +#define PL330_FAULT_OPERAND_INVALID (1 <<  1)
> +#define PL330_FAULT_DMAGO_ER        (1 <<  4)

In the TRM these are all "_ERR" -- can we follow the same
naming system here, please?

> +#define PL330_FAULT_EVENT_ER        (1 <<  5)
> +#define PL330_FAULT_CH_PERIPH_ER    (1 <<  6)
> +#define PL330_FAULT_CH_RDWR_ER      (1 <<  7)
> +#define PL330_FAULT_MFIFO_ER        (1 << 12)
> +#define PL330_FAULT_FIFOEMPTY_ER    (1 << 13)

I think the TRM calls this st_data_unavailable ?

> +#define PL330_FAULT_INSTR_FETCH_ER  (1 << 16)
> +#define PL330_FAULT_DATA_WRITE_ER   (1 << 17)
> +#define PL330_FAULT_DATA_READ_ER    (1 << 18)
> +#define PL330_FAULT_DBG_INSTR       (1 << 30)
> +#define PL330_FAULT_LOCKUP_ER       (1 << 31)
> +
> +#define PL330_UNTAGGED              0xff
> +
> +#define PL330_SINGLE                0x0
> +#define PL330_BURST                 0x1
> +
> +#define PL330_WATCHDOG_LIMIT        1024
> +
> +/* IOMEM mapped registers */
> +#define PL330_REG_DS        0x000

DSR, not DS.

> +#define PL330_REG_DPC       0x004
> +#define PL330_REG_INTEN     0x020
> +#define PL330_REG_ES        0x024

INT_EVENT_RIS

> +#define PL330_REG_INTSTATUS 0x028

INTMIS.

Etc. Use the names in table 3.1.

> +#define PL330_REG_INTCLR    0x02C
> +#define PL330_REG_FSM       0x030
> +#define PL330_REG_FSC       0x034
> +#define PL330_REG_FTM       0x038
> +#define PL330_REG_FTC_BASE  0x040
> +#define PL330_REG_CS_BASE   0x100
> +#define PL330_REG_CPC_BASE  0x104
> +#define PL330_REG_CHANCTRL  0x400
> +#define PL330_REG_DBGSTATUS 0xD00
> +#define PL330_REG_DBGCMD    0xD04
> +#define PL330_REG_DBGINST0  0xD08
> +#define PL330_REG_DBGINST1  0xD0C
> +#define PL330_REG_CONFIG    0xE00
> +#define PL330_REG_ID        0xFE0
> +
> +#define PL330_IOMEM_SIZE    0x1000
> +
> +static const uint32_t pl330_id[] = {
> +    0x30, 0x13, 0x04, 0x00, 0xB1, 0x05, 0xF0, 0x0D
> +};
> +
> +/* DMA chanel states as they are described in PL330 Technical Reference 
> Manual
> + * Most of them will not be used in emulation.
> + */
> +typedef enum  {
> +    pl330_chan_stopped = 0,
> +    pl330_chan_executing = 1,
> +    pl330_chan_completing,
> +    pl330_chan_waiting_periph,
> +    pl330_chan_at_barrier,
> +    pl330_chan_waiting_event = 4,
> +    pl330_chan_updating_pc = 3,
> +    pl330_chan_cache_miss,
> +    pl330_chan_fault_completing,
> +    pl330_chan_fault = 15,
> +    pl330_chan_killing
> +} PL330ChanState;

This looks pretty weird: it doesn't match up with the set
of values in the description of the channel status register
'channel status' field in section 3.3.11.

> +
> +typedef struct PL330 PL330;
> +
> +typedef struct PL330Chan {
> +    target_phys_addr_t src;
> +    target_phys_addr_t dst;
> +    target_phys_addr_t pc;
> +    uint32_t control;
> +    uint32_t status;
> +    uint32_t lc[2];
> +    uint32_t fault_type;
> +
> +    uint8_t ns;
> +    uint8_t is_manager;
> +    uint8_t request_flag;
> +    uint8_t wakeup;
> +    uint8_t wfp_sbp;

Some of these should probably be booleans.

> +
> +    PL330ChanState state;
> +    uint8_t stall;
> +
> +    PL330 *parent;
> +    uint8_t tag;
> +    uint32_t watchdog_timer;
> +} PL330Chan;
> +
> +typedef struct PL330Fifo {
> +    uint8_t *buf;
> +    uint8_t *tag;
> +    int s, t;
> +    int buf_size;
> +} PL330Fifo;
> +
> +typedef struct PL330QueueEntry {
> +    PL330Chan *c;
> +    target_phys_addr_t addr;
> +    int len;
> +    int n;
> +    int inc;
> +    int z;
> +    uint8_t tag;
> +    uint8_t seqn;
> +} PL330QueueEntry;
> +
> +typedef struct PL330Queue {
> +    PL330QueueEntry *queue;
> +    int queue_size;
> +    uint8_t *lo_seqn;
> +    uint8_t *hi_seqn;
> +} PL330Queue;
> +
> +struct PL330 {
> +    SysBusDevice busdev;
> +    MemoryRegion iomem;
> +
> +    PL330Chan manager;
> +    PL330Chan *chan;
> +    PL330Fifo fifo;
> +    PL330Queue read_queue;
> +    PL330Queue write_queue;
> +
> +    /* Config registers. cfg[5] = CfgDn. */
> +    uint32_t inten;
> +    uint32_t int_status;
> +    uint32_t ev_status;
> +    uint32_t cfg[6];
> +    uint32_t dbg[2];
> +    uint8_t debug_status;
> +
> +    qemu_irq irq_abort;
> +    qemu_irq *irq;
> +
> +    uint8_t num_faulting;
> +
> +    int chan_num;
> +    int periph_num;
> +    unsigned int event_num;
> +
> +    QEMUTimer *timer; /* is used for restore dma. */
> +    int8_t periph_busy[PL330_PERIPH_NUM];
> +};
> +
> +typedef struct PL330InsnDesc {
> +    /* OPCODE of the instruction */
> +    uint8_t opcode;
> +    /* Mask so we can select several sibling instructions, such as
> +       DMALD, DMALDS and DMALDB */
> +    uint8_t opmask;
> +    /* Size of instruction in bytes */
> +    uint8_t size;
> +    /* Interpreter */
> +    void (*exec)(PL330Chan *, uint8_t opcode, uint8_t *args, int len);
> +} PL330InsnDesc;
> +
> +
> +/* MFIFO Implementation
> + *
> + * MFIFO is implemented as a cyclic buffer of BUF_SIZE size. Tagged bytes are
> + * stored in this buffer. Data is stored in BUF field, tags - in the
> + * corresponding array elemnets of TAG field.
> + */
> +
> +/* Initialize queue. */
> +static void PL330Fifo_init(PL330Fifo *s, uint32_t size)

Function names should be lowercase, so pl330_fifo_init().
More generally, can you make sure they're consistently named
according to whether they take a PL330Fifo*, PL330*, PL330Queue*
or PL330Chan* ? The fifo ones seem to mostly use a consistent
prefix, but the channel/whole device/queue functions are less so.

> +{
> +    s->buf = g_malloc0(size * sizeof(uint8_t));
> +    s->tag = g_malloc0(size * sizeof(uint8_t));

sizeof(uint8_t) is pointless.

> +    s->buf_size = size;
> +}
> +
> +/* Cyclic increment */
> +static inline int PL330Fifo_inc(int x, int size)
> +{
> +    return (x + 1) % size;
> +}

Every call to this function passes s->buf_size as the size;
I think it would be better to have
 pl330_fifo_inc(PL330Fifo *s, int x).

> +
> +/* Number of empty bytes in MFIFO */
> +static inline int PL330Fifo_num_free(PL330Fifo *s)
> +{
> +    if (s->t < s->s) {
> +        return s->s - s->t;
> +    } else {
> +        return s->buf_size - s->t + s->s;
> +    }
> +}
> +
> +/* Number of bytes in MFIFO */
> +static inline int PL330Fifo_num_used(PL330Fifo *s)
> +{
> +    if (s->t >= s->s) {
> +        return s->t - s->s;
> +    } else {
> +        return s->buf_size - s->s + s->t;
> +    }
> +}

Isn't this just s->buf_size - PL330Fifo_num_free(s) ?
In any case I can't see it used anywhere so you should just drop it.

> +
> +/* Push LEN bytes of data stored in BUF to MFIFO and tag it with TAG.
> + * Zero returned on success, PL330FIFO_STALL if there is no enough free
> + * space in MFIFO to store requested amount of data. If push was unsaccessful
> + * no data is stored to MFIFO.
> + */
> +static int PL330Fifo_push(PL330Fifo *s, uint8_t *buf, int len, uint8_t tag)
> +{
> +    int i;
> +    int old_s, old_t;
> +
> +    old_s = s->s;
> +    old_t = s->t;
> +    for (i = 0; i < len; i++) {
> +        if (PL330Fifo_inc(s->t, s->buf_size) != s->s) {

We always do one push for every byte of data, so you can tell
in advance whether we're going to run out of space and fail
then. There's no need for this check-and-rollback code.

> +            s->buf[s->t] = buf[i];
> +            s->tag[s->t] = tag;
> +            s->t = PL330Fifo_inc(s->t, s->buf_size);
> +        } else {
> +            /* Rollback transaction */
> +            s->s = old_s;
> +            s->t = old_t;
> +            return PL330FIFO_STALL;
> +        }
> +    }
> +    return PL330FIFO_OK;
> +}
> +
> +/* Get LEN bytes of data from MFIFO and store it to BUF. Tag value of each
> + * byte is veryfied. Zero returned on success, PL330FIFO_ERR on tag missmatch
> + * and PL330FIFO_STALL if there is no enough data in MFIFO. If get was
> + * unsaccessful no data is removed from MFIFO.
> + */
> +static int PL330Fifo_get(PL330Fifo *s, uint8_t *buf, int len, uint8_t tag)
> +{
> +    int i, ret;
> +    int old_s, old_t;
> +
> +    old_s = s->s;
> +    old_t = s->t;
> +    for (i = 0; i < len; i++) {
> +        if (s->t != s->s && s->tag[s->s] == tag) {
> +            buf[i] = s->buf[s->s];
> +            s->s = PL330Fifo_inc(s->s, s->buf_size);
> +        } else {
> +            /* Rollback transaction */
> +            ret = (s->t == s->s) ? PL330FIFO_STALL : PL330FIFO_ERR;
> +            s->s = old_s;
> +            s->t = old_t;
> +            return ret;
> +        }
> +    }

Again, you might as well do the length and tag checks
early and avoid having to do rollback.

(If this function is a performance bottleneck then you could also
do the actual data copies with memcpy rather than a loop, but
that might be premature optimisation.)

> +    return PL330FIFO_OK;
> +}
> +
> +/* Reset MFIFO. This completely erases all data in it. */
> +static inline void PL330Fifo_reset(PL330Fifo *s)
> +{
> +    s->s = 0;
> +    s->t = 0;
> +}
> +
> +/* Return tag of the first byte stored in MFIFO. If MFIFO is empty
> +   PL330_UNTAGGED is returned. */
> +static inline uint8_t PL330Fifo_tag(PL330Fifo *s)
> +{
> +    return (s->t == s->s) ? PL330_UNTAGGED : s->tag[s->s];
> +}
> +
> +/* Returns non-zero if tag TAG is present in fifo or zero otherwise */
> +static int PL330Fifo_has_tag(PL330Fifo *s, uint8_t tag)
> +{
> +    int i;
> +
> +    for (i = s->s; i != s->t; i = PL330Fifo_inc(i, s->buf_size)) {
> +        if (s->tag[i] == tag) {
> +            return 1;
> +        }
> +    }
> +    return 0;
> +}
> +
> +/* Remove all entry tagged with TAG from MFIFO */

"entries"

> +static void PL330Fifo_tagged_remove(PL330Fifo *s, uint8_t tag)
> +{
> +    int i, t;
> +
> +    t = s->s;
> +    for (i = s->s; i != s->t; i = PL330Fifo_inc(i, s->buf_size)) {
> +        if (s->tag[i] != tag) {
> +            s->buf[t] = s->buf[i];
> +            s->tag[t] = s->tag[i];
> +            t++;

Shouldn't this be using PL330Fifo_inc()?

> +        }
> +    }
> +    s->t = t;
> +}
> +
> +/* Read-Write Queue implementation
> + *
> + * A Read-Write Queue stores up to QUEUE_SIZE instructions (loads or stores).
> + * Each instructions is described by source (for loads) or destination (for

"instruction"

> + * stores) address ADDR, width of data to be loaded/stored LEN, number of
> + * stores/loads to be performed N, INC bit, Z bit and TAG to identify channel
> + * this instruction belongs to. Queue does not store any information about
> + * nature of the instruction: is it load or store. PL330 has different queues
> + * for loads and stores so this is already known at the top level where it
> + * matters.
> + *
> + * Queue works as FIFO for instructions with equivalent tags, but can issue
> + * instructions with different tags in arbitrary order. SEQN field attached 
> to
> + * each instruction helps to achieve this. For each TAG queue contains
> + * instructions with consecutive SEQN values ranged from LO_SEQN[TAG] to

"ranging", I think.

> + * HI_SEQN[TAG]-1 inclusive. SEQN is 8-bit unsigned integer, so SEQN=255 is
> + * followed by SEQN=0.
> + *
> + * Z bit indicates that zeroes should be stored. No MFIFO fetches are 
> performed
> + * in this case.
> + */
> +
> +static void PL330Queue_reset(PL330Queue *s)
> +{
> +    int i;
> +
> +    for (i = 0; i < s->queue_size; i++) {
> +        s->queue[i].tag = PL330_UNTAGGED;
> +    }
> +}
> +
> +/* Initialize queue */
> +static void PL330Queue_init(PL330Queue *s, int size, int channum)
> +{
> +    s->queue = (PL330QueueEntry *)
> +        g_malloc0(size * sizeof(PL330QueueEntry));
> +    s->lo_seqn = (uint8_t *)g_malloc0(channum * sizeof(uint8_t));
> +    s->hi_seqn = (uint8_t *)g_malloc0(channum * sizeof(uint8_t));

sizeof(uint8_t) still pointless.

> +    s->queue_size = size;
> +}
> +
> +/* Returns pointer to an empty slot or NULL if queue is full */
> +static PL330QueueEntry *PL330Queue_find_empty(PL330Queue *s)
> +{
> +    int i;
> +
> +    for (i = 0; i < s->queue_size; i++) {
> +        if (s->queue[i].tag == PL330_UNTAGGED) {
> +            return &s->queue[i];
> +        }
> +    }
> +    return NULL;
> +}
> +
> +/* Puts instruction to queue.

"Put instruction in queue".


> +   Return value:
> +     - zero - OK
> +     - non-zero - queue is full */
> +static int pl330_insn_to_queue(PL330Queue *s, target_phys_addr_t addr,
> +                               int len, int n, int inc, int z, uint8_t tag,
> +                               PL330Chan *c)

Call this "pl330_queue_add_insn", please. "pl330_insn_to_queue" sounds
like it ought to be returning an insn, not taking one.

> +{
> +    PL330QueueEntry *entry = PL330Queue_find_empty(s);
> +
> +    if (!entry) {
> +        return 1;
> +    }
> +    entry->c = c;
> +    entry->tag = tag;
> +    entry->addr = addr;
> +    entry->len = len;
> +    entry->n = n;
> +    entry->z = z;
> +    entry->inc = inc;
> +    entry->seqn = s->hi_seqn[tag];
> +    s->hi_seqn[tag]++;
> +    return 0;
> +}
> +
> +/* Returns a pointer to queue slot containing instruction which satisfies
> +   following conditions:
> +    - it has valid tag value (not PL330_UNTAGGED)
> +    - it can be issued without violating queue logic (see above)
> +    - if TAG argument is not PL330_UNTAGGED this instruction has tag value
> +      equivalent to the argument TAG value.
> +   If such instruction cannot be found NULL is returned. */
> +static PL330QueueEntry *PL330Queue_find_insn(PL330Queue *s, uint8_t tag)
> +{
> +    int i;
> +
> +    for (i = 0; i < s->queue_size; i++) {
> +        if (s->queue[i].tag != PL330_UNTAGGED) {
> +            if (s->queue[i].seqn == s->lo_seqn[s->queue[i].tag] && (
> +                    s->queue[i].tag == tag ||
> +                    tag == PL330_UNTAGGED ||
> +                    s->queue[i].z
> +                    )) {

I think the choice of linebreak locations here is pretty ugly...

> +                return &s->queue[i];
> +            }
> +        }
> +    }
> +    return NULL;
> +}
> +
> +/* Removes instruction from queue. */
> +static inline void pl330_insn_from_queue(PL330Queue *s, PL330QueueEntry *e)
> +{
> +    s->lo_seqn[e->tag]++;
> +    e->tag = PL330_UNTAGGED;
> +}
> +
> +/* Removes all instructions tagged with TAG from queue. */
> +static inline void pl330_tag_from_queue(PL330Queue *s, uint8_t tag)
> +{
> +    int i;
> +
> +    for (i = 0; i < s->queue_size; i++) {
> +        if (s->queue[i].tag == tag) {
> +            s->queue[i].tag = PL330_UNTAGGED;
> +        }
> +    }
> +}

Do we not need to reset lo_seqn/hi_seqn at all here?

> +
> +
> +/* DMA instruction execution engine */
> +
> +/* Moves DMA channel to the FAULT state and updates it's status. */

"its".

> +static inline void pl330_fault(PL330Chan *ch, uint32_t flags)
> +{
> +    DB_PRINT("ch: %p, flags: %x\n", ch, flags);
> +    ch->fault_type |= flags;
> +    if (ch->state == pl330_chan_fault) {
> +        return;
> +    }
> +    ch->state = pl330_chan_fault;
> +    ch->parent->num_faulting++;
> +    if (ch->parent->num_faulting == 1) {
> +        DB_PRINT("abort interrupt raised\n");
> +        qemu_irq_raise(ch->parent->irq_abort);
> +    }
> +}
> +
> +/*
> + * For information about instructions see PL330 Technical Reference Manual.
> + *
> + * Arguments:
> + *   CH - chanel executing the instruction

"channel"

> + *   OPCODE - opcode
> + *   ARGS - array of 8-bit arguments
> + *   LEN - number of elements in ARGS array
> + */
> +static void pl330_dmaaddh(PL330Chan *ch, uint8_t opcode, uint8_t *args, int 
> len)
> +{
> +    uint16_t im = (((uint16_t)args[0]) << 8) | ((uint16_t)args[1]);
> +    uint8_t ra = (opcode >> 1) & 1;
> +
> +    if (ra) {
> +        ch->dst += im;
> +    } else {
> +        ch->src += im;
> +    }
> +}

DMAADDH can only be used in a DMA channel thread. It would
probably be a good idea to include "ok for channel thread/master
thread/both" element in the insn table so you can do the check
there rather than ad-hoc in each function. (Also if you're in a
position to check whether these should abort as operand_invalid
or undef_instr that would be useful to know.)

> +
> +static void pl330_dmaend(PL330Chan *ch, uint8_t opcode,
> +                         uint8_t *args, int len)
> +{
> +    if (ch->state == pl330_chan_executing && !ch->is_manager) {
> +        /* Wait for all transfers to complete */
> +        if (PL330Fifo_has_tag(&ch->parent->fifo, ch->tag) ||
> +            PL330Queue_find_insn(&ch->parent->read_queue, ch->tag) != NULL ||
> +            PL330Queue_find_insn(&ch->parent->write_queue, ch->tag) != NULL) 
> {
> +
> +            ch->stall = 1;
> +            return;
> +        }
> +    }
> +    PL330Fifo_tagged_remove(&ch->parent->fifo, ch->tag);
> +    pl330_tag_from_queue(&ch->parent->read_queue, ch->tag);
> +    pl330_tag_from_queue(&ch->parent->write_queue, ch->tag);
> +    ch->state = pl330_chan_stopped;
> +}
> +
> +static void pl330_dmaflushp(PL330Chan *ch, uint8_t opcode,
> +                                            uint8_t *args, int len)
> +{
> +    uint8_t periph_id;
> +
> +    if (args[0] & 7) {
> +        pl330_fault(ch, PL330_FAULT_OPERAND_INVALID);
> +        return;
> +    }
> +    periph_id = (args[0] >> 3) & 0x1f;
> +    if (periph_id >= ch->parent->periph_num) {
> +        pl330_fault(ch, PL330_FAULT_OPERAND_INVALID);
> +        return;
> +    }
> +    if (ch->ns > ((ch->parent->cfg[4] & (1 << periph_id)) >> periph_id)) {
> +        pl330_fault(ch, PL330_FAULT_CH_PERIPH_ER);
> +        return;
> +    }
> +    /* Do nothing */
> +}
> +
> +static void pl330_dmago(PL330Chan *ch, uint8_t opcode, uint8_t *args, int 
> len)
> +{
> +    uint8_t chan_id;
> +    uint8_t ns;
> +    uint32_t pc;
> +    PL330Chan *s;
> +
> +    DB_PRINT("\n");
> +
> +    if (!ch->is_manager) {
> +        /* TODO: what error actually is it? */
> +        pl330_fault(ch, PL330_FAULT_OPERAND_INVALID);
> +        return;
> +    }
> +    ns = (opcode >> 1) & 1;
> +    chan_id = args[0] & 7;
> +    if ((args[0] >> 3)) {
> +        pl330_fault(ch, PL330_FAULT_OPERAND_INVALID);
> +        return;
> +    }
> +    if (chan_id >= ch->parent->chan_num) {
> +        pl330_fault(ch, PL330_FAULT_OPERAND_INVALID);
> +        return;
> +    }
> +    pc = (((uint32_t)args[4]) << 24) | (((uint32_t)args[3]) << 16) |
> +         (((uint32_t)args[2]) << 8)  | (((uint32_t)args[1]));
> +    if (ch->parent->chan[chan_id].state != pl330_chan_stopped) {
> +        /* TODO: what error actually is it? */
> +        pl330_fault(ch, PL330_FAULT_OPERAND_INVALID);
> +        return;
> +    }
> +    if (ch->ns > ns) {
> +        pl330_fault(ch, PL330_FAULT_DMAGO_ER);
> +        return;
> +    }
> +    s = &ch->parent->chan[chan_id];
> +    s->ns = ns;
> +    s->pc = pc;
> +    s->state = pl330_chan_executing;
> +}
> +
> +static void pl330_dmald(PL330Chan *ch, uint8_t opcode, uint8_t *args, int 
> len)
> +{
> +    uint8_t bs = opcode & 3;
> +    uint32_t size, num, inc;
> +
> +    if (bs == 2) {
> +        pl330_fault(ch, PL330_FAULT_OPERAND_INVALID);
> +        return;
> +    }
> +    if ((bs == 1 && ch->request_flag == PL330_BURST) ||
> +        (bs == 3 && ch->request_flag == PL330_SINGLE)) {
> +        /* Perform NOP */
> +        return;
> +    }
> +    num = ((ch->control >> 4) & 0xf) + 1;
> +    size = (uint32_t)1 << ((ch->control >> 1) & 0x7);
> +    inc = ch->control & 1;
> +    ch->src &= ~(size - 1);
> +    ch->stall = pl330_insn_to_queue(&ch->parent->read_queue, ch->src,
> +                                    size, num, inc, 0, ch->tag, ch);
> +    if (inc & !ch->stall) {
> +        ch->src += size * num;
> +    }
> +}

This doesn't seem to be implementing the load-lock ? (see TRM
section 2.11.4)

> +
> +static void pl330_dmaldp(PL330Chan *ch, uint8_t opcode, uint8_t *args, int 
> len)
> +{
> +    uint8_t periph_id;
> +
> +    if (args[0] & 7) {
> +        pl330_fault(ch, PL330_FAULT_OPERAND_INVALID);
> +        return;
> +    }
> +    periph_id = (args[0] >> 3) & 0x1f;
> +    if (periph_id >= ch->parent->periph_num) {
> +        pl330_fault(ch, PL330_FAULT_OPERAND_INVALID);
> +        return;
> +    }
> +    if (ch->ns > ((ch->parent->cfg[4] & (1 << periph_id)) >> periph_id)) {
> +        pl330_fault(ch, PL330_FAULT_CH_PERIPH_ER);
> +        return;
> +    }
> +    pl330_dmald(ch, opcode, args, len);
> +}
> +
> +static void pl330_dmalp(PL330Chan *ch, uint8_t opcode, uint8_t *args, int 
> len)
> +{
> +    uint8_t lc = (opcode & 2) >> 1;
> +
> +    ch->lc[lc] = args[0];
> +}
> +
> +static void pl330_dmalpend(PL330Chan *ch, uint8_t opcode,
> +                                    uint8_t *args, int len)
> +{
> +    uint8_t nf = (opcode & 0x10) >> 4;
> +    uint8_t bs = opcode & 3;
> +    uint8_t lc = (opcode & 4) >> 2;
> +
> +    if (bs == 2) {
> +        pl330_fault(ch, PL330_FAULT_OPERAND_INVALID);
> +        return;
> +    }
> +    if ((bs == 1 && ch->request_flag == PL330_BURST) ||
> +        (bs == 3 && ch->request_flag == PL330_SINGLE)) {
> +        /* Perform NOP */
> +        return;
> +    }
> +    if (!nf || ch->lc[lc]) {
> +        if (nf) {
> +            ch->lc[lc]--;
> +        }
> +        ch->pc -= args[0];
> +        ch->pc -= len + 1;
> +        /* "ch->pc -= args[0] + len + 1" is incorrect when args[0] == 256 */
> +    }
> +}
> +
> +static void pl330_dmakill(PL330Chan *ch, uint8_t opcode, uint8_t *args, int 
> len)

This function is out of alphabetical order.

> +{
> +    if (ch->state == pl330_chan_fault ||
> +        ch->state == pl330_chan_fault_completing) {
> +        /* This is the only way for chanel from faulting state */

"for a channel to leave the faulting state".

> +        ch->fault_type = 0;
> +        ch->parent->num_faulting--;
> +        if (ch->parent->num_faulting == 0) {
> +            DB_PRINT("abort interrupt lowered\n");
> +            qemu_irq_lower(ch->parent->irq_abort);
> +        }
> +    }
> +    ch->state = pl330_chan_killing;
> +    PL330Fifo_tagged_remove(&ch->parent->fifo, ch->tag);
> +    pl330_tag_from_queue(&ch->parent->read_queue, ch->tag);
> +    pl330_tag_from_queue(&ch->parent->write_queue, ch->tag);
> +    ch->state = pl330_chan_stopped;
> +}
> +
> +static void pl330_dmamov(PL330Chan *ch, uint8_t opcode, uint8_t *args, int 
> len)
> +{
> +    uint8_t rd = args[0] & 7;
> +    uint32_t im;
> +
> +    if ((args[0] >> 3)) {
> +        pl330_fault(ch, PL330_FAULT_OPERAND_INVALID);
> +        return;
> +    }
> +    im = (((uint32_t)args[4]) << 24) | (((uint32_t)args[3]) << 16) |
> +         (((uint32_t)args[2]) << 8)  | (((uint32_t)args[1]));
> +    switch (rd) {
> +    case 0:
> +        ch->src = im;
> +        break;
> +    case 1:
> +        ch->control = im;
> +        break;
> +    case 2:
> +        ch->dst = im;
> +        break;
> +    default:
> +        pl330_fault(ch, PL330_FAULT_OPERAND_INVALID);
> +        return;
> +    }
> +}
> +
> +static void pl330_dmanop(PL330Chan *ch, uint8_t opcode,
> +                         uint8_t *args, int len)
> +{
> +    /* NOP is NOP. */
> +}
> +
> +static void pl330_dmarmb(PL330Chan *ch, uint8_t opcode, uint8_t *args, int 
> len)
> +{
> +    /* Do nothing. Since we do not emulate AXI Bus transactions there is no
> +       stalls. So we are allways on barrier. */

However we do still implement the read and write queues. I think
you need to stall to avoid the following hazard:

 * channel issues a DMALD, which goes into the read queue
 * channel does DMARMB
 * channel issues one DMAST, which goes into the write queue

If the read queue was pretty full when the DMALD was issued
then there's a good chance the load won't have actually been
pulled out of the queue by pl330_exec_cycle before the DMAST,
and if the write queue is empty at that point, then the
"handle one insn per channel then one read queue entry then
one write queue entry" logic means the store might happen
before the loads, in violation of the barrier.

So we need to stall until there aren't any more loads for this
channel in the read queue.

> +}
> +
> +static void pl330_dmasev(PL330Chan *ch, uint8_t opcode, uint8_t *args, int 
> len)
> +{
> +    uint8_t ev_id;
> +
> +    if (args[0] & 7) {
> +        pl330_fault(ch, PL330_FAULT_OPERAND_INVALID);
> +        return;
> +    }
> +    ev_id = (args[0] >> 3) & 0x1f;
> +    if (ev_id >= ch->parent->event_num) {
> +        pl330_fault(ch, PL330_FAULT_OPERAND_INVALID);
> +        return;
> +    }
> +    if (ch->ns > ((ch->parent->cfg[3] & (1 << ev_id)) >> ev_id)) {

ns is really just a boolean. This is a horribly opaque way of
testing it against bit X in the cfg[3] register. Try:
  if (ch->ns && !(ch->parent->cfg[3] & (1 << ev_id)) {
      /* Non-secure channel attempted to set a secure event */

Similar remarks apply about similar code fragments elsewhere.


> +        pl330_fault(ch, PL330_FAULT_EVENT_ER);
> +        return;
> +    }
> +    if (ch->parent->inten & (1 << ev_id)) {
> +        ch->parent->int_status |= (1 << ev_id);
> +        DB_PRINT("event interrupt raised %d\n", ev_id);
> +        qemu_irq_raise(ch->parent->irq[ev_id]);
> +    } else {
> +        ch->parent->ev_status |= (1 << ev_id);
> +    }
> +}

As Mitsyanko points out, your sev/wfe implementation appears to
be missing some piecese.

> +
> +static void pl330_dmast(PL330Chan *ch, uint8_t opcode, uint8_t *args, int 
> len)
> +{
> +    uint8_t bs = opcode & 3;
> +    uint32_t size, num, inc;
> +
> +    if (bs == 2) {
> +        pl330_fault(ch, PL330_FAULT_OPERAND_INVALID);
> +        return;
> +    }
> +    if ((bs == 1 && ch->request_flag == PL330_BURST) ||
> +        (bs == 3 && ch->request_flag == PL330_SINGLE)) {
> +        /* Perform NOP */
> +        return;
> +    }
> +    num = ((ch->control >> 18) & 0xf) + 1;
> +    size = (uint32_t)1 << ((ch->control >> 15) & 0x7);
> +    inc = (ch->control >> 14) & 1;
> +    ch->dst &= ~(size - 1);
> +    ch->stall = pl330_insn_to_queue(&ch->parent->write_queue, ch->dst,
> +                                    size, num, inc, 0, ch->tag, ch);
> +    if (inc) {
> +        ch->dst += size * num;
> +    }
> +}
> +
> +static void pl330_dmastp(PL330Chan *ch, uint8_t opcode,
> +                         uint8_t *args, int len)
> +{
> +    uint8_t periph_id;
> +
> +    if (args[0] & 7) {
> +        pl330_fault(ch, PL330_FAULT_OPERAND_INVALID);
> +        return;
> +    }
> +    periph_id = (args[0] >> 3) & 0x1f;
> +    if (periph_id >= ch->parent->periph_num) {
> +        pl330_fault(ch, PL330_FAULT_OPERAND_INVALID);
> +        return;
> +    }
> +    if (ch->ns > ((ch->parent->cfg[4] & (1 << periph_id)) >> periph_id)) {
> +        pl330_fault(ch, PL330_FAULT_CH_PERIPH_ER);
> +        return;
> +    }
> +    pl330_dmast(ch, opcode, args, len);
> +}
> +
> +static void pl330_dmastz(PL330Chan *ch, uint8_t opcode,
> +                         uint8_t *args, int len)
> +{
> +    uint32_t size, num, inc;
> +
> +    num = ((ch->control >> 18) & 0xf) + 1;
> +    size = (uint32_t)1 << ((ch->control >> 15) & 0x7);
> +    inc = (ch->control >> 14) & 1;
> +    ch->stall = pl330_insn_to_queue(&ch->parent->write_queue, ch->dst,
> +                                    size, num, inc, 1, ch->tag, ch);
> +    if (inc) {
> +        ch->dst += size * num;
> +    }
> +}
> +
> +static void pl330_dmawfe(PL330Chan *ch, uint8_t opcode,
> +                         uint8_t *args, int len)
> +{
> +    uint8_t ev_id;
> +
> +    if (args[0] & 5) {
> +        pl330_fault(ch, PL330_FAULT_OPERAND_INVALID);
> +        return;
> +    }
> +    ev_id = (args[0] >> 3) & 0x1f;
> +    if (ev_id >= ch->parent->event_num) {
> +        pl330_fault(ch, PL330_FAULT_OPERAND_INVALID);
> +        return;
> +    }
> +    if (ch->ns > ((ch->parent->cfg[3] & (1 << ev_id)) >> ev_id)) {
> +        pl330_fault(ch, PL330_FAULT_EVENT_ER);
> +        return;
> +    }
> +    ch->wakeup = ev_id;
> +}
> +
> +static void pl330_dmawfp(PL330Chan *ch, uint8_t opcode,
> +                         uint8_t *args, int len)
> +{
> +    uint8_t bs = opcode & 3;
> +    uint8_t periph_id;
> +
> +    if (args[0] & 7) {
> +        pl330_fault(ch, PL330_FAULT_OPERAND_INVALID);
> +        return;
> +    }
> +    periph_id = (args[0] >> 3) & 0x1f;
> +    if (periph_id >= ch->parent->periph_num) {
> +        pl330_fault(ch, PL330_FAULT_OPERAND_INVALID);
> +        return;
> +    }
> +    if (ch->ns > ((ch->parent->cfg[4] & (1 << periph_id)) >> periph_id)) {
> +        pl330_fault(ch, PL330_FAULT_CH_PERIPH_ER);
> +        return;
> +    }
> +    switch (bs) {
> +    case 0: /* S */
> +        ch->request_flag = PL330_SINGLE;
> +        ch->wfp_sbp = 0;
> +        break;
> +    case 1: /* P */
> +        ch->request_flag = PL330_BURST;
> +        ch->wfp_sbp = 2;
> +        break;
> +    case 2: /* B */
> +        ch->request_flag = PL330_BURST;
> +        ch->wfp_sbp = 1;
> +        break;
> +    default:
> +        pl330_fault(ch, PL330_FAULT_OPERAND_INVALID);
> +        return;
> +    }
> +
> +    if (ch->parent->periph_busy[periph_id]) {
> +        ch->state = pl330_chan_waiting_periph;
> +    } else if (ch->state == pl330_chan_waiting_periph) {
> +        ch->state = pl330_chan_executing;
> +    }
> +}
> +
> +static void pl330_dmawmb(PL330Chan *ch, uint8_t opcode,
> +                         uint8_t *args, int len)
> +{
> +    /* Do nothing. Since we do not emulate AXI Bus transactions there is no
> +     * stalls. So we are allways on barrier.
> +     */

Same remarks apply as for dmarmb.

> +}
> +
> +/* "NULL" terminated array of the instruction descriptions. */

You don't need quotes around NULL...

> +static const PL330InsnDesc insn_desc[] = {
> +    { .opcode = 0x54, .opmask = 0xFD, .size = 3, .exec = pl330_dmaaddh, },

Where's DMAADNH ? (.opcode = 0x5c, .opmask = 0xfd, insn added in r1p0).

> +    { .opcode = 0x00, .opmask = 0xFF, .size = 1, .exec = pl330_dmaend, },
> +    { .opcode = 0x35, .opmask = 0xFF, .size = 2, .exec = pl330_dmaflushp, },
> +    { .opcode = 0xA0, .opmask = 0xFD, .size = 6, .exec = pl330_dmago, },
> +    { .opcode = 0x04, .opmask = 0xFC, .size = 1, .exec = pl330_dmald, },
> +    { .opcode = 0x25, .opmask = 0xFD, .size = 2, .exec = pl330_dmaldp, },
> +    { .opcode = 0x20, .opmask = 0xFD, .size = 2, .exec = pl330_dmalp, },
> +    /* dmastp  must be before dmalpend in this list, because their maps
> +     * are overlapping
> +     */
> +    { .opcode = 0x29, .opmask = 0xFD, .size = 2, .exec = pl330_dmastp, },
> +    { .opcode = 0x28, .opmask = 0xE8, .size = 2, .exec = pl330_dmalpend, },
> +    { .opcode = 0x01, .opmask = 0xFF, .size = 1, .exec = pl330_dmakill, },

This is out of alphabetical order -- it should be between dmago and dmald.

> +    { .opcode = 0xBC, .opmask = 0xFF, .size = 6, .exec = pl330_dmamov, },
> +    { .opcode = 0x18, .opmask = 0xFF, .size = 1, .exec = pl330_dmanop, },
> +    { .opcode = 0x12, .opmask = 0xFF, .size = 1, .exec = pl330_dmarmb, },
> +    { .opcode = 0x34, .opmask = 0xFF, .size = 2, .exec = pl330_dmasev, },
> +    { .opcode = 0x08, .opmask = 0xFC, .size = 1, .exec = pl330_dmast, },

Missing dmastp.

> +    { .opcode = 0x0C, .opmask = 0xFF, .size = 1, .exec = pl330_dmastz, },
> +    { .opcode = 0x36, .opmask = 0xFF, .size = 2, .exec = pl330_dmawfe, },
> +    { .opcode = 0x30, .opmask = 0xFC, .size = 2, .exec = pl330_dmawfp, },
> +    { .opcode = 0x13, .opmask = 0xFF, .size = 1, .exec = pl330_dmawmb, },
> +    { .opcode = 0x00, .opmask = 0x00, .size = 0, .exec = NULL, }
> +};
> +
> +/* Instructions which can be issued via debug registers. */
> +static const PL330InsnDesc debug_insn_desc[] = {
> +    { .opcode = 0xA0, .opmask = 0xFD, .size = 6, .exec = pl330_dmago, },
> +    { .opcode = 0x01, .opmask = 0xFF, .size = 1, .exec = pl330_dmakill, },
> +    { .opcode = 0x34, .opmask = 0xFF, .size = 2, .exec = pl330_dmasev, },
> +    { .opcode = 0x00, .opmask = 0x00, .size = 0, .exec = NULL, }
> +};
> +
> +static inline const PL330InsnDesc *pl330_fetch_insn(PL330Chan *ch)
> +{
> +    uint8_t opcode;
> +    int i;
> +
> +    cpu_physical_memory_read(ch->pc, &opcode, 1);
> +    for (i = 0; insn_desc[i].size; i++) {
> +        if ((opcode & insn_desc[i].opmask) == insn_desc[i].opcode) {
> +            return &insn_desc[i];
> +        }
> +    }
> +    return NULL;
> +}
> +
> +static inline void pl330_exec_insn(PL330Chan *ch, const PL330InsnDesc *insn)
> +{
> +    uint8_t buf[PL330_INSN_MAXSIZE];

Worth asserting that insn->size is <= MAXSIZE.

> +
> +    cpu_physical_memory_read(ch->pc, buf, insn->size);
> +    insn->exec(ch, buf[0], &buf[1], insn->size - 1);
> +}
> +
> +static inline void pl330_update_pc(PL330Chan *ch,
> +                                   const PL330InsnDesc *insn)
> +{
> +    ch->pc += insn->size;
> +}
> +
> +/* Try to execute current instruction in channel CH. Number of executed
> +   instructions is returned (0 or 1). */
> +static int pl330_chan_exec(PL330Chan *ch)
> +{
> +    const PL330InsnDesc *insn;
> +
> +    if (ch->state != pl330_chan_executing &&
> +            ch->state != pl330_chan_waiting_periph) {
> +        DB_PRINT("%d\n", ch->state);
> +        return 0;
> +    }
> +    ch->stall = 0;
> +    insn = pl330_fetch_insn(ch);
> +    if (!insn) {
> +        DB_PRINT("pl330 undefined instruction\n");
> +        pl330_fault(ch, PL330_FAULT_UNDEF_INSTR);
> +        return 0;
> +    }
> +    pl330_exec_insn(ch, insn);
> +    if (ch->state == pl330_chan_executing && !ch->stall) {
> +        pl330_update_pc(ch, insn);
> +        ch->watchdog_timer = 0;
> +        return 1;
> +    } else {
> +        if (ch->stall) {
> +            ch->watchdog_timer++;
> +            if (ch->watchdog_timer >= PL330_WATCHDOG_LIMIT) {
> +                pl330_fault(ch, PL330_FAULT_LOCKUP_ER);
> +            }
> +        }
> +    }
> +    return 0;
> +}
> +
> +/* Try to execute 1 instruction in each channel, one instruction from read
> +   queue and one instruction from write queue. Number of successfully 
> executed
> +   instructions is returned. */
> +static int pl330_exec_cycle(PL330Chan *channel)
> +{
> +    PL330 *s = channel->parent;
> +    PL330QueueEntry *q;
> +    int i;
> +    int num_exec = 0;
> +    int fifo_res = 0;
> +    uint8_t buf[PL330_MAX_BURST_LEN];
> +
> +    /* Execute one instruction in each channel */
> +    num_exec += pl330_chan_exec(channel);
> +
> +    /* Execute one instruction from read queue */
> +    q = PL330Queue_find_insn(&s->read_queue, PL330_UNTAGGED);
> +    if (q != NULL && q->len <= PL330Fifo_num_free(&s->fifo)) {
> +        cpu_physical_memory_read(q->addr, buf, q->len);
> +        fifo_res = PL330Fifo_push(&s->fifo, buf, q->len, q->tag);
> +        if (fifo_res == PL330FIFO_OK) {
> +            if (q->inc) {
> +                q->addr += q->len;
> +            }
> +            q->n--;
> +            if (!q->n) {
> +                pl330_insn_from_queue(&s->read_queue, q);
> +            }
> +            num_exec++;
> +        }
> +    }
> +
> +    /* Execute one instruction from write queue. */
> +    q = PL330Queue_find_insn(&s->write_queue, PL330Fifo_tag(&s->fifo));
> +    if (q != NULL) {
> +        if (q->z) {
> +            for (i = 0; i < q->len; i++) {
> +                buf[i] = 0;
> +            }
> +        } else {
> +            fifo_res = PL330Fifo_get(&s->fifo, buf, q->len, q->tag);
> +        }
> +        if (fifo_res == PL330FIFO_OK || q->z) {
> +            cpu_physical_memory_write(q->addr, buf, q->len);
> +            if (q->inc) {
> +                q->addr += q->len;
> +            }
> +            num_exec++;
> +        } else if (fifo_res == PL330FIFO_STALL) {
> +            pl330_fault(q->c, PL330_FAULT_FIFOEMPTY_ER);
> +        }
> +        q->n--;
> +        if (!q->n) {
> +            pl330_insn_from_queue(&s->write_queue, q);
> +        }
> +    }
> +
> +    return num_exec;
> +}
> +
> +static int pl330_exec_channel(PL330Chan *channel)
> +{
> +    int insr_exec = 0;
> +
> +    /* TODO: Is it all right to execute everything or should we do per-cycle
> +       simulation? */
> +    while (pl330_exec_cycle(channel)) {
> +        insr_exec++;
> +    }
> +
> +    /* Detect deadlock */
> +    if (channel->state == pl330_chan_executing) {
> +        pl330_fault(channel, PL330_FAULT_LOCKUP_ER);
> +    }
> +    /* Situation when one of the queues has deadlocked but all channels
> +       has finished their programs should be impossible. */

"have finished".

> +
> +    return insr_exec;
> +}
> +
> +
> +static inline void pl330_exec(PL330 *s)
> +{
> +    DB_PRINT("\n");
> +    int i, insr_exec;
> +    do {
> +        insr_exec = pl330_exec_channel(&s->manager);
> +
> +        for (i = 0; i < s->chan_num; i++) {
> +            insr_exec += pl330_exec_channel(&s->chan[i]);
> +        }
> +    } while (insr_exec);
> +}
> +
> +static void pl330_exec_cycle_timer(void *opaque)
> +{
> +    PL330 *s = (PL330 *)opaque;
> +    pl330_exec(s);
> +}
> +
> +/* Stop or restore dma operations */
> +static void pl330_dma_stop_irq(void *opaque, int irq, int level)
> +{
> +    PL330 *s = (PL330 *)opaque;
> +
> +    if (s->periph_busy[irq] != level) {
> +        s->periph_busy[irq] = level;
> +        qemu_mod_timer(s->timer, qemu_get_clock_ns(vm_clock));
> +    }
> +}
> +
> +static void pl330_debug_exec(PL330 *s)
> +{
> +    uint8_t args[5];
> +    uint8_t opcode;
> +    uint8_t chan_id;
> +    int i;
> +    PL330Chan *ch;
> +    const PL330InsnDesc *insn;
> +
> +    s->debug_status = 1;
> +    chan_id = (s->dbg[0] >>  8) & 0x07;
> +    opcode  = (s->dbg[0] >> 16) & 0xff;
> +    args[0] = (s->dbg[0] >> 24) & 0xff;
> +    args[1] = (s->dbg[1] >>  0) & 0xff;
> +    args[2] = (s->dbg[1] >>  8) & 0xff;
> +    args[3] = (s->dbg[1] >> 16) & 0xff;
> +    args[4] = (s->dbg[1] >> 24) & 0xff;
> +    DB_PRINT("chan id: %d\n", chan_id);
> +    if (s->dbg[0] & 1) {
> +        ch = &s->chan[chan_id];
> +    } else {
> +        ch = &s->manager;
> +    }
> +    insn = NULL;
> +    for (i = 0; debug_insn_desc[i].size; i++) {
> +        if ((opcode & debug_insn_desc[i].opmask) == 
> debug_insn_desc[i].opcode) {
> +            insn = &debug_insn_desc[i];
> +        }
> +    }
> +    if (!insn) {
> +        pl330_fault(ch, PL330_FAULT_UNDEF_INSTR);
> +        return ;
> +    }
> +    ch->stall = 0;
> +    insn->exec(ch, opcode, args, insn->size - 1);
> +    if (ch->stall) {
> +        hw_error("pl330: stall of debug instruction not implemented\n");
> +    }
> +    s->debug_status = 0;
> +}
> +
> +
> +/* IOMEM mapped registers */
> +
> +static void pl330_iomem_write(void *opaque, target_phys_addr_t offset,
> +                              uint64_t value, unsigned size)
> +{
> +    PL330 *s = (PL330 *) opaque;
> +    uint32_t i;
> +
> +    DB_PRINT("addr: %08x data: %08x\n", offset, (unsigned)value);
> +
> +    if (offset & 3) {
> +        hw_error("pl330: bad write offset " TARGET_FMT_plx "\n", offset);
> +    }
> +    switch (offset) {
> +    case PL330_REG_INTEN:
> +        s->inten = value;

I rather suspect this should cause us to recalculate the IRQ
output line states.

> +        break;
> +    case PL330_REG_INTCLR:
> +        for (i = 0; i < s->event_num; i++) {
> +            if (s->int_status & s->inten & value & (1 << i)) {
> +                DB_PRINT("event interrupt lowered %d\n", i);
> +                qemu_irq_lower(s->irq[i]);
> +            }
> +        }
> +        s->int_status &= ~value;

This is wrong, I think. Writing to INTCLR should clear bits in
the event-interrupt raw status register (INT_EVENT_RIS) if they
are set in INTEN. (ie s->ev_status &= ~(value & s->inten)). The
INTMIS register (and the output irq line state) is almost certainly
just the result of ANDing INT_EVENT_RIS with INTEN, and so we
don't need separate int_status and ev_status fields in our state
structure.

> +        break;
> +    case PL330_REG_DBGCMD:
> +        if ((value & 3) == 0) {
> +            pl330_debug_exec(s);
> +            pl330_exec(s);
> +        } else {
> +            hw_error("pl330: write of illegal value %u for offset "
> +                     TARGET_FMT_plx "\n", (unsigned)value, offset);
> +        }
> +        break;
> +    case PL330_REG_DBGINST0:
> +        DB_PRINT("s->dbg[0] = %08x\n", (unsigned)value);
> +        s->dbg[0] = value;
> +        break;
> +    case PL330_REG_DBGINST1:
> +        DB_PRINT("s->dbg[1] = %08x\n", (unsigned)value);
> +        s->dbg[1] = value;
> +        break;
> +    default:
> +        hw_error("pl330: bad write offset " TARGET_FMT_plx "\n", offset);
> +        break;
> +    }
> +}
> +
> +static inline uint32_t pl330_iomem_read_imp(void *opaque,
> +        target_phys_addr_t offset)
> +{
> +    PL330 *s = (PL330 *) opaque;
> +    int chan_id;
> +    int i;
> +    uint32_t res;
> +
> +    if (offset & 3) {
> +        hw_error("pl330: bad read offset " TARGET_FMT_plx "\n", offset);
> +    }
> +
> +    if (offset >= PL330_REG_ID && offset < PL330_REG_ID + 32) {
> +        return pl330_id[(offset - PL330_REG_ID) >> 2];
> +    }
> +    if (offset >= PL330_REG_CONFIG && offset < PL330_REG_CONFIG + 24) {
> +        return s->cfg[(offset - PL330_REG_CONFIG) >> 2];
> +    }
> +    if (offset >= PL330_REG_CHANCTRL && offset < 0xD00) {

Rather odd to have a symbolic constant in one term of this condition
but not the other...

> +        offset -= PL330_REG_CHANCTRL;
> +        chan_id = offset >> 5;
> +        if (chan_id >= s->chan_num) {
> +            hw_error("pl330: bad read offset " TARGET_FMT_plx "\n", offset);
> +        }
> +        switch (offset & 0x1f) {
> +        case 0x00:
> +            return s->chan[chan_id].src;
> +        case 0x04:
> +            return s->chan[chan_id].dst;
> +        case 0x08:
> +            return s->chan[chan_id].control;
> +        case 0x0C:
> +            return s->chan[chan_id].lc[0];
> +        case 0x10:
> +            return s->chan[chan_id].lc[1];
> +        default:
> +            hw_error("pl330: bad read offset " TARGET_FMT_plx "\n", offset);
> +        }
> +    }
> +    if (offset >= PL330_REG_CS_BASE && offset < 0x400) {
> +        offset -= PL330_REG_CS_BASE;
> +        chan_id = offset >> 3;
> +        if (chan_id >= s->chan_num) {
> +            hw_error("pl330: bad read offset " TARGET_FMT_plx "\n", offset);
> +        }
> +        switch ((offset >> 2) & 1) {
> +        case 0x0:

Channel Status Register, right? Some of these bitfields appear to
be in the wrong places:

> +            res = (s->chan[chan_id].ns << 20) |

 << 21

> +                    (s->chan[chan_id].wakeup << 4) |
> +                    (s->chan[chan_id].state & 0xf) |
> +                    (s->chan[chan_id].wfp_sbp << 13);

 << 14

Also, is it ever possible for the state to be > 15 ?
If not, no need to mask.

> +            return res;
> +        case 0x1:
> +            return s->chan[chan_id].pc;
> +        default:
> +            hw_error("pl330: read error\n");
> +        }
> +    }
> +    if (offset >= PL330_REG_FTC_BASE && offset < 0x100) {
> +        offset -= PL330_REG_FTC_BASE;
> +        chan_id = offset >> 2;
> +        if (chan_id >= s->chan_num) {
> +            hw_error("pl330: bad read offset " TARGET_FMT_plx "\n", offset);
> +        }
> +        return s->chan[chan_id].fault_type;
> +    }
> +    switch (offset) {
> +    case PL330_REG_DS:
> +        return (s->manager.ns << 8) | (s->manager.wakeup << 4) |

ns is bit 9.

> +            (s->manager.state & 0xf);
> +    case PL330_REG_DPC:
> +        return s->manager.pc;
> +    case PL330_REG_INTEN:
> +        return s->inten;
> +    case PL330_REG_ES:
> +        return s->ev_status;
> +    case PL330_REG_INTSTATUS:
> +        return s->int_status;
> +    case PL330_REG_INTCLR:
> +        /* Documentation says that we can't read this register
> +         * but linux kernel does it */
> +        return 0;
> +    case PL330_REG_FSM:
> +        return s->manager.fault_type ? 1 : 0;

Shouldn't this be
 s->manager.state == pl330_chan_fault ? 1 : 0;
?

> +    case PL330_REG_FSC:
> +        res = 0;
> +        for (i = 0; i < s->chan_num; i++) {
> +            if (s->chan[i].state == pl330_chan_fault ||
> +                s->chan[i].state == pl330_chan_fault_completing) {
> +                res |= 1 << i;
> +            }
> +        }
> +        return res;
> +    case PL330_REG_FTM:
> +        return s->manager.fault_type;
> +    case PL330_REG_DBGSTATUS:
> +        return s->debug_status;
> +    default:
> +        hw_error("pl330: bad read offset " TARGET_FMT_plx "\n", offset);
> +    }
> +    return 0;
> +}
> +
> +static uint64_t pl330_iomem_read(void *opaque, target_phys_addr_t offset,
> +        unsigned size)
> +{
> +    int ret = pl330_iomem_read_imp(opaque, offset);
> +    DB_PRINT("addr: %08x data: %08x\n", offset, ret);
> +    return ret;
> +}
> +
> +static const MemoryRegionOps pl330_ops = {
> +    .read = pl330_iomem_read,
> +    .write = pl330_iomem_write,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
> +};
> +
> +/* Controller logic and initialization */
> +
> +static void pl330_chan_reset(PL330Chan *ch)
> +{
> +    ch->src = 0;
> +    ch->dst = 0;
> +    ch->pc = 0;
> +    ch->state = pl330_chan_stopped;
> +    ch->watchdog_timer = 0;
> +    ch->stall = 0;
> +    ch->control = 0;
> +    ch->status = 0;
> +    ch->fault_type = 0;
> +}
> +
> +static void pl330_reset(DeviceState *d)
> +{
> +    int i;
> +    PL330 *s = FROM_SYSBUS(PL330, sysbus_from_qdev(d));
> +
> +    s->inten = 0;
> +    s->int_status = 0;
> +    s->ev_status = 0;
> +    s->debug_status = 0;
> +    s->num_faulting = 0;
> +    PL330Fifo_reset(&s->fifo);
> +    PL330Queue_reset(&s->read_queue);
> +    PL330Queue_reset(&s->write_queue);
> +
> +    for (i = 0; i < s->chan_num; i++) {
> +        pl330_chan_reset(&s->chan[i]);
> +    }
> +    for (i = 0; i < s->periph_num; i++) {
> +        s->periph_busy[i] = 0;
> +    }

You need to stop the qemu_timer here too (otherwise across
a reset it could still be running).

> +}
> +
> +static int pl330_init(SysBusDevice *dev)
> +{
> +    int i;
> +    PL330 *s = FROM_SYSBUS(PL330, dev);
> +
> +    sysbus_init_irq(dev, &s->irq_abort);
> +    memory_region_init_io(&s->iomem, &pl330_ops, s, "dma", PL330_IOMEM_SIZE);
> +    sysbus_init_mmio(dev, &s->iomem);
> +
> +    s->timer = qemu_new_timer_ns(vm_clock, pl330_exec_cycle_timer, s);
> +
> +    s->chan_num = ((s->cfg[0] >> 4) & 7) + 1;
> +    s->chan = g_malloc0(sizeof(PL330Chan) * s->chan_num);

g_new0().

> +        DB_PRINT("address@hidden", &s->manager);
> +    for (i = 0; i < s->chan_num; i++) {
> +        DB_PRINT("channel address@hidden", i, &s->chan[i]);
> +        s->chan[i].parent = s;
> +        s->chan[i].tag = (uint8_t)i;
> +    }
> +    s->manager.parent = s;
> +    s->manager.tag = s->chan_num;
> +    s->manager.ns = (s->cfg[0] >> 2) & 1;
> +    s->manager.is_manager = 1;
> +    if (s->cfg[0] & 1) {
> +        s->periph_num = ((s->cfg[0] >> 12) & 0x1f) + 1;
> +    } else {
> +        s->periph_num = 0;
> +    }
> +    s->event_num = ((s->cfg[0] >> 17) & 0x1f) + 1;
> +
> +    s->irq = g_malloc0(sizeof(qemu_irq) * s->event_num);
> +    for (i = 0; i < s->event_num; i++) {
> +        sysbus_init_irq(dev, &s->irq[i]);
> +    }
> +
> +    qdev_init_gpio_in(&dev->qdev, pl330_dma_stop_irq, PL330_PERIPH_NUM);
> +
> +    PL330Queue_init(&s->read_queue, ((s->cfg[5] >> 16) & 0xf) + 1, 
> s->chan_num);
> +    PL330Queue_init(&s->write_queue, ((s->cfg[5] >> 8) & 0xf) + 1, 
> s->chan_num);
> +    PL330Fifo_init(&s->fifo, ((s->cfg[5] >> 20) & 0x1ff) + 1);
> +    pl330_reset(&s->busdev.qdev);
> +
> +    return 0;
> +}
> +
> +static Property pl330_properties[] = {
> +    DEFINE_PROP_UINT32("cfg0", PL330, cfg[0], 0),
> +    DEFINE_PROP_UINT32("cfg1", PL330, cfg[1], 0),
> +    DEFINE_PROP_UINT32("cfg2", PL330, cfg[2], 0),
> +    DEFINE_PROP_UINT32("cfg3", PL330, cfg[3], 0),
> +    DEFINE_PROP_UINT32("cfg4", PL330, cfg[4], 0),
> +    DEFINE_PROP_UINT32("cfg5", PL330, cfg[5], 0),

I think it would be nicer if our config properties matched the
ones the hardware provides (as h/w tie-offs or signal sampled
at reset):
  num_events
  num_periph_req
  num_chnls
  mgr_ns_at_rst
  boot_en
  periph_req [although we don't actually honour this one]
  num_i-cache_lines
  i-cache_len
  boot_addr
  ins
  pns
  data_buffer_dep
  rd_q_dep
  rd_cap
  wr_q_dep
  wr_cap
  data_width



> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void pl330_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
> +
> +    k->init = pl330_init;
> +    dc->reset = pl330_reset;
> +    dc->props = pl330_properties;
> +}
> +
> +static TypeInfo pl330_info = {
> +    .name           = "pl330",
> +    .parent         = TYPE_SYS_BUS_DEVICE,
> +    .instance_size  = sizeof(PL330),
> +    .class_init      = pl330_class_init,
> +};
> +
> +static void pl330_register_types(void)
> +{
> +    type_register_static(&pl330_info);
> +}
> +
> +type_init(pl330_register_types)
> --
> 1.7.3.2

-- PMM



reply via email to

[Prev in Thread] Current Thread [Next in Thread]