Re: [kvm-unit-tests PATCH v2 11/12] lib: arm64: gic-v3-its: Add wmb() barrier before INT command

["André Przywara" <andre.przywara@arm.com>]

On 17/12/2020 14:13, Alexandru Elisei wrote:
> The ITS tests use the INT command like an SGI. The its_send_int() function
> kicks a CPU and then the test checks that the interrupt was observed as
> expected in check_lpi_stats(). This is done by using lpi_stats.observed and
> lpi_stats.expected, where the target CPU only writes to lpi_stats.observed,
> and the source CPU reads it and compares the values with
> lpi_stats.expected.
> 
> The fact that the target CPU doesn't read data written by the source CPU
> means that we don't need to do inter-processor memory synchronization
> for that between the two at the moment.
> 
> The acked array is used by its-pending-migration test, but the reset value
> for acked (zero) is the same as the initialization value for static
> variables, so memory synchronization is again not needed.
> 
> However, that is all about to change when we modify all ITS tests to use
> the same functions as the IPI tests. Add a write memory barrier to
> its_send_int(), similar to the gicv3_ipi_send_mask(), which has similar
> semantics.

I agree to the requirement for having the barrier, but am not sure this
is the right place. Wouldn't it be better to have the barrier in the
callers?

Besides: This command is written to the command queue (in normal
memory), then we notify the ITS via an MMIO writeq. And this one has a
"wmb" barrier already (though for other reasons).

Cheers,
Andre


> 
> Suggested-by: Auger Eric <eric.auger@redhat.com>
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> ---
>  lib/arm64/gic-v3-its-cmd.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/lib/arm64/gic-v3-its-cmd.c b/lib/arm64/gic-v3-its-cmd.c
> index 34574f71d171..32703147ee85 100644
> --- a/lib/arm64/gic-v3-its-cmd.c
> +++ b/lib/arm64/gic-v3-its-cmd.c
> @@ -385,6 +385,12 @@ void __its_send_int(struct its_device *dev, u32 event_id, bool verbose)
>  {
>  	struct its_cmd_desc desc;
>  
> +	/*
> +	 * The INT command is used by tests as an IPI. Ensure stores to Normal
> +	 * memory are visible to other CPUs before sending the LPI.
> +	 */
> +	wmb();
> +
>  	desc.its_int_cmd.dev = dev;
>  	desc.its_int_cmd.event_id = event_id;
>  	desc.verbose = verbose;
>