Re: [PATCH v2] KVM: Specify byte order for KVM_EXIT_MMIO

[Christoffer Dall <christoffer.dall@linaro.org>]

On Sat, Jan 25, 2014 at 12:24:08AM +0000, Peter Maydell wrote:
> On 24 January 2014 23:51, Scott Wood <scottwood@freescale.com> wrote:
> > On Fri, 2014-01-24 at 15:39 -0800, Christoffer Dall wrote:
> >> The KVM API documentation is not clear about the semantics of the data
> >> field on the mmio struct on the kvm_run struct.
> >>
> >> This has become problematic when supporting ARM guests on big-endian
> >> host systems with guests of both endianness types, because it is unclear
> >> how the data should be exported to user space.
> >>
> >> This should not break with existing implementations as all supported
> >> existing implementations of known user space applications (QEMU and
> >> kvmtools for virtio) only support default endianness of the
> >> architectures on the host side.
> >>
> >> Cc: Marc Zyngier <marc.zyngier@arm.com>
> >> Cc: Peter Maydell <peter.maydell@linaro.org>
> >> Cc: Alexander Graf <agraf@suse.de>
> >> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> >> ---
> >> Changes [v1 - v2]:
> >>  - s/host kernel should/host user space should/
> >>
> >>  Documentation/virtual/kvm/api.txt | 5 +++++
> >>  1 file changed, 5 insertions(+)
> >>
> >> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> >> index 366bf4b..6dbd68c 100644
> >> --- a/Documentation/virtual/kvm/api.txt
> >> +++ b/Documentation/virtual/kvm/api.txt
> >> @@ -2565,6 +2565,11 @@ executed a memory-mapped I/O instruction which could not be satisfied
> >>  by kvm.  The 'data' member contains the written data if 'is_write' is
> >>  true, and should be filled by application code otherwise.
> >>
> >> +The 'data' member byte order is host kernel native endianness, regardless of
> >> +the endianness of the guest, and represents the the value as it would go on the
> >> +bus in real hardware.  The host user space should always be able to do:
> >> +<type> val = *((<type> *)mmio.data).
> >
> > Host userspace should be able to do that with what results?  It would
> > only produce a directly usable value if host endianness is the same as
> > the emulated device's endianness.
> 
> With the result that it gets the value the CPU has sent out on
> the bus as the memory transaction. Obviously if what userspace
> is emulating is a bus which has a byteswapping bridge or if it's
> being helpful to device emulation by providing "here's the value
> even though you think you're wired up backwards" then it needs
> to byteswap.
> 

Yes, because KVM emulates the CPU, it's interface should emulate the
external interface of the CPU, not CPU-internal bits.

The value is directly usable to emulate the bus to attached devices in
user space.

> > I'm not sure that "host kernel native endianness" is an accurate way of
> > describing what currently happens.  Regardless of host or guest
> > endianness, the guest should be swapping the value as necessary to
> > ensure that the value that goes on the (real or emulated) bus is the
> > same.
> 
> I don't know why you're bringing the guest in here. Whether
> the guest chooses to byteswap or not is IMHO not relevant.
> What KVM and userspace need to combine to achieve is that
> whatever the guest happens to have done causes the same result
> it would on the real hardware. Whether the guest sends out a
> write of 0x12345678 because it wrote 0x12345678 directly or
> because it started with 0x87654321 and issued a byte-reverse
> instruction doesn't matter.
> 
> > Plus, if it were really "as it would go on the bus" the value wouldn't
> > necessarily be left justified within data[], depending on how the bus
> > works.
> 
> The point is that the value in data[] is not "as it would go on the bus",
> but the value you get out by treating it as a host-native-endianness
> value of the relevant size left-justified within data[] is the value as
> it would go on the bus.
> 
> > How about a wording like this:
> >
> >   The 'data' member contains, in its first 'len' bytes, the value as it
> >   would appear if the guest had accessed memory rather than I/O.
> 
> I think this is confusing, because now userspace authors have
> to figure out how to get back to "value X of size Y at address Z"
> by interpreting this text... Can you write out the equivalent of
> Christoffer's text "here's how you get the memory transaction
> value" for what you want?
> 
> (Also, value as it would appear to who?)
> 
> I think your wording implies that the order of bytes in data[] depend
> on the guest CPU "usual byte order", ie the order which the CPU
> does not do a byte-lane-swap for (LE for ARM, BE for PPC),
> and it would mean it would come out differently from
> my/Alex/Christoffer's proposal if the host kernel was the opposite
> endianness from that "usual" order.
> 
> Finally, I think it's a bit confusing in that "as if the guest had
> accessed memory" is assigning implicit semantics to memory
> in the emulated system, when memory is actually kind of outside
> KVM's purview because it's not part of the CPU.
> 

The "as if the guest had accessed memory", would imply that user space
needs to look at the settings of the CPU (in the case of ARM the E-bit)
to understand which value the CPU intended for the memory transaction.

However, I'm fine with rewording the patch to something like:

  The 'data' member contains, in its first 'len' bytes, the value of the
  memory transaction in host kernel native endianness, regardless of the
  endianness of the guest.  Host user space should always be able to
  directly do the following to emulate the bus and attached peripherals:
  <type> val = *((<type> *)mmio.data)

if people find this more clear.

-Christoffer