Re: [PATCH 20/26] hw/intc/arm_gicv3_its: Use enum for return value of process_* functions

[Philippe Mathieu-Daudé]

On 12/11/21 20:11, Peter Maydell wrote:
> When an ITS detects an error in a command, it has an
> implementation-defined (CONSTRAINED UNPREDICTABLE) choice of whether
> to ignore the command, proceeding to the next one in the queue, or to
> stall the ITS command queue, processing nothing further.  The
> behaviour required when the read of the command packet from memory
> fails is less clearly documented, but the same set of choices as for
> command errors seem reasonable.
> 
> The intention of the QEMU implementation, as documented in the
> comments, is that if we encounter a memory error reading the command
> packet or one of the various data tables then we should stall, but
> for command parameter errors we should ignore the queue and continue.
> However, we don't actually do this.  To get the desired behaviour,
> the various process_* functions need to return true to cause
> process_cmdq() to advance to the next command and keep processing,
> and false to stall command processing.  What they mostly do is return
> false for any kind of error.
> 
> To make the code clearer, replace the 'bool' return from the process_
> functions with an enum which may be either CMD_STALL or CMD_CONTINUE.
> In this commit no behaviour changes; in subsequent commits we will
> adjust the error-return paths for the process_ functions one by one.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/intc/arm_gicv3_its.c | 59 ++++++++++++++++++++++++++---------------
>  1 file changed, 38 insertions(+), 21 deletions(-)
> 
> diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c
> index f3eba92946d..59dd564d91c 100644
> --- a/hw/intc/arm_gicv3_its.c
> +++ b/hw/intc/arm_gicv3_its.c
> @@ -45,6 +45,23 @@ typedef struct {
>      uint64_t itel;
>  } IteEntry;
>  
> +/*
> + * The ITS spec permits a range of CONSTRAINED UNPREDICTABLE options
> + * if a command parameter is not correct. These include both "stall
> + * processing of the command queue" and "ignore this command, and
> + * keep processing the queue". In our implementation we choose that
> + * memory transaction errors reading the command packet provoke a
> + * stall, but errors in parameters cause us to ignore the command
> + * and continue processing.
> + * The process_* functions which handle invididual ITS commands all

Typo "individual".

> + * return an ItsCmdResult which tells process_cmdq() whether it should
> + * stall or keep going.
> + */
> +typedef enum ItsCmdResult {
> +    CMD_STALL = 0,
> +    CMD_CONTINUE = 1,
> +} ItsCmdResult;

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>