| Message ID | 20210428101844.22656-2-alex.bennee@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | enable LPI and ITS for TCG | expand |
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. M.
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? > > M.
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
On Wed, 28 Apr 2021 15:00:15 +0100, Alexandru Elisei <alexandru.elisei@arm.com> wrote: > > I interpret that as that an INVALL guarantees that a change is > visible, but it the change can become visible even without the > INVALL. Yes. Expecting the LPI to be delivered or not in the absence of an invalidate when its configuration has been altered is wrong. The architecture doesn't guarantee anything of the sort. > 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. If that's what the test expects (I haven't tried to investigate), it should be dropped completely, rather than trying to sidestep it for TCG. Thanks, M.
Hi, On 4/28/21 4:36 PM, Marc Zyngier wrote: > On Wed, 28 Apr 2021 15:00:15 +0100, > Alexandru Elisei <alexandru.elisei@arm.com> wrote: >> >> I interpret that as that an INVALL guarantees that a change is >> visible, but it the change can become visible even without the >> INVALL. > > Yes. Expecting the LPI to be delivered or not in the absence of an > invalidate when its configuration has been altered is wrong. The > architecture doesn't guarantee anything of the sort. > >> 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. Yes I do agree with Alexandru and Marc after another reading of the spec. I initially thought the INVALL was the gate keeper for the new config but that sounds wrong. This test shall be removed then. Eric > > If that's what the test expects (I haven't tried to investigate), it > should be dropped completely, rather than trying to sidestep it for > TCG. > > Thanks, > > M. >
Marc Zyngier <maz@kernel.org> writes: > On Wed, 28 Apr 2021 15:00:15 +0100, > Alexandru Elisei <alexandru.elisei@arm.com> wrote: >> >> I interpret that as that an INVALL guarantees that a change is >> visible, but it the change can become visible even without the >> INVALL. > > Yes. Expecting the LPI to be delivered or not in the absence of an > invalidate when its configuration has been altered is wrong. The > architecture doesn't guarantee anything of the sort. Is the underlying hypervisor allowed to invalidate and reload the configuration whenever it wants or should it only be driven by the guests requests? I did consider a more nuanced variant of the test that allowed for a delivery pre-inval and a pass for post-inval as long as it had been delivered one way or another: --8<---------------cut here---------------start------------->8--- modified 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) { @@ -687,6 +688,7 @@ static void test_its_trigger(void) struct its_collection *col3; struct its_device *dev2, *dev7; cpumask_t mask; + bool before, after; if (its_setup1()) return; @@ -734,15 +736,17 @@ 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. */ gicv3_lpi_set_config(8195, LPI_PROP_DEFAULT); stats_reset(); cpumask_clear(&mask); its_send_int(dev2, 20); wait_for_interrupts(&mask); - report(check_acked(&mask, -1, -1), - "dev2/eventid=20 still does not trigger any LPI"); + before = check_acked(&mask, -1, -1); + report_xfail(under_tcg, before, "dev2/eventid=20 still may not trigger any LPI"); /* Now call the invall and check the LPI hits */ stats_reset(); @@ -750,8 +754,8 @@ static void test_its_trigger(void) cpumask_set_cpu(3, &mask); its_send_invall(col3); wait_for_interrupts(&mask); - report(check_acked(&mask, 0, 8195), - "dev2/eventid=20 pending LPI is received"); + after = check_acked(&mask, 0, 8195); + report(before != after, "dev2/eventid=20 pending LPI is received"); stats_reset(); cpumask_clear(&mask); @@ -759,7 +763,7 @@ static void test_its_trigger(void) its_send_int(dev2, 20); wait_for_interrupts(&mask); report(check_acked(&mask, 0, 8195), - "dev2/eventid=20 now triggers an LPI"); + "dev2/eventid=20 now triggers an LPI"); report_prefix_pop(); @@ -981,6 +985,9 @@ int main(int argc, char **argv) if (argc < 2) report_abort("no test specified"); + if (argc == 3 && strcmp(argv[2], "tcg") == 0) + under_tcg = true; + if (strcmp(argv[1], "ipi") == 0) { report_prefix_push(argv[1]); nr_cpu_check(2); --8<---------------cut here---------------end--------------->8--- But that gets confused (that may be something for Sashi to look at): ITS: MAPD devid=2 size = 0x8 itt=0x40440000 valid=1 ITS: MAPD devid=7 size = 0x8 itt=0x40450000 valid=1 MAPC col_id=3 target_addr = 0x30000 valid=1 MAPC col_id=2 target_addr = 0x20000 valid=1 INVALL col_id=2 INVALL col_id=3 MAPTI dev_id=2 event_id=20 -> phys_id=8195, col_id=3 MAPTI dev_id=7 event_id=255 -> phys_id=8196, col_id=2 INT dev_id=2 event_id=20 PASS: gicv3: its-trigger: int: dev=2, eventid=20 -> lpi= 8195, col=3 INT dev_id=7 event_id=255 PASS: gicv3: its-trigger: int: dev=7, eventid=255 -> lpi= 8196, col=2 INV dev_id=2 event_id=20 INT dev_id=2 event_id=20 PASS: gicv3: its-trigger: inv/invall: dev2/eventid=20 does not trigger any LPI INT dev_id=2 event_id=20 INFO: gicv3: its-trigger: inv/invall: interrupts timed-out (5s) INFO: gicv3: its-trigger: inv/invall: cpu3 received wrong irq 8195 INFO: gicv3: its-trigger: inv/invall: ACKS: missing=0 extra=0 unexpected=1 XFAIL: gicv3: its-trigger: inv/invall: dev2/eventid=20 still may not trigger any LPI INVALL col_id=3 INFO: gicv3: its-trigger: inv/invall: interrupts timed-out (5s) INFO: gicv3: its-trigger: inv/invall: ACKS: missing=1 extra=0 unexpected=0 FAIL: gicv3: its-trigger: inv/invall: dev2/eventid=20 pending LPI is received INT dev_id=2 event_id=20 PASS: gicv3: its-trigger: inv/invall: dev2/eventid=20 now triggers an LPI ITS: MAPD devid=2 size = 0x8 itt=0x40440000 valid=0 INT dev_id=2 event_id=20 PASS: gicv3: its-trigger: mapd valid=false: no LPI after device unmap SUMMARY: 7 tests, 1 unexpected failures, 1 expected failures >> 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. > > If that's what the test expects (I haven't tried to investigate), it > should be dropped completely, rather than trying to sidestep it for > TCG. All three parts of that section? report(check_acked(&mask, -1, -1), "dev2/eventid=20 still does not trigger any LPI"); report(check_acked(&mask, 0, 8195), "dev2/eventid=20 pending LPI is received"); report(check_acked(&mask, 0, 8195), "dev2/eventid=20 now triggers an LPI");
Alex Bennée <alex.bennee@linaro.org> writes: > Marc Zyngier <maz@kernel.org> writes: > >> On Wed, 28 Apr 2021 15:00:15 +0100, >> Alexandru Elisei <alexandru.elisei@arm.com> wrote: >>> >>> I interpret that as that an INVALL guarantees that a change is >>> visible, but it the change can become visible even without the >>> INVALL. >> >> Yes. Expecting the LPI to be delivered or not in the absence of an >> invalidate when its configuration has been altered is wrong. The >> architecture doesn't guarantee anything of the sort. > > Is the underlying hypervisor allowed to invalidate and reload the > configuration whenever it wants or should it only be driven by the > guests requests? > > I did consider a more nuanced variant of the test that allowed for a > delivery pre-inval and a pass for post-inval as long as it had been > delivered one way or another: > > --8<---------------cut here---------------start------------->8--- > modified 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) > { > @@ -687,6 +688,7 @@ static void test_its_trigger(void) > struct its_collection *col3; > struct its_device *dev2, *dev7; > cpumask_t mask; > + bool before, after; > > if (its_setup1()) > return; > @@ -734,15 +736,17 @@ 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. > */ > gicv3_lpi_set_config(8195, LPI_PROP_DEFAULT); > stats_reset(); > cpumask_clear(&mask); > its_send_int(dev2, 20); > wait_for_interrupts(&mask); > - report(check_acked(&mask, -1, -1), > - "dev2/eventid=20 still does not trigger any LPI"); > + before = check_acked(&mask, -1, -1); > + report_xfail(under_tcg, before, "dev2/eventid=20 still may not trigger any LPI"); > > /* Now call the invall and check the LPI hits */ > stats_reset(); > @@ -750,8 +754,8 @@ static void test_its_trigger(void) > cpumask_set_cpu(3, &mask); > its_send_invall(col3); > wait_for_interrupts(&mask); > - report(check_acked(&mask, 0, 8195), > - "dev2/eventid=20 pending LPI is received"); > + after = check_acked(&mask, 0, 8195); > + report(before != after, "dev2/eventid=20 pending LPI is > received"); Actually that should be: report(after || !before, "dev2/eventid=20 pending LPI is received"); so either the IRQ arrives after the flush or it had previously.
On Wed, 28 Apr 2021 16:37:45 +0100, Alex Bennée <alex.bennee@linaro.org> wrote: > > > Marc Zyngier <maz@kernel.org> writes: > > > On Wed, 28 Apr 2021 15:00:15 +0100, > > Alexandru Elisei <alexandru.elisei@arm.com> wrote: > >> > >> I interpret that as that an INVALL guarantees that a change is > >> visible, but it the change can become visible even without the > >> INVALL. > > > > Yes. Expecting the LPI to be delivered or not in the absence of an > > invalidate when its configuration has been altered is wrong. The > > architecture doesn't guarantee anything of the sort. > > Is the underlying hypervisor allowed to invalidate and reload the > configuration whenever it wants or should it only be driven by the > guests requests? The HW can do it at any time. It all depends on whether the RD has cached this LPI configuration or not. KVM relies on the required invalidation as a hook to reload the cached state, as it has an infinite LPI configuration cache, while TCG doesn't have a cache at all. Both approaches are valid implementations. > I did consider a more nuanced variant of the test that allowed for a > delivery pre-inval and a pass for post-inval as long as it had been > delivered one way or another: > > --8<---------------cut here---------------start------------->8--- > modified 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) > { > @@ -687,6 +688,7 @@ static void test_its_trigger(void) > struct its_collection *col3; > struct its_device *dev2, *dev7; > cpumask_t mask; > + bool before, after; > > if (its_setup1()) > return; > @@ -734,15 +736,17 @@ 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 This first point is *wrong*. From the architecture spec: <quote> * A change to the LPI configuration is not guaranteed to be visible until an appropriate invalidation operation has completed: - If one or more ITS is implemented, invalidation is performed using the INV or INVALL command. A SYNC command completes the INV and INVALL commands. </quote> *not guaranteed* means that it may fire, it may not. > + * implementation defined behaviour - under QEMU TCG emulation > + * it can quite correctly process the event directly. I really don't see the point in testing IMPDEF behaviours. We should test for architectural compliance, not for implementation choices. M.
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. */ - gicv3_lpi_set_config(8195, LPI_PROP_DEFAULT); - stats_reset(); - cpumask_clear(&mask); - its_send_int(dev2, 20); - wait_for_interrupts(&mask); - report(check_acked(&mask, -1, -1), - "dev2/eventid=20 still does not trigger any LPI"); - - /* Now call the invall and check the LPI hits */ - stats_reset(); - cpumask_clear(&mask); - cpumask_set_cpu(3, &mask); - its_send_invall(col3); - wait_for_interrupts(&mask); - report(check_acked(&mask, 0, 8195), - "dev2/eventid=20 pending LPI is received"); - - stats_reset(); - cpumask_clear(&mask); - cpumask_set_cpu(3, &mask); - its_send_int(dev2, 20); - wait_for_interrupts(&mask); - report(check_acked(&mask, 0, 8195), - "dev2/eventid=20 now triggers an LPI"); + if (under_tcg) { + report_skip("checking LPI triggers without invall"); + } else { + gicv3_lpi_set_config(8195, LPI_PROP_DEFAULT); + stats_reset(); + cpumask_clear(&mask); + its_send_int(dev2, 20); + wait_for_interrupts(&mask); + report(check_acked(&mask, -1, -1), + "dev2/eventid=20 still does not trigger any LPI"); + + /* Now call the invall and check the LPI hits */ + stats_reset(); + cpumask_clear(&mask); + cpumask_set_cpu(3, &mask); + its_send_invall(col3); + wait_for_interrupts(&mask); + report(check_acked(&mask, 0, 8195), + "dev2/eventid=20 pending LPI is received"); + + stats_reset(); + cpumask_clear(&mask); + cpumask_set_cpu(3, &mask); + its_send_int(dev2, 20); + wait_for_interrupts(&mask); + report(check_acked(&mask, 0, 8195), + "dev2/eventid=20 now triggers an LPI"); + } report_prefix_pop(); @@ -981,6 +988,9 @@ int main(int argc, char **argv) if (argc < 2) report_abort("no test specified"); + if (argc == 3 && strcmp(argv[2], "tcg") == 0) + under_tcg = true; + if (strcmp(argv[1], "ipi") == 0) { report_prefix_push(argv[1]); nr_cpu_check(2); diff --git a/arm/unittests.cfg b/arm/unittests.cfg index f776b66..c72dc34 100644 --- a/arm/unittests.cfg +++ b/arm/unittests.cfg @@ -184,13 +184,22 @@ extra_params = -machine gic-version=3 -append 'its-introspection' groups = its arch = arm64 -[its-trigger] +[its-trigger-kvm] file = gic.flat smp = $MAX_SMP +accel = kvm extra_params = -machine gic-version=3 -append 'its-trigger' groups = its arch = arm64 +[its-trigger-tcg] +file = gic.flat +smp = $MAX_SMP +accel = tcg +extra_params = -machine gic-version=3 -append 'its-trigger tcg' +groups = its +arch = arm64 + [its-migration] file = gic.flat smp = $MAX_SMP
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(-)