| Message ID | 20201217141400.106137-12-alexandru.elisei@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | GIC fixes and improvements | expand |
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; >
Hi Andre, On 12/18/20 6:36 PM, André Przywara wrote: > 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). Had another look, and you're totally right: its_post_commands() already has a wmb() in writeq(), thank you for pointing it out. This makes the wmb() which I've added pointless, I'll drop the patch since it doesn't add useful. Thanks, Alex > > 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; >>
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;
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. 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(+)