Re: [PATCH v4 1/2] iommu/arm-smmu-v3: Match Stall behaviour for S2

[Nicolin Chen <nicolinc@nvidia.com>]

On Fri, Aug 30, 2024 at 06:02:35PM +0100, Robin Murphy wrote:

> On 30/08/2024 5:30 pm, Nicolin Chen wrote:
> > On Fri, Aug 30, 2024 at 11:03:47AM +0000, Mostafa Saleh wrote:
> > 
> > > According to the spec (ARM IHI 0070 F.b), in
> > > "5.5 Fault configuration (A, R, S bits)":
> > >      A STE with stage 2 translation enabled and STE.S2S == 0 is
> > >      considered ILLEGAL if SMMU_IDR0.STALL_MODEL == 0b10.
> > > 
> > > Also described in the pseudocode “SteIllegal()”
> > >      if STE.Config == '11x' then
> > >          [..]
> > >          if eff_idr0_stall_model == '10' && STE.S2S == '0' then
> > >              // stall_model forcing stall, but S2S == 0
> > >              return TRUE;
> > > 
> > > Which means, S2S must be set when stall model is
> > > "ARM_SMMU_FEAT_STALL_FORCE", but currently the driver ignores that.
> > > 
> > > Although, the driver can do the minimum and only set S2S for
> > > “ARM_SMMU_FEAT_STALL_FORCE”, it is more consistent to match S1
> > > behaviour, which also sets it for “ARM_SMMU_FEAT_STALL” if the
> > > master has requested stalls.
> > 
> > If I read the SteIllegal() correctly, it seems S2S would conflict
> > against the STE.EATS settings?
> > 
> > // Check ATS configuration
> > if ((sec_sid == SS_NonSecure && SMMU_IDR0.ATS == '1') ||
> >      (sec_sid == SS_Realm && SMMU_R_IDR0.ATS == '1')) &&
> >      STE.Config != 'x00' then
> >      // Needs to be NS/Realm, ATS enabled, and not Bypass
> >          if STE.EATS == '01' && STE.S2S == '1' then
> >              // Full ATS mode
> >              if STE.Config == '11x' || constr_unpred_EATS_S2S then
> >                  // if stage 2 enabled or CONSTRAINED UNPREDICTABLE for SMMUv3.0
> >                  return TRUE;
> > 
> > So, if master->stall_enabled and master->ats_enabled, there would
> > be a bad STE?
> 
> Indeed, but as discussed previously, to get there would require either
> firmware or hardware to bogusly advertise both stall and ATS
> capabilities for the same device, which we decided is beyond the scope
> of what's worth trying to reason about. If a nonsensical system leads to
> obviously blowing up with C_BAD_STE, that's arguably not such a bad thing.

Oh, I see. Thanks for the note!

Nicolin