[PATCH] KVM: PPC: Book3S HV: return htab entries in big endian

[Cédric Le Goater]

Saving and restoring guests on a KVM little endian host is currently=20
broken because qemu assumes that htabs are big endian.=20

This patch modifies kvm_htab_read and kvm_htab_write to make sure=20
that the endianness expected by qemu is enforced on big and little
endian hosts.

Signed-off-by: C=C3=A9dric Le Goater <***@fr.ibm.com>
---

Tested on 3.17-rc7 with LE and BE host.

Looking at the code, it is not very clear what we are doing
in terms of endianness. May be this needs more work on both=20
side, KVM and qemu, to remove the big endian assumptions ?=20

Thanks,

arch/powerpc/kvm/book3s_64_mmu_hv.c | 5 +++++
1 file changed, 5 insertions(+)

diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c b/arch/powerpc/kvm/boo=
k3s_64_mmu_hv.c
index 79294c4c5015..51dbf772158b 100644
--- a/arch/powerpc/kvm/book3s_64_mmu_hv.c
+++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c
@@ -1463,6 +1463,9 @@ static ssize_t kvm_htab_read(struct file *file, c=
har __user *buf,
}
=20
if (hdr.n_valid || hdr.n_invalid) {
+ hdr.index =3D cpu_to_be32(hdr.index);
+ hdr.n_valid =3D cpu_to_be16(hdr.n_valid);
+ hdr.n_invalid =3D cpu_to_be16(hdr.n_invalid);
/* write back the header */
if (__copy_to_user(hptr, &hdr, sizeof(hdr)))
return -EFAULT;
@@ -1542,6 +1545,8 @@ static ssize_t kvm_htab_write(struct file *file, =
const char __user *buf,
err =3D -EFAULT;
if (__get_user(v, lbuf) || __get_user(r, lbuf + 1))
goto out;
+ v =3D be64_to_cpu(v);
+ r =3D be64_to_cpu(r);
err =3D -EINVAL;
if (!(v & HPTE_V_VALID))
goto out;
--=20
1.7.10.4

[Alexander Graf]

Post by Cédric Le Goater
Saving and restoring guests on a KVM little endian host is currently=20
broken because qemu assumes that htabs are big endian.=20
=20
This patch modifies kvm_htab_read and kvm_htab_write to make sure=20
that the endianness expected by qemu is enforced on big and little
endian hosts.
=20
---
=20
Tested on 3.17-rc7 with LE and BE host.
=20
Looking at the code, it is not very clear what we are doing
in terms of endianness. May be this needs more work on both=20
side, KVM and qemu, to remove the big endian assumptions ?=20
=20
Thanks,
=20
arch/powerpc/kvm/book3s_64_mmu_hv.c | 5 +++++
1 file changed, 5 insertions(+)
=20
diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c b/arch/powerpc/kvm/b=

ook3s_64_mmu_hv.c

Post by Cédric Le Goater
index 79294c4c5015..51dbf772158b 100644
--- a/arch/powerpc/kvm/book3s_64_mmu_hv.c
+++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c
@@ -1463,6 +1463,9 @@ static ssize_t kvm_htab_read(struct file *file,=

char __user *buf,

Post by Cédric Le Goater
}
=20
if (hdr.n_valid || hdr.n_invalid) {
+ hdr.index =3D cpu_to_be32(hdr.index);
+ hdr.n_valid =3D cpu_to_be16(hdr.n_valid);
+ hdr.n_invalid =3D cpu_to_be16(hdr.n_invalid);

This breaks strict endianness checking. cpu_to_be16 returns a be16 and
takes a u16 as input. Same for the 32bit version.

I think we're best off to keep the user space API native endian. So
really we should only ever have to convert from big to native endian on
read and native to big on write.

With that QEMU should do the "right thing" already, no?


Alex

Post by Cédric Le Goater
/* write back the header */
if (__copy_to_user(hptr, &hdr, sizeof(hdr)))
return -EFAULT;
@@ -1542,6 +1545,8 @@ static ssize_t kvm_htab_write(struct file *file=

, const char __user *buf,

Post by Cédric Le Goater
err =3D -EFAULT;
if (__get_user(v, lbuf) || __get_user(r, lbuf + 1))
goto out;
+ v =3D be64_to_cpu(v);
+ r =3D be64_to_cpu(r);
err =3D -EINVAL;
if (!(v & HPTE_V_VALID))
goto out;
=20

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Paul Mackerras]

Post by Alexander Graf
I think we're best off to keep the user space API native endian. So
really we should only ever have to convert from big to native endian on
read and native to big on write.
With that QEMU should do the "right thing" already, no?

I believe that when migrating a guest, QEMU just passes the byte
stream it gets from the htab fd along to the destination QEMU with
little or no interpretation, and the destination QEMU writes the byte
stream to the htab fd for the receiving VM. Alexey would be able to
say for sure.

If that's the case, then perhaps it's best to define that byte stream
to contain values of one specific endianness, and for compatibility
that would be big endian.

I assume we would want to be able to migrate a guest from a BE host to
an LE host or vice versa. If not then the question is moot.

Paul.

[Alexander Graf]

Post by Paul Mackerras

Post by Alexander Graf
I think we're best off to keep the user space API native endian. So
really we should only ever have to convert from big to native endian on
read and native to big on write.
With that QEMU should do the "right thing" already, no?

I believe that when migrating a guest, QEMU just passes the byte
stream it gets from the htab fd along to the destination QEMU with
little or no interpretation, and the destination QEMU writes the byte
stream to the htab fd for the receiving VM. Alexey would be able to
say for sure.
If that's the case, then perhaps it's best to define that byte stream
to contain values of one specific endianness, and for compatibility
that would be big endian.
I assume we would want to be able to migrate a guest from a BE host to
an LE host or vice versa. If not then the question is moot.

Yes, the migration stream needs to stay host endian agnostic, so it should be BE. Whether QEMU or kvm does the conversion is an implementation detail I don't mind much for either way. But keep in mind that qemu also uses the htab fd for htab introspection that it does for gdbstub emulation or the x monitor command.


Alex

[Alexey Kardashevskiy]

Post by Alexander Graf

Post by Paul Mackerras

Post by Alexander Graf
I think we're best off to keep the user space API native endian.
So really we should only ever have to convert from big to native
endian on read and native to big on write.
With that QEMU should do the "right thing" already, no?

I believe that when migrating a guest, QEMU just passes the byte
stream it gets from the htab fd along to the destination QEMU with
little or no interpretation, and the destination QEMU writes the
byte stream to the htab fd for the receiving VM. Alexey would be
able to say for sure.
If that's the case, then perhaps it's best to define that byte
stream to contain values of one specific endianness, and for
compatibility that would be big endian.
I assume we would want to be able to migrate a guest from a BE host
to an LE host or vice versa. If not then the question is moot.

Yes, the migration stream needs to stay host endian agnostic, so it
should be BE. Whether QEMU or kvm does the conversion is an
implementation detail I don't mind much for either way.

I find it lot easier if every binary interface has defined endianness,
using native endian just creates problems.

Post by Alexander Graf
But keep in mind that qemu also uses the htab fd for htab
introspection that it does for gdbstub emulation or the x monitor
command.

- --
Alexey