Re: [PATCH] hw/dma/pl330: Add memory region to replace default address_space_memory

[Peter Maydell]

On Fri, 13 Aug 2021 at 07:44, Wen, Jianxian
<Jianxian.Wen@verisilicon.com> wrote:
>
> From f780b0ee2ee36c562ab814915fff0e7217b25e63 Mon Sep 17 00:00:00 2001
>
> From: Jianxian Wen <jianxian.wen@verisilicon.com>
>
> Date: Tue, 3 Aug 2021 09:44:35 +0800
>
> Subject: [PATCH] hw/dma/pl330: Add memory region to replace default
>
>  address_space_memory
>
>
>
> PL330 needs a memory region which can connect with SMMU translate IOMMU 
> region to support SMMU.
>
>
>
> Signed-off-by: Jianxian Wen <jianxian.wen@verisilicon.com>

Hi; thanks for sending this patch. It mostly looks good to me.
I have a review comment (which you can find further down in this
email), and one note on the patch-sending mechanics:

Your mail client is sending these patch emails as multipart/alternative
with both a plain text and an HTML version. In general the list prefers
plain text only, but this is particularly important for patch emails because
the HTML can confuse automatic patch handling tools.

If you haven't read the SubmitAPatch guidelines you can find them here:
https://wiki.qemu.org/Contribute/SubmitAPatch

Using 'git send-email' is usually the most reliable way to send patches
if you're a regular submitter, but it can take a bit of setup to get
working. You might also be interested in the sourcehut web-based UI for
sending patch emails described in this subsection:
https://wiki.qemu.org/Contribute/SubmitAPatch#If_you_cannot_send_patch_emails

>
> ---
> @@ -1108,7 +1111,8 @@ static inline const PL330InsnDesc 
> *pl330_fetch_insn(PL330Chan *ch)
>
>      uint8_t opcode;
>
>      int i;
>
>
>
> -    dma_memory_read(&address_space_memory, ch->pc, &opcode, 1);
>
> +    address_space_read(&ch->parent->mem_as, ch->pc,
>
> +                        MEMTXATTRS_UNSPECIFIED, &opcode, 1);

dma_memory_read() can already take an AddressSpace* -- you don't need
to change these calls to address_space_read(), just change the AS*
we pass them.

Otherwise the change looks OK to me.

thanks
-- PMM