[PATCH] clk: explicitly disable CONFIG_UML_PCI_OVER_VIRTIO in .kunitconfig

[Daniel Latypov]

CONFIG_UML_PCI_OVER_VIRTIO=y is needed to enable CONFIG_PCI=y on UML.
However, this causes test failures when running the clk tests, i.e.
$ ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/clk

A snippet of the particular error is:
> ok 1 - clk_gate_test_parent_rate
> ------------[ cut here ]------------
> WARNING: CPU: 0 PID: 45 at lib/logic_iomem.c:141 __raw_readl+0x9f/0xd0

This is triggered by this cast in the test:
143 ctx->fake_mem = (void __force __iomem *)&ctx->fake_reg;
this seems to work except when logic iomem is enabled, i.e.
CONFIG_INDIRECT_IOMEM=y.

As a short-term fix, explicitly disable CONFIG_UML_PCI_OVER_VIRTIO in
drivers/clk/.kunitconfig so we can enable it for everyone else by
default in kunit.py.

The long-term fix probably requires something more complicated, like
#ifdef CONFIG_INDIRECT_IOMEM
logic_iomem_add_region(...);
#endif

Signed-off-by: Daniel Latypov <dlat...@google.com>
Reported-by: Maxime Ripard <max...@cerno.tech>
Tested-by: Maxime Ripard <max...@cerno.tech>
---
Note: this targeting the -kselftest kunit branch.
There's a commit that triggers this by enabling logic iomem by default
[1] and there's also a commit that lets disable it via kunitconfig file
[2], which this fix relies on.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git/commit/?h=kunit&id=6fc3a8636a7b0f7dbd6d0a4e450e765dc17518d4
[2] https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git/commit/?h=kunit&id=8a7c6f859a20ca36a9e3ce71662de697898c9ef5
---
drivers/clk/.kunitconfig | 1 +
1 file changed, 1 insertion(+)

diff --git a/drivers/clk/.kunitconfig b/drivers/clk/.kunitconfig
index cdbc7d7deba9..2fbeb71316f8 100644
--- a/drivers/clk/.kunitconfig
+++ b/drivers/clk/.kunitconfig
@@ -2,3 +2,4 @@ CONFIG_KUNIT=y
CONFIG_COMMON_CLK=y
CONFIG_CLK_KUNIT_TEST=y
CONFIG_CLK_GATE_KUNIT_TEST=y
+CONFIG_UML_PCI_OVER_VIRTIO=n

base-commit: 7635778bac7e46458392c1261e3916e8e9e86860
--
2.37.0.rc0.161.g10f37bed90-goog

[Stephen Boyd]

Quoting Daniel Latypov (2022-07-11 09:27:13)

> CONFIG_UML_PCI_OVER_VIRTIO=y is needed to enable CONFIG_PCI=y on UML.
> However, this causes test failures when running the clk tests, i.e.
> $ ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/clk
>
> A snippet of the particular error is:
> > ok 1 - clk_gate_test_parent_rate
> > ------------[ cut here ]------------
> > WARNING: CPU: 0 PID: 45 at lib/logic_iomem.c:141 __raw_readl+0x9f/0xd0
>
> This is triggered by this cast in the test:
> 143 ctx->fake_mem = (void __force __iomem *)&ctx->fake_reg;
> this seems to work except when logic iomem is enabled, i.e.
> CONFIG_INDIRECT_IOMEM=y.
>
> As a short-term fix, explicitly disable CONFIG_UML_PCI_OVER_VIRTIO in
> drivers/clk/.kunitconfig so we can enable it for everyone else by
> default in kunit.py.
>
> The long-term fix probably requires something more complicated, like
> #ifdef CONFIG_INDIRECT_IOMEM
> logic_iomem_add_region(...);
> #endif

If this is how platform IO is mocked then yes we'll have to implement
that.

>
> Signed-off-by: Daniel Latypov <dlat...@google.com>
> Reported-by: Maxime Ripard <max...@cerno.tech>
> Tested-by: Maxime Ripard <max...@cerno.tech>
> ---
> Note: this targeting the -kselftest kunit branch.
> There's a commit that triggers this by enabling logic iomem by default
> [1] and there's also a commit that lets disable it via kunitconfig file
> [2], which this fix relies on.
>
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git/commit/?h=kunit&id=6fc3a8636a7b0f7dbd6d0a4e450e765dc17518d4
> [2] https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git/commit/?h=kunit&id=8a7c6f859a20ca36a9e3ce71662de697898c9ef5
> ---

Acked-by: Stephen Boyd <sb...@kernel.org>

[Daniel Latypov]

On Mon, Jul 11, 2022 at 1:49 PM Stephen Boyd <sb...@kernel.org> wrote:
>
> Quoting Daniel Latypov (2022-07-11 09:27:13)
> > CONFIG_UML_PCI_OVER_VIRTIO=y is needed to enable CONFIG_PCI=y on UML.
> > However, this causes test failures when running the clk tests, i.e.
> > $ ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/clk
> >
> > A snippet of the particular error is:
> > > ok 1 - clk_gate_test_parent_rate
> > > ------------[ cut here ]------------
> > > WARNING: CPU: 0 PID: 45 at lib/logic_iomem.c:141 __raw_readl+0x9f/0xd0
> >
> > This is triggered by this cast in the test:
> > 143 ctx->fake_mem = (void __force __iomem *)&ctx->fake_reg;
> > this seems to work except when logic iomem is enabled, i.e.
> > CONFIG_INDIRECT_IOMEM=y.
> >
> > As a short-term fix, explicitly disable CONFIG_UML_PCI_OVER_VIRTIO in
> > drivers/clk/.kunitconfig so we can enable it for everyone else by
> > default in kunit.py.
> >
> > The long-term fix probably requires something more complicated, like
> > #ifdef CONFIG_INDIRECT_IOMEM
> > logic_iomem_add_region(...);
> > #endif
>
> If this is how platform IO is mocked then yes we'll have to implement
> that.

It's not clear that it will be, but it's likely.
Until it's more clear, I figured we'd go with this simple bandaid fix for now.

There was initially some talk of using function-level
"stubbing"/"mocking" [1] to do the same, but Brendan started playing
around with logic iomem more. He and David can comment on it more
cogently than I can.

[1] https://lore.kernel.org/linux-kselftest/20220318021314.3...@google.com/

Daniel

[David Gow]

So, as I understand it, casting a regular pointer to an __iomem
pointer (as the clk test does) isn't technically correct, though it
does work on almost every architecture out there. If we want some way
of intercepting I/O access, then then that'll need to be handled by
the various read()/write() functions.

UML doesn't support iomem at all out of the box, and logic_iomem is a
way of implementing it which allow us to attach handler functions to
blocks of memory, albeit with more constraints about what addresses
get used. Brendan started implementing a "fake hardware" interface on
top of this here, though it's still in-progress:
https://kunit-review.googlesource.com/c/linux/+/5272/4

Ultimately, I think the 'correct' solution here will be
logic_iomem-based, but doing that nicely will probably require one of
two things:
- logic_iomem to support non-UML architectures as well (becoming a
generic "insert a 'fake' device here" system)
- logic_iomem to have some way of "passing through" access to an io
memory region through to the normal underlying memory.

Ideally, we'll have both, and maybe even defaults which will allow
hacks like this to continue working (perhaps with a warning?). That'll
require some significant (and probably not uncontroversial) work on
how iomem accesses work in general, though, possibly with performance
impact.

The other option of using function redirection on the io read()
write() functions exists, and would be a bit simpler in the
short-term, but would probably result in a lot of tests reimplementing
this, and also would have some performance impacts, as the I/O
accesses wouldn't be able to be inlined if KUNIT is enabled.

Cheers,
-- David

[David Gow]

Looks good, works for me (and doesn't break either i386 or arm64,
which I also tested on).

Reviewed-by: David Gow <davi...@google.com>

Cheers,
-- David

> drivers/clk/.kunitconfig | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/clk/.kunitconfig b/drivers/clk/.kunitconfig
> index cdbc7d7deba9..2fbeb71316f8 100644
> --- a/drivers/clk/.kunitconfig
> +++ b/drivers/clk/.kunitconfig
> @@ -2,3 +2,4 @@ CONFIG_KUNIT=y
> CONFIG_COMMON_CLK=y
> CONFIG_CLK_KUNIT_TEST=y
> CONFIG_CLK_GATE_KUNIT_TEST=y
> +CONFIG_UML_PCI_OVER_VIRTIO=n
>
> base-commit: 7635778bac7e46458392c1261e3916e8e9e86860
> --
> 2.37.0.rc0.161.g10f37bed90-goog
>

> --
> You received this message because you are subscribed to the Google Groups "KUnit Development" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kunit-dev+...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20220711162713.2467476-1-dlatypov%40google.com.

[Brendan Higgins]

On Mon, Jul 11, 2022 at 12:27 PM 'Daniel Latypov' via KUnit
Development <kuni...@googlegroups.com> wrote:
>
> CONFIG_UML_PCI_OVER_VIRTIO=y is needed to enable CONFIG_PCI=y on UML.
> However, this causes test failures when running the clk tests, i.e.
> $ ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/clk
>
> A snippet of the particular error is:
> > ok 1 - clk_gate_test_parent_rate
> > ------------[ cut here ]------------
> > WARNING: CPU: 0 PID: 45 at lib/logic_iomem.c:141 __raw_readl+0x9f/0xd0
>
> This is triggered by this cast in the test:
> 143 ctx->fake_mem = (void __force __iomem *)&ctx->fake_reg;
> this seems to work except when logic iomem is enabled, i.e.
> CONFIG_INDIRECT_IOMEM=y.
>
> As a short-term fix, explicitly disable CONFIG_UML_PCI_OVER_VIRTIO in
> drivers/clk/.kunitconfig so we can enable it for everyone else by
> default in kunit.py.
>
> The long-term fix probably requires something more complicated, like
> #ifdef CONFIG_INDIRECT_IOMEM
> logic_iomem_add_region(...);
> #endif
>
> Signed-off-by: Daniel Latypov <dlat...@google.com>
> Reported-by: Maxime Ripard <max...@cerno.tech>
> Tested-by: Maxime Ripard <max...@cerno.tech>

Acked-by: Brendan Higgins <brendan...@google.com>

[Stephen Boyd]

Quoting David Gow (2022-07-11 23:44:08)

>
> So, as I understand it, casting a regular pointer to an __iomem
> pointer (as the clk test does) isn't technically correct, though it
> does work on almost every architecture out there. If we want some way
> of intercepting I/O access, then then that'll need to be handled by
> the various read()/write() functions.

Yep. It's test code though so it seemed ok at the time.

>
> UML doesn't support iomem at all out of the box, and logic_iomem is a
> way of implementing it which allow us to attach handler functions to
> blocks of memory, albeit with more constraints about what addresses
> get used. Brendan started implementing a "fake hardware" interface on
> top of this here, though it's still in-progress:
> https://kunit-review.googlesource.com/c/linux/+/5272/4

Cool.

>
> Ultimately, I think the 'correct' solution here will be
> logic_iomem-based, but doing that nicely will probably require one of
> two things:
> - logic_iomem to support non-UML architectures as well (becoming a
> generic "insert a 'fake' device here" system)
> - logic_iomem to have some way of "passing through" access to an io
> memory region through to the normal underlying memory.
>
> Ideally, we'll have both, and maybe even defaults which will allow
> hacks like this to continue working (perhaps with a warning?). That'll
> require some significant (and probably not uncontroversial) work on
> how iomem accesses work in general, though, possibly with performance
> impact.

Does it matter to pass through to real iomem? I'd think we wouldn't want
to actually affect real hardware in test code. Instead we'd like to fake
it and then look at the result, like how the clk test works.

>
> The other option of using function redirection on the io read()
> write() functions exists, and would be a bit simpler in the
> short-term, but would probably result in a lot of tests reimplementing
> this, and also would have some performance impacts, as the I/O
> accesses wouldn't be able to be inlined if KUNIT is enabled.
>

That sounds OK to me because nobody is enabling KUNIT in production,
right?

[Daniel Latypov]

On Tue, Jul 26, 2022 at 1:07 PM Stephen Boyd <sb...@kernel.org> wrote:

> > The other option of using function redirection on the io read()
> > write() functions exists, and would be a bit simpler in the
> > short-term, but would probably result in a lot of tests reimplementing
> > this, and also would have some performance impacts, as the I/O
> > accesses wouldn't be able to be inlined if KUNIT is enabled.
> >
>
> That sounds OK to me because nobody is enabling KUNIT in production,
> right?

You'd think so, but...

Android GKI [1] is enabling it. I had thought as =m, but maybe it was =y.
I can't find a good link for it now, but e.g. here's a patch they're
carrying to prevent KUnit from running built-in tests [2]

They could just carry another patch if we go down this route, but it
goes to show that there might be others out there w/ reasons to do
this.

[1] https://source.android.com/devices/architecture/kernel/generic-kernel-image
[2] https://android.googlesource.com/kernel/common-patches/+/refs/heads/master/android-mainline/ANDROID-GKI-Disable-KUnit-built-in-testing.patch

[David Gow]

The only case I'm worried about is where real iomem is necessary to
boot to the point where tests run. It won't affect UML, but if you
were (e.g.) running tests as modules on real hardware (or under qemu),
and iomem was required to access the disk containing these modules,
that would be a case for having both real and fake iomem supported on
the same system.

Given the sheer number of things which depend on iomem in some regard,
I suspect this'd affect pretty much every architecture except UML.

> >
> > The other option of using function redirection on the io read()
> > write() functions exists, and would be a bit simpler in the
> > short-term, but would probably result in a lot of tests reimplementing
> > this, and also would have some performance impacts, as the I/O
> > accesses wouldn't be able to be inlined if KUNIT is enabled.
> >
>
> That sounds OK to me because nobody is enabling KUNIT in production,
> right?

As Daniel noted, Android is looking at enabling KUNIT in production
(but tainting the system if any actual tests run). That's a pretty
unusual use-case, though, so it definitely doesn't preclude people
from redirecting functions, particularly if ftrace-based stubbing is
used to avoid the overhead when disabled (though I can't recall if
that works on ARM, so Android might be out of luck for those tests
anyway...)

But yeah, I'm definitely not considering the potential performance
impact a blocker here, just something to be aware of.

Either way, neither the stubbing support or the logic_iomem stuff is
quite ready yet, so disabling CONFIG_UML_PCI_OVER_VIRTIO is the right
solution in the meantime.

Cheers,
-- David