Re: [PATCH for-8.2 3/3] hw/arm: Set number of MPU regions correctly for an505, an521, an524

[Richard Henderson <richard.henderson@linaro.org>]

On 7/24/23 10:43, Peter Maydell wrote:
> The IoTKit, SSE200 and SSE300 all default to 8 MPU regions.  The
> MPS2/MPS3 FPGA images don't override these except in the case of
> AN547, which uses 16 MPU regions.
> 
> Define properties on the ARMSSE object for the MPU regions (using the
> same names as the documented RTL configuration settings, and
> following the pattern we already have for this device of using
> all-caps names as the RTL does), and set them in the board code.
> 
> We don't actually need to override the default except on AN547,
> but it's simpler code to have the board code set them always
> rather than tracking which board subtypes want to set them to
> a non-default value separately from what that value is.
> 
> Tho overall effect is that for mps2-an505, mps2-an521 and mps3-an524
> we now correctly use 8 MPU regions, while mps3-an547 stays at its
> current 16 regions.
> 
> It's possible some guest code wrongly depended on the previous
> incorrectly modeled number of memory regions. (Such guest code
> should ideally check the number of regions via the MPU_TYPE
> register.) The old behaviour can be obtained with additional
> -global arguments to QEMU:
> 
> For mps2-an521 and mps2-an524:
>   -global sse-200.CPU0_MPU_NS=16 -global sse-200.CPU0_MPU_S=16 -global sse-200.CPU1_MPU_NS=16 -global sse-200.CPU1_MPU_S=16
> 
> For mps2-an505:
>   -global sse-200.CPU0_MPU_NS=16 -global sse-200.CPU0_MPU_S=16
> 
> NB that the way the implementation allows this use of -global
> is slightly fragile: if the board code explicitly sets the
> properties on the sse-200 object, this overrides the -global
> command line option. So we rely on:
>   - the boards that need fixing all happen to use the SSE defaults
>   - we can write the board code to only set the property if it
>     is different from the default, rather than having all boards
>     explicitly set the property
>   - the board that does need to use a non-default value happens
>     to need to set it to the same value (16) we previously used
> This works, but there are some kinds of refactoring of the
> mps2-tz.c code that would break the support for -global here.
> 
> Resolves:https://gitlab.com/qemu-project/qemu/-/issues/1772
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
> I'm not super-enthusiastic about the -global handling here, as you
> may have guessed from the wording above, though it does avoid having
> explicit back-compat code.  The other option for back-compat would be
> to add an explicit board property to say "use the old values".
> ---
>   include/hw/arm/armsse.h |  5 +++++
>   hw/arm/armsse.c         | 16 ++++++++++++++++
>   hw/arm/mps2-tz.c        | 29 +++++++++++++++++++++++++++++
>   3 files changed, 50 insertions(+)

Looks reasonable.  I can't think of any global properties that are better.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~