Re: [PULL 21/26] hw/arm/smmu: Refactor SMMU OAS

[Peter Maydell]

On Sat, 20 Jul 2024 at 23:07, Mostafa Saleh <smostafa@google.com> wrote:
>
> Hi Peter,
>
> On Sat, Jul 20, 2024 at 04:05:40PM +0100, Peter Maydell wrote:
> > On Thu, 18 Jul 2024 at 14:20, Peter Maydell <peter.maydell@linaro.org> 
> > wrote:
> > >
> > > From: Mostafa Saleh <smostafa@google.com>
> > >
> > > SMMUv3 OAS is currently hardcoded in the code to 44 bits, for nested
> > > configurations that can be a problem, as stage-2 might be shared with
> > > the CPU which might have different PARANGE, and according to SMMU manual
> > > ARM IHI 0070F.b:
> > >     6.3.6 SMMU_IDR5, OAS must match the system physical address size.
> > >
> > > This patch doesn't change the SMMU OAS, but refactors the code to
> > > make it easier to do that:
> > > - Rely everywhere on IDR5 for reading OAS instead of using the
> > >   SMMU_IDR5_OAS macro, so, it is easier just to change IDR5 and
> > >   it propagages correctly.
> > > - Add additional checks when OAS is greater than 48bits.
> > > - Remove unused functions/macros: pa_range/MAX_PA.
> >
> > Hi; Coverity has spotted a possible problem with the OAS handling
> > in this code (CID 1558464). I'm not sure if that's directly because of
> > this patch or if it's just that the code change has caused Coverity to
> > flag up a preexisting problem.
> >
> > It's quite possible this is a false-positive because Coverity hasn't
> > noticed that the situation can't happen; but if so I think it's also
> > sufficiently unclear to a human reader to be worth addressing anyway.
> >
> > > -static int decode_ste_s2_cfg(SMMUTransCfg *cfg, STE *ste)
> > > +static int decode_ste_s2_cfg(SMMUv3State *s, SMMUTransCfg *cfg,
> > > +                             STE *ste)
> > >  {
> > > +    uint8_t oas = FIELD_EX32(s->idr[5], IDR5, OAS);
> > > +
> > >      if (STE_S2AA64(ste) == 0x0) {
> > >          qemu_log_mask(LOG_UNIMP,
> > >                        "SMMUv3 AArch32 tables not supported\n");
> > > @@ -460,7 +463,15 @@ static int decode_ste_s2_cfg(SMMUTransCfg *cfg, STE 
> > > *ste)
> > >      }
> > >
> > >      /* For AA64, The effective S2PS size is capped to the OAS. */
> > > -    cfg->s2cfg.eff_ps = oas2bits(MIN(STE_S2PS(ste), SMMU_IDR5_OAS));
> > > +    cfg->s2cfg.eff_ps = oas2bits(MIN(STE_S2PS(ste), oas));
> >
> > oas2bits() is implemented as a function that returns -1 if the input
> > isn't a valid OAS. But we don't check for that failure here, so we put
> > the result into a uint8_t, which ends up as 255. Then later in
> > the function we will do
> >   MAKE_64BIT_MASK(0, cfg->s2cfg.eff_ps)
> > which will do an undefined-behaviour shift by a negative number
> > if eff_ps is 255.
> >
> > If the invalid-OAS case is impossible we should assert rather
> > than returning -1; if it's not impossible we should handle it.
> >
> > Mostafa, could you have a look at this, please?
>
> Yes, it should be impossible to have an invalid OAS.
>
> This patch doesn't change the old behaviour, it just consolidate OAS
> setting in one place, instead hardcoding it everywhere, so here
> instead of using the macro (SMMU_IDR5_OAS) directly we now read it
> from IDR5, which is set to SMMU_IDR5_OAS at smmuv3_init_regs().
>
> The other field S2PS is casted to 6 bits, and as we use MIN, and
> all the previous values are valid, so it should be fine:
> - 0b000: 32 bits
> - 0b001: 36 bits
> - 0b010: 40 bits
> - 0b011: 42 bits
> - 0b100: 44 bits
>
> Adding an assertion makes sense to me. Please, let me know if you
> want me to send a patch for that.

Yes, if this can't happen even with invalid guest input, please
send a patch that asserts that.

thanks
-- PMM