Re: [PATCH] clk: explicitly disable CONFIG_UML_PCI_OVER_VIRTIO in .kunitconfig

[David Gow]

On Tue, Jul 12, 2022 at 7:03 AM Daniel Latypov <dlatypov@xxxxxxxxxx> wrote:
>
> On Mon, Jul 11, 2022 at 1:49 PM Stephen Boyd <sboyd@xxxxxxxxxx> 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.

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

Attachment: smime.p7s
Description: S/MIME Cryptographic Signature