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

[Peter Maydell]

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?

thanks
-- PMM