Re: [RFC PATCH 06/16] hw/arm/smmuv3: Parse STE config for stage-2

[Eric Auger]

Hi Mostafa,

On 2/5/23 10:44, Mostafa Saleh wrote:
> Parse stage-2 configuration and populate it in SMMUTransCfg.
> Configs in this patch (s2g, ttb, tsz, sl0).
above 'sentence' a bit cryptic.
> Checking validity of values added when possible.
>
> MAX IPA supported is 48 bits and only AA64 tables are supported.
>
> Signed-off-by: Mostafa Saleh <smostafa@google.com>
> ---
>  hw/arm/smmuv3.c              | 43 +++++++++++++++++++++++++++++++++++-
>  include/hw/arm/smmu-common.h |  1 +
>  2 files changed, 43 insertions(+), 1 deletion(-)
>
> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> index 54dd8e5ec1..6633fe40fa 100644
> --- a/hw/arm/smmuv3.c
> +++ b/hw/arm/smmuv3.c
> @@ -366,7 +366,48 @@ static int decode_ste(SMMUv3State *s, SMMUTransCfg *cfg,
>          return 0;
>      }
>  
> -    if (STE_CFG_S2_ENABLED(config)) {
> +    if (STAGE2_SUPPORTED(s->features) && STE_CFG_S2_ENABLED(config)) {
Don't you want to check both S1 and S2 aren't set?
> +        cfg->stage = 2;
> +
> +        if (STE_S2AA64(ste) == 0x0) {
> +            qemu_log_mask(LOG_UNIMP,
> +                          "SMMUv3 AArch32 tables not supported\n");
> +            goto bad_ste;
> +        }
> +
> +        switch (STE_S2TG(ste)) {
> +        case 0x0: /* 4KB */
> +            cfg->s2cfg.granule_sz = 12;
> +            break;
> +        case 0x1: /* 64KB */
> +            cfg->s2cfg.granule_sz = 16;
> +            break;
> +        case 0x2: /* 16KB */
> +            cfg->s2cfg.granule_sz = 14;
> +            break;
> +        default:
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                          "SMMUv3 bad STE S2TG: %x\n", STE_S2TG(ste));
> +            goto bad_ste;
> +        }
> +
> +        cfg->s2cfg.vttb = STE_S2TTB(ste);
> +        cfg->s2cfg.tsz = STE_S2T0SZ(ste);
What about IDR3.STT currently 0 so S2T0SZ <= 39

don't you need to check against SMMU_IDR3.STT/S2TG

• In architectures after SMMUv3.0:
– If STE.S2TG selects a 4KB or 16KB granule, the minimum valid value for
this field is MAX(16,
64-IAS).
– If STE.S2TG selects a 64KB granule, the minimum valid value for this
field is (64-IAS).
> +
> +        if ((64 - cfg->s2cfg.tsz) > SMMU_MAX_IPA_BITS) {
> +            qemu_log_mask(LOG_UNIMP, "SMMUv3 IPA too big! TS0Z = %x\n",
> +                          cfg->s2cfg.tsz);
> +            goto bad_ste;
> +        }
> +
> +        cfg->s2cfg.sl0 = STE_S2SL0(ste);
> +        if (cfg->s2cfg.sl0 == 0x3) {
> +            qemu_log_mask(LOG_UNIMP,
> +                          "SMMUv3 STE->SL0 0x3 has no meaning!\n");
> +            goto bad_ste;
what about S2PS, S2VMID?

you may either squash [RFC PATCH 11/16] hw/arm/smmuv3: Read VMID from
STE into that patch or at least mention in the commit msg that S2VMID
will be dealt with separately

Eric

> +        }
> +
> +        /* This is still here as stage 2 has not been fully enabled yet. */
>          qemu_log_mask(LOG_UNIMP, "SMMUv3 does not support stage 2 yet\n");
>          goto bad_ste;
>      }
> diff --git a/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-common.h
> index 1e666e8b6d..7906e359d9 100644
> --- a/include/hw/arm/smmu-common.h
> +++ b/include/hw/arm/smmu-common.h
> @@ -28,6 +28,7 @@
>  #define SMMU_PCI_DEVFN(sid)   (sid & 0xFF)
>  
>  #define SMMU_MAX_VA_BITS      48
> +#define SMMU_MAX_IPA_BITS     48
>  #define SMMU_MAX_LEVELS       4
>  
>  /*