On Sat, 20 Jul 2024 at 23:07, Mostafa Saleh <smost...@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.mayd...@linaro.org> > > wrote: > > > > > > From: Mostafa Saleh <smost...@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