Re: [Qemu-devel] [PATCH v7 03/20] hw/arm/smmu-common: smmu_read/write_sysmem

[Peter Maydell]

On 1 September 2017 at 18:21, Eric Auger <address@hidden> wrote:
> Those two functions will be used to access configuration
> data (STE, CD) and page table entries in guest RAM.
>
> Signed-off-by: Eric Auger <address@hidden>
> ---
>  hw/arm/smmu-common.c         | 37 +++++++++++++++++++++++++++++++++++++
>  include/hw/arm/smmu-common.h |  5 +++++
>  2 files changed, 42 insertions(+)
>
> diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
> index 3e67992..2a94547 100644
> --- a/hw/arm/smmu-common.c
> +++ b/hw/arm/smmu-common.c
> @@ -30,6 +30,43 @@
>  #include "qemu/error-report.h"
>  #include "hw/arm/smmu-common.h"
>
> +inline MemTxResult smmu_read_sysmem(dma_addr_t addr, void *buf, dma_addr_t 
> len,
> +                                    bool secure)
> +{
> +    MemTxAttrs attrs = {.unspecified = 1, .secure = secure};

This isn't right. .unspecified = 1 means "transaction master has
not explicitly specified any attributes", but you are specifying
one (the secure one).

> +    switch (len) {
> +    case 4:
> +        *(uint32_t *)buf = ldl_le_phys(&address_space_memory, addr);
> +        break;
> +    case 8:
> +        *(uint64_t *)buf = ldq_le_phys(&address_space_memory, addr);
> +        break;
> +    default:
> +        return address_space_rw(&address_space_memory, addr,
> +                                attrs, buf, len, false);

Why do we have the special cases for 4 and 8? In particular, those
code paths will not correctly detect memory transaction failures.

> +    }
> +    return MEMTX_OK;
> +}
> +
> +inline void
> +smmu_write_sysmem(dma_addr_t addr, void *buf, dma_addr_t len, bool secure)
> +{
> +    MemTxAttrs attrs = {.unspecified = 1, .secure = secure};
> +
> +    switch (len) {
> +    case 4:
> +        stl_le_phys(&address_space_memory, addr, *(uint32_t *)buf);
> +        break;
> +    case 8:
> +        stq_le_phys(&address_space_memory, addr, *(uint64_t *)buf);
> +        break;
> +    default:
> +        address_space_rw(&address_space_memory, addr,
> +                         attrs, buf, len, true);
> +    }
> +}
> +
>  /******************/
>  /* Infrastructure */
>  /******************/
> diff --git a/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-common.h
> index 20f3fe6..a5999b0 100644
> --- a/include/hw/arm/smmu-common.h
> +++ b/include/hw/arm/smmu-common.h
> @@ -111,4 +111,9 @@ static inline uint16_t smmu_get_sid(SMMUDevice *sdev)
>  {
>      return  ((pci_bus_num(sdev->bus) & 0xff) << 8) | sdev->devfn;
>  }
> +
> +MemTxResult smmu_read_sysmem(dma_addr_t addr, void *buf,
> +                             dma_addr_t len, bool secure);
> +void smmu_write_sysmem(dma_addr_t addr, void *buf, dma_addr_t len, bool 
> secure);
> +

There are so few callers of this that I'm inclined to think you
should just open code the right kind of memory accessor function
in the callsites rather than having a weird switch statement.

>  #endif  /* HW_ARM_SMMU_COMMON */
> --
> 2.5.5
>

thanks
-- PMM