On 06/24/2014 08:41 PM, Alexander Graf wrote:
On 24.06.14 12:11, Alexey Kardashevskiy wrote:iowrite32 is a non-inline function so conversions take place so are the
On 06/21/2014 09:12 AM, Benjamin Herrenschmidt wrote:Is there actually any difference in generated code with this patch applied
On Thu, 2014-06-19 at 21:21 -0600, Alex Williamson wrote:Ping?
Working on big endian being an accident may be a matter of perspective:-)
The comment remains that this patch doesn't actually fix anything exceptYes, that works by accident because technically VFIO is a transport and
the overhead on big endian systems doing redundant byte swapping and
maybe the philosophy that vfio regions are little endian.
thus shouldn't perform any endian swapping of any sort, which remains
the responsibility of the end driver which is the only one to know
whether a given BAR location is a a register or some streaming data
and in the former case whether it's LE or BE (some PCI devices are BE
even ! :-)
But yes, in the end, it works with the dual "cancelling" swaps and the
overhead of those swaps is probably drowned in the noise of the syscall
overhead.
I'm still not a fan of iowrite vs iowritebe, there must be something weSadly there isn't ... In the old day we didn't even have the "be"
can use that doesn't have an implicit swap.
variant and readl/writel style accessors still don't have them either
for all archs.
There is __raw_readl/writel but here the semantics are much more than
just "don't swap", they also don't have memory barriers (which means
they are essentially useless to most drivers unless those are platform
specific drivers which know exactly what they are doing, or in the rare
cases such as accessing a framebuffer which we know never have side
effects).
Calling it iowrite*_native is also an abuse of the namespace.I might make sense to those definitions into a common header. There have
Next thing we know some common code
will legitimately use that name.
been a handful of cases in the past that wanted that sort of "native
byte order" MMIOs iirc (though don't ask me for examples, I can't really
remember).
If we do need to define an alias
(which I'd like to avoid) it should be something like vfio_iowrite32.
We need to make a decision whether to move those xxx_native() helpers
somewhere (where?) or leave the patch as is (as we figured out that
iowriteXX functions implement barriers and we cannot just use raw
accessors) and fix commit log to explain everything.
and without? I would hope that iowrite..() is inlined and cancels out the
cpu_to_le..() calls that are also inlined?
others. And sorry but I fail to see why this matters. We are not trying to
accelerate things, we are removing redundant operations which confuse
people who read the code.