Re: [Qemu-arm] [PATCH v11 09/17] hw/arm/smmuv3: Implement translate callback

[Peter Maydell]

On 12 April 2018 at 08:38, Eric Auger <address@hidden> wrote:
> This patch implements the IOMMU Memory Region translate()
> callback. Most of the code relates to the translation
> configuration decoding and check (STE, CD).
>
> Signed-off-by: Eric Auger <address@hidden>
> Signed-off-by: Prem Mallappa <address@hidden>

> +/* @ssid > 0 not supported yet */
> +static int smmu_get_cd(SMMUv3State *s, STE *ste, uint32_t ssid,
> +                       CD *buf, SMMUEventInfo *event)
> +{
> +    dma_addr_t addr = STE_CTXPTR(ste);
> +    int ret;
> +
> +    trace_smmuv3_get_cd(addr);
> +    /* TODO: guarantee 64-bit single-copy atomicity */
> +    ret = dma_memory_read(&address_space_memory, addr,
> +                           (void *)buf, sizeof(*buf));
> +    if (ret != MEMTX_OK) {
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "Cannot fetch pte at address=0x%"PRIx64"\n", addr);
> +        event->type = SMMU_EVT_F_CD_FETCH;
> +        event->u.f_ste_fetch.addr = addr;
> +        return -EINVAL;
> +    }
> +    return 0;
> +}
> +
> +/* Returns <0 if the caller has no need to purse the translation */

Typo, but I'm not sure what you meant instead of "purse".

> +static int decode_ste(SMMUv3State *s, SMMUTransCfg *cfg,
> +                      STE *ste, SMMUEventInfo *event)
> +{
> +    uint32_t config = STE_CONFIG(ste);
> +    int ret = -EINVAL;
> +
> +    if (STE_CFG_ABORT(config)) {
> +        cfg->aborted = true; /* abort but don't record any event */
> +        return ret;
> +    }
> +
> +    if (STE_CFG_BYPASS(config)) {
> +        cfg->bypassed = true;
> +        return ret;
> +    }
> +
> +    if (!STE_VALID(ste)) {
> +        goto bad_ste;
> +    }

This check is happening too late -- if STE.V is 0 then other STE fields
must be ignored, including STE.Config, so it has to be done as the first
thing in this function.

> +
> +    if (STE_CFG_S2_ENABLED(config)) {
> +        qemu_log_mask(LOG_UNIMP, "SMMUv3 does not support stage 2 yet\n");
> +        goto bad_ste;
> +    }
> +
> +    if (STE_S1CDMAX(ste) != 0) {
> +        qemu_log_mask(LOG_UNIMP,
> +                      "SMMUv3 does not support multiple context descriptors 
> yet\n");
> +        goto bad_ste;
> +    }
> +
> +    if (STE_S1STALLD(ste)) {
> +        qemu_log_mask(LOG_UNIMP,
> +                      "SMMUv3 S1 stalling fault model not allowed yet\n");
> +        goto bad_ste;
> +    }
> +    return 0;
> +
> +bad_ste:
> +    event->type = SMMU_EVT_C_BAD_STE;
> +    return -EINVAL;
> +}

> +static int decode_cd(SMMUTransCfg *cfg, CD *cd, SMMUEventInfo *event)
> +{
> +    int ret = -EINVAL;
> +    int i;
> +
> +    if (!CD_VALID(cd) || !CD_AARCH64(cd)) {
> +        goto bad_cd;
> +    }
> +
> +    /* we support only those at the moment */
> +    cfg->aa64 = true;
> +    cfg->stage = 1;
> +
> +    cfg->oas = oas2bits(CD_IPS(cd));
> +    cfg->oas = MIN(oas2bits(SMMU_IDR5_OAS), cfg->oas);
> +    cfg->tbi = CD_TBI(cd);
> +    cfg->asid = CD_ASID(cd);
> +
> +    trace_smmuv3_decode_cd(cfg->oas);
> +
> +    /* decode data dependent on TT */
> +    for (i = 0; i <= 1; i++) {
> +        int tg, tsz;
> +        SMMUTransTableInfo *tt = &cfg->tt[i];
> +
> +        cfg->tt[i].disabled = CD_EPD(cd, i);
> +        if (cfg->tt[i].disabled) {
> +            continue;
> +        }
> +
> +        if (!CD_A(cd)) {
> +            goto bad_cd; /* SMMU_IDR0.TERM_MODEL == 1 */
> +        }
> +        if (CD_S(cd)) {
> +            goto bad_cd; /* !STE_SECURE && SMMU_IDR0.STALL_MODEL == 1 */
> +        }
> +        if (CD_HA(cd) || CD_HD(cd)) {
> +            goto bad_cd; /* HTTU = 0 */
> +        }

The location of this check means you're going to be ignoring HA and HD
if EPD0 and EPD1 are both zero, which isn't what the spec says.
Since the HA/HD bits aren't dependent on EPD*, it would make more sense
for this check to be outside the for() loop. This applies
to some of the other checks too: check the pseudocode CD_ILLEGAL
expression and in particular which parts of it involve checks for
N_TRANS_CFG0 or N_TRANSL_CFG1. At least the CD_A and CD_S checks
should also be outside the loop.

> +
> +        tsz = CD_TSZ(cd, i);
> +        if (tsz < 16 || tsz > 39) {
> +            goto bad_cd;
> +        }
> +
> +        tg = CD_TG(cd, i);
> +        tt->granule_sz = tg2granule(tg, i);
> +        if ((tt->granule_sz != 12 && tt->granule_sz != 16) || CD_ENDI(cd)) {
> +            goto bad_cd;
> +        }
> +
> +        tt->tsz = tsz;
> +        tt->initial_level = 4 - (64 - tsz - 4) / (tt->granule_sz - 3);
> +        tt->ttb = CD_TTB(cd, i);
> +        if (tt->ttb & ~(MAKE_64BIT_MASK(0, cfg->oas))) {
> +            goto bad_cd;
> +        }
> +        trace_smmuv3_decode_cd_tt(i, tt->tsz, tt->ttb,
> +                                  tt->granule_sz, tt->initial_level);
> +    }
> +
> +    event->record_trans_faults = CD_R(cd);
> +
> +    return 0;
> +
> +bad_cd:
> +    event->type = SMMU_EVT_C_BAD_CD;
> +    return ret;
> +}

thanks
-- PMM