On 1 September 2017 at 18:21, Eric Auger <eric.au...@redhat.com> 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 <eric.au...@redhat.com>
> ---
> 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
