Re: [kvm-unit-tests PATCH v1 1/4] arm64: split its-trigger test into KVM and TCG variants

[Alexandru Elisei]

Hi,

On 4/28/21 1:06 PM, Alex Bennée wrote:
> Marc Zyngier <maz@kernel.org> writes:
>
>> On 2021-04-28 11:18, Alex Bennée wrote:
>>> A few of the its-trigger tests rely on IMPDEF behaviour where caches
>>> aren't flushed before invall events. However TCG emulation doesn't
>>> model any invall behaviour and as we can't probe for it we need to be
>>> told. Split the test into a KVM and TCG variant and skip the invall
>>> tests when under TCG.
>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>> Cc: Shashi Mallela <shashi.mallela@linaro.org>
>>> ---
>>>  arm/gic.c         | 60 +++++++++++++++++++++++++++--------------------
>>>  arm/unittests.cfg | 11 ++++++++-
>>>  2 files changed, 45 insertions(+), 26 deletions(-)
>>> diff --git a/arm/gic.c b/arm/gic.c
>>> index 98135ef..96a329d 100644
>>> --- a/arm/gic.c
>>> +++ b/arm/gic.c
>>> @@ -36,6 +36,7 @@ static struct gic *gic;
>>>  static int acked[NR_CPUS], spurious[NR_CPUS];
>>>  static int irq_sender[NR_CPUS], irq_number[NR_CPUS];
>>>  static cpumask_t ready;
>>> +static bool under_tcg;
>>>  static void nr_cpu_check(int nr)
>>>  {
>>> @@ -734,32 +735,38 @@ static void test_its_trigger(void)
>>>     /*
>>>      * re-enable the LPI but willingly do not call invall
>>>      * so the change in config is not taken into account.
>>> -    * The LPI should not hit
>>> +    * The LPI should not hit. This does however depend on
>>> +    * implementation defined behaviour - under QEMU TCG emulation
>>> +    * it can quite correctly process the event directly.
>> It looks to me that you are using an IMPDEF behaviour of *TCG*
>> here. The programming model mandates that there is an invalidation
>> if you change the configuration of the LPI.
> But does it mandate that the LPI cannot be sent until the invalidation?

I think Marc is referring to this section of the GIC architecture (Arm IHI 
0069F,
page 5-82, I've highlighted the interesting bits):

"A Redistributor can cache the information from the LPI Configuration tables
pointed to by GICR_PROPBASER, when GICR_CTLR.EnableLPI == 1, subject to all of 
the
following rules:
* Whether or not one or more caches are present is IMPLEMENTATION DEFINED. Where
at least one cache is present, the structure and size is IMPLEMENTATION DEFINED.
* An LPI Configuration table entry might be allocated into the cache at any 
time.
* A cached LPI Configuration table entry is not guaranteed to remain in the 
cache.
* A cached LPI Configuration table entry *is not guaranteed to remain incoherent
with memory*.
* A change to the LPI configuration *is not guaranteed to be visible until an
appropriate invalidation operation has completed*"

I interpret that as that an INVALL guarantees that a change is visible, but it 
the
change can become visible even without the INVALL.

The test relies on the fact that changes to the LPI tables are not visible 
*under
KVM* until the INVALL command, but that's not necessarily the case on real
hardware. To match the spec, I think the test "dev2/eventid=20 still does not
trigger any LPI" should be removed and the stats reset should take place before
the configuration for LPI 8195 is set to the default.

Thanks,

Alex