On 8/18/21 4:47 AM, Wen, Jianxian wrote:
> Add configurable property memory region which can connect with IOMMU region
> to support SMMU translate.
>
> Signed-off-by: Jianxian Wen <jianxian....@verisilicon.com>
> ---
> v3 -> v4 (after review of Philippe Mathieu-Daudé):
> - Avoid adding unnecessary AS, add AS if we connect with IOMMU region.
> v2 -> v3 (after review of Philippe Mathieu-Daudé):
> - Refine code to comply with code style, update error message if memory link
> is not set.
> v1 -> v2 (after review of Peter Maydell):
> - Data access use dma_memory_read/write functions, update function
> AddressSpace* parameter.
>
> hw/dma/pl330.c | 21 +++++++++++++++++----
> 1 file changed, 17 insertions(+), 4 deletions(-)
>
> diff --git a/hw/dma/pl330.c b/hw/dma/pl330.c
> index 944ba29..8ae56c7 100644
> --- a/hw/dma/pl330.c
> +++ b/hw/dma/pl330.c
> @@ -269,6 +269,9 @@ struct PL330State {
> uint8_t num_faulting;
> uint8_t periph_busy[PL330_PERIPH_NUM];
>
> + /* Memory region that DMA operation access */
> + MemoryRegion *mem_mr;
> + AddressSpace *mem_as;
> };
>
> #define TYPE_PL330 "pl330"
> @@ -1108,7 +1111,7 @@ static inline const PL330InsnDesc
> *pl330_fetch_insn(PL330Chan *ch)
> uint8_t opcode;
> int i;
>
> - dma_memory_read(&address_space_memory, ch->pc, &opcode, 1);
> + dma_memory_read(ch->parent->mem_as, 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];
> @@ -1122,7 +1125,7 @@ static inline void pl330_exec_insn(PL330Chan *ch, const
> PL330InsnDesc *insn)
> uint8_t buf[PL330_INSN_MAXSIZE];
>
> assert(insn->size <= PL330_INSN_MAXSIZE);
> - dma_memory_read(&address_space_memory, ch->pc, buf, insn->size);
> + dma_memory_read(ch->parent->mem_as, ch->pc, buf, insn->size);
> insn->exec(ch, buf[0], &buf[1], insn->size - 1);
> }
>
> @@ -1186,7 +1189,7 @@ static int pl330_exec_cycle(PL330Chan *channel)
> if (q != NULL && q->len <= pl330_fifo_num_free(&s->fifo)) {
> int len = q->len - (q->addr & (q->len - 1));
>
> - dma_memory_read(&address_space_memory, q->addr, buf, len);
> + dma_memory_read(s->mem_as, q->addr, buf, len);
> trace_pl330_exec_cycle(q->addr, len);
> if (trace_event_get_state_backends(TRACE_PL330_HEXDUMP)) {
> pl330_hexdump(buf, len);
> @@ -1217,7 +1220,7 @@ static int pl330_exec_cycle(PL330Chan *channel)
> fifo_res = pl330_fifo_get(&s->fifo, buf, len, q->tag);
> }
> if (fifo_res == PL330_FIFO_OK || q->z) {
> - dma_memory_write(&address_space_memory, q->addr, buf, len);
> + dma_memory_write(s->mem_as, q->addr, buf, len);
> trace_pl330_exec_cycle(q->addr, len);
> if (trace_event_get_state_backends(TRACE_PL330_HEXDUMP)) {
> pl330_hexdump(buf, len);
> @@ -1562,6 +1565,13 @@ static void pl330_realize(DeviceState *dev, Error
> **errp)
> "dma", PL330_IOMEM_SIZE);
> sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->iomem);
>
> + if (s->mem_mr && (s->mem_mr != get_system_memory())) {
> + s->mem_as = g_malloc0(sizeof(AddressSpace));
> + address_space_init(s->mem_as, s->mem_mr, s->mem_mr->name);
I'm not sure about the description. Anyhow, don't access MemoryRegion
internals, use memory_region_name().
> + } else {
> + s->mem_as = &address_space_memory;
> + }
A bit convoluted but I like the result.
Maybe easier to follow as:
if (!s->mem_mr) {
error_setg(errp, "'memory' link is not set");
return;
} else if (s->mem_mr == get_system_memory()) {
/* Avoid creating new AS for system memory. */
s->mem_as = &address_space_memory;
} else {
g_autofree char *desc =
g_strdup_printf("%s-as", memory_region_name(s->mem_mr);
s->mem_as = g_new0(AddressSpace, 1);
address_space_init(s->mem_as, s->mem_mr, desc);
}
'desc' is optional, I don't mind if you prefer the simpler:
address_space_init(s->mem_as, s->mem_mr,
memory_region_name(s->mem_mr));
> s->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, pl330_exec_cycle_timer, s);
>
> s->cfg[0] = (s->mgr_ns_at_rst ? 0x4 : 0) |
> @@ -1656,6 +1666,9 @@ static Property pl330_properties[] = {
> DEFINE_PROP_UINT8("rd_q_dep", PL330State, rd_q_dep, 16),
> DEFINE_PROP_UINT16("data_buffer_dep", PL330State, data_buffer_dep, 256),
>
> + DEFINE_PROP_LINK("memory", PL330State, mem_mr,
> + TYPE_MEMORY_REGION, MemoryRegion *),
> +
> DEFINE_PROP_END_OF_LIST(),
> };
>
>
