Re: [RFC PATCH v2 17/20] hw/arm/smmuv3: Check idr registers for STE_S1CDMAX and STE_S1STALLD

[Nicolin Chen <nicolinc@nvidia.com>]

On Wed, Mar 26, 2025 at 06:18:49PM +0100, Eric Auger wrote:
> > @@ -561,6 +561,16 @@ static int decode_ste(SMMUv3State *s, SMMUTransCfg *cfg,
> >  
> >      decode_ste_config(cfg, config);
> >  
> > +      /* S1DSS.Terminate is same as Config.abort for default stream */
> 
> S1DSS. Termination

The spec uses "Terminate":
"• 0b00: Terminate. An abort is reported to the device and the
                     F_STREAM_DISABLED event is recorded."

> 
> > +    if (STE_CFG_S1_ENABLED(config) && STE_S1DSS(ste) == 0) {
> > +        cfg->aborted = true;
> The spec says:
> "
> When substreams are enabled (STE.S1CDMax != 0), this field determines
> the behavior of a transaction or
> translation request that arrives without an associated substream:
> "
> So I understand you should also check STE.S1CDMax.

Yea, that's missing, as spec also says:
"If Config[0] == 0 (stage 1 disabled) or S1CDMax == 0 (substreams
 disabled) or SMMU_IDR1.SSIDSIZE == 0 (substreams unsupported),
 this field is IGNORED."

> Also how do we check
> that the incoming transacrion arrives without a substream?
> 
> In general shouldn't we add the support of subtreams in the emulated
> code too?
> 
> the spec also says that in that case you should record a
> F_STREAM_DISABLED event.

Yea, that's seemingly missing too, for value 0b10.

> > +    }
> > +
> > +    /* S1DSS.Bypass is same as Config.bypass for default stream */

> S1DSS. Bypass

Perhaps, instead of FIELD.VALUE format, we should do:
	FIELD=VALUE?
e.g. S1DSS=Terminate (0b01) and S1DSS=Bypass (0b01).

> > +    if (STE_CFG_S1_ENABLED(config) && STE_S1DSS(ste) == 0x1) {
> > +        cfg->bypassed = true;
> > +    }
> > +
> >      if (cfg->aborted || cfg->bypassed) {
> >          return 0;
> >      }
> > @@ -598,13 +608,14 @@ static int decode_ste(SMMUv3State *s, SMMUTransCfg *cfg,
> >          }
> >      }
> >  
> > -    if (STE_S1CDMAX(ste) != 0) {
> > +    if (!FIELD_EX32(s->idr[1], IDR1, SSIDSIZE) && STE_S1CDMAX(ste) != 0) {
> >          qemu_log_mask(LOG_UNIMP,
> >                        "SMMUv3 does not support multiple context descriptors yet\n");
> the log message should be different because it becomes a guest error:
> qemu_log_mask(LOG_GUEST_ERROR, "invalid S1CDMAX as SSIDSIZE==0") or
> something alike
> 
> >          goto bad_ste;
> >      }
> >  
> > -    if (STE_S1STALLD(ste)) {
> > +    /* STALL_MODEL being 0b01 means "stall is not supported" */
> > +    if ((FIELD_EX32(s->idr[0], IDR0, STALL_MODEL) & 0x1) && STE_S1STALLD(ste)) {
> >          qemu_log_mask(LOG_UNIMP,
> >                        "SMMUv3 S1 stalling fault model not allowed yet\n");
> same here.
> 
> Again I think we need to understand the consequence of having a more
> comprehensive support of SSID. This also holds with old the IDR fields
> that may be inherited from the HW and we don't support yet in the
> emulation code

To support guest-level SVA, it must support SSID. We can keep the
SSIDSIZE=0 in an emulated SMMU. Would you elaborate the concern of
doing so?

Thanks
Nicolin