On Wed, Jul 27, 2022 at 4:07 AM Stephen Boyd sboyd@kernel.org wrote:
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 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