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

[Mostafa Saleh <smostafa@google.com>]

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.

Thanks,
Mostafa

> 
> thanks
> -- PMM
>