Re: [Qemu-arm] [PATCH v11 03/17] hw/arm/smmu-common: VMSAv8-64 page table walk

[Peter Maydell]

On 12 April 2018 at 08:37, Eric Auger <address@hidden> wrote:
> This patch implements the page table walk for VMSAv8-64.
>
> Signed-off-by: Eric Auger <address@hidden>
> Signed-off-by: Prem Mallappa <address@hidden>

> diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
> index 9a966bb..6a58948 100644
> --- a/hw/arm/smmu-common.c
> +++ b/hw/arm/smmu-common.c
> @@ -27,6 +27,215 @@
>
>  #include "qemu/error-report.h"
>  #include "hw/arm/smmu-common.h"
> +#include "smmu-internal.h"
> +
> +/* VMSAv8-64 Translation */
> +
> +/**
> + * get_pte - Get the content of a page table entry located t
> + * @address@hidden
> + */

Comment appears to be truncated ?

> +static int get_pte(dma_addr_t baseaddr, uint32_t index, uint64_t *pte,
> +                   SMMUPTWEventInfo *info)
> +{
> +    int ret;
> +    dma_addr_t addr = baseaddr + index * sizeof(*pte);
> +
> +    /* TODO: guarantee 64-bit single-copy atomicity */
> +    ret = dma_memory_read(&address_space_memory, addr,
> +                          (uint8_t *)pte, sizeof(*pte));
> +
> +    if (ret != MEMTX_OK) {
> +        info->type = SMMU_PTW_ERR_WALK_EABT;
> +        info->addr = addr;
> +        return -EINVAL;
> +    }
> +    trace_smmu_get_pte(baseaddr, index, addr, *pte);
> +    return 0;
> +}
> +

> +/**
> + * smmu_ptw - Walk the page tables for an IOVA, according to @cfg
> + *
> + * @cfg: translation configuration
> + * @iova: iova to translate
> + * @perm: tentative access type
> + * @tlbe: returned entry
> + * @info: ptw event handle
> + *
> + * return 0 on success
> + */
> +inline int smmu_ptw(SMMUTransCfg *cfg, dma_addr_t iova, IOMMUAccessFlags 
> perm,
> +             IOMMUTLBEntry *tlbe, SMMUPTWEventInfo *info)
> +{
> +    if (!cfg->aa64) {
> +        /*
> +         * This code path is not entered as we check this while decoding
> +         * the configuration data in the derived SMMU model.
> +         */
> +        assert(0);

Prefer either g_assert_not_reached() or just assert(cfg->aa64).

> +    }
> +
> +    return smmu_ptw_64(cfg, iova, perm, tlbe, info);
> +}
>

> +
> +/*
> + * TODO: At the moment all transactions are considered as priviledged (EL1)

"privileged"

> + * as IOMMU translation callback does not pass user/priv attributes.
> + */
> +#define is_permission_fault(ap, perm) \
> +    (((perm) & IOMMU_WO) && ((ap) & 0x2))
> +
> +#define PTE_AP_TO_PERM(ap) \
> +    (IOMMU_ACCESS_FLAG(true, !((ap) & 0x2)))
> +
> +/* Level Indexing */
> +
> +static inline int level_shift(int level, int granule_sz)
> +{
> +    return granule_sz + (3 - level) * (granule_sz - 3);
> +}
> +
> +static inline uint64_t level_page_mask(int level, int granule_sz)
> +{
> +    return ~(MAKE_64BIT_MASK(0, level_shift(level, granule_sz)));
> +}
> +
> +/**
> + * TODO: handle the case where the level resolves less than
> + * granule_sz -3 IA bits.
> + */
> +static inline
> +uint64_t iova_level_offset(uint64_t iova, int level, int granule_sz)
> +{
> +    return (iova >> level_shift(level, granule_sz)) &
> +            MAKE_64BIT_MASK(0, granule_sz - 3);
> +}
> +
> +#endif

When does the TODO case happen, and what goes wrong?

> --- a/include/hw/arm/smmu-common.h
> +++ b/include/hw/arm/smmu-common.h
> @@ -129,4 +129,18 @@ static inline uint16_t smmu_get_sid(SMMUDevice *sdev)
>  {
>      return  ((pci_bus_num(sdev->bus) & 0xff) << 8) | sdev->devfn;
>  }
> +
> +/**
> + * smmu_ptw - Perform the page table walk for a given iova / access flags
> + * pair, according to @cfg translation config
> + */
> +int smmu_ptw(SMMUTransCfg *cfg, dma_addr_t iova, IOMMUAccessFlags perm,
> +             IOMMUTLBEntry *tlbe, SMMUPTWEventInfo *info);
> +
> +/**
> + * select_tt - compute which translation table shall be used according

"according to"

> + * the input iova and tranlsation config and return the TT specific info

"translation"

> + */
> +SMMUTransTableInfo *select_tt(SMMUTransCfg *cfg, dma_addr_t iova);
> +
>  #endif  /* HW_ARM_SMMU_COMMON */

Otherwise
Reviewed-by: Peter Maydell <address@hidden>

thanks
-- PMM