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

[Mostafa Saleh]

Hi Eric,

On Wed, Feb 15, 2023 at 06:47:52PM +0100, Eric Auger wrote:
> 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.
I will reword it.

> > +++ 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?
Yes, currently this is ignored, but looking at the SMMU manual it is
illegal to configure an unsupported stage, I will update it.

> > +            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).
I will add a function to validate S2T0SZ based on the behaviour after
SMMUv3.0 and checks against STT disabled. I believe it is safe just to
check S2T0SZ <= 39 without checking IDR3.STT as we don’t support it
for now.

> > +                          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
I will squash it with
"[RFC PATCH 11/16] hw/arm/smmuv3: Read VMID from STE"
I missed S2PS, I will add it in V2.

Thanks,
Mostafa