I hope I've explained in the other mail I just sent why Qemu assuming
little-endian for everything is not OK.
One other important clarification: kvm_run->mmio.is_bigendian is about
*one* *particular* *MMIO* *access*. It has only coincidental
relationship to the "endianness mode" of the guest.
--
Hollis Blanchard
IBM Linux Technology Center
On Mon, 2008-01-14 at 13:42 +0800, Xu, Anthony wrote:
> > kvm_run->mmio.is_bigendian = vcpu->arch.some_register &
> From your example code, I can know is_bigendian indicate whether guest
> is in bigendian mode when accessing MMIO.
>
> Qemu is responsible for handling MMIO emulation, Qemus always assume it
> is running on littleendian mode.
> So if guest accesses MMIO in bigendian mode,
> kvm need to transform it to littleendian before delivering this
> MMIO request to Qemu.
>
>
> This works for IA64 side.
>
>
> Thanks,
> - Anthony
>
>
> Hollis Blanchard wrote:
> > We're just talking about a flag in the kvm_run.mmio structure, so it
> > does not represent the state of any software, guest or host, other
> > than that single MMIO access.
> >
> > This flag is only used for communication between host kernel and host
> > userland, so the host kernel is always responsible for setting it.
> >
> > "is_bigendian" is just one more necessary piece of information for
> > MMIO emulation. kvm_run already tells you that you are loading 4
> > bytes from address 0. What you don't know today is if byte 0 is the
> > least significant or most significant byte. If "is_bigendian" is set,
> > you know that byte 0 is the MSB and byte 3 is the LSB. If not, the
> > opposite is true.
> >
> > In the simplest case, IA64's in-kernel MMIO emulation code could look
> > something like:
> > kvm_run->mmio.phys_addr = addr;
> > kvm_run->mmio.len = len;
> > ...
> > kvm_run->mmio.is_bigendian = vcpu->arch.some_register &
> > BIGENDIAN;
> >
> > If IA64 has reverse-endian load/store instructions like PowerPC, then
> > you would also need to consider the particular instruction used as
> > well as the guest state.
> >
> >
> > On Fri, 2008-01-11 at 10:02 +0800, Xu, Anthony wrote:
> >> Hi all,
> >>
> >> That's a good start to consider BE.
> >> Yes, IA64 support BE and LE.
> >>
> >> I have below comments.
> >>
> >> What does is_bigendian mean?
> >> Host is runing with BE or guest is running with BE.
> >> Who will set is_bigendian?
> >>
> >> For supporing BE,
> >> We need to consider host BE and guest BE.
> >> For IA64, most OS is running with LE, and
> >> Application can run with BE or LE,
> >> For example, Qemu can run with BE or LE.
> >>
> >> IMHO, we need two flags,
> >> host_is_bigendian indicates Qemu is running with BE
> >> Guest_is_bigendian indecates the guest application who is accessing
> >> MMIO
> >>
> >> Is running with LE.
> >>
> >>
> >> - Anthony
> >>
> >>
> >>
> >>
> >>
> >>
> >> Hollis Blanchard wrote:
> >>> On Thu, 2008-01-10 at 17:28 +0200, Avi Kivity wrote:
> >>>> I'll apply that patch (with a #ifdef CONFIG_PPC so other archs
> >>>> don't use it by mistake).
> >>>
> >>> I don't think that's the right ifdef. For example, I believe IA64
> >>> can run in BE mode and so will have the same issue, and there are
> >>> certainly other architectures (less relevant to the current code)
> >>> that definitely are in the same situation.
> >>>
> >>> We need to plumb this through to the libkvm users anyways. Take a
> >>> look at the patch below and tell me if you think it's not the right
> >>> approach. x86 simply won't consider 'is_bigendian'. I spent a lot
> >>> of time on this, and it's by far the cleanest solution I could come
> >>> up with.
> >>>
> >>>
> >>>
> >>> diff --git a/libkvm/config-i386.mak b/libkvm/config-i386.mak ---
> >>> a/libkvm/config-i386.mak +++ b/libkvm/config-i386.mak
> >>> @@ -2,5 +2,6 @@ LIBDIR := /lib
> >>> LIBDIR := /lib
> >>> CFLAGS += -m32
> >>> CFLAGS += -D__i386__
> >>> +CFLAGS += -DARCH_MMIO_ENDIAN_LITTLE
> >>>
> >>> libkvm-$(ARCH)-objs := libkvm-x86.o
> >>> diff --git a/libkvm/config-ia64.mak b/libkvm/config-ia64.mak ---
> >>> a/libkvm/config-ia64.mak +++ b/libkvm/config-ia64.mak
> >>> @@ -1,5 +1,6 @@
> >>>
> >>> LIBDIR := /lib
> >>> CFLAGS += -D__ia64__
> >>> +CFLAGS += -DARCH_MMIO_ENDIAN_LITTLE -DARCH_MMIO_ENDIAN_BIG
> >>>
> >>> libkvm-$(ARCH)-objs := libkvm-ia64.o
> >>> diff --git a/libkvm/config-powerpc.mak b/libkvm/config-powerpc.mak
> >>> --- a/libkvm/config-powerpc.mak
> >>> +++ b/libkvm/config-powerpc.mak
> >>> @@ -2,5 +2,6 @@ LIBDIR := /lib
> >>> LIBDIR := /lib
> >>> CFLAGS += -m32
> >>> CFLAGS += -D__powerpc__
> >>> +CFLAGS += -DARCH_MMIO_ENDIAN_BIG -DARCH_MMIO_ENDIAN_LITTLE
> >>>
> >>> libkvm-$(ARCH)-objs := libkvm-powerpc.o
> >>> diff --git a/libkvm/config-x86_64.mak b/libkvm/config-x86_64.mak
> >>> --- a/libkvm/config-x86_64.mak
> >>> +++ b/libkvm/config-x86_64.mak
> >>> @@ -2,5 +2,6 @@ LIBDIR := /lib64
> >>> LIBDIR := /lib64
> >>> CFLAGS += -m64
> >>> CFLAGS += -D__x86_64__
> >>> +CFLAGS += -DARCH_MMIO_ENDIAN_LITTLE
> >>>
> >>> libkvm-$(ARCH)-objs := libkvm-x86.o
> >>> diff --git a/libkvm/libkvm.c b/libkvm/libkvm.c
> >>> --- a/libkvm/libkvm.c
> >>> +++ b/libkvm/libkvm.c
> >>> @@ -774,21 +774,56 @@ int kvm_set_sregs(kvm_context_t kvm, int
> >>> return ioctl(kvm->vcpu_fd[vcpu], KVM_SET_SREGS, sregs); }
> >>>
> >>> +#ifdef ARCH_MMIO_ENDIAN_BIG
> >>> +static int handle_mmio_bigendian(kvm_context_t kvm, struct kvm_run
> >>> *kvm_run) +{ + if (kvm_run->mmio.is_write)
> >>> + return kvm->callbacks->mmio_write_be(kvm->opaque,
> >>> +
> >> kvm_run->mmio.phys_addr,
> >>> + kvm_run->mmio.data,
> >>> + kvm_run->mmio.len);
> >>> + else
> >>> + return kvm->callbacks->mmio_read_be(kvm->opaque,
> >>> +
> >> kvm_run->mmio.phys_addr,
> >>> + kvm_run->mmio.data,
> >>> + kvm_run->mmio.len);
> >>> +}
> >>> +#endif
> >>> +
> >>> +#ifdef ARCH_MMIO_ENDIAN_LITTLE
> >>> +static int handle_mmio_littleendian(kvm_context_t kvm, struct
> >>> kvm_run *kvm_run) +{ + if (kvm_run->mmio.is_write)
> >>> + return kvm->callbacks->mmio_write_le(kvm->opaque,
> >>> +
> >> kvm_run->mmio.phys_addr,
> >>> + kvm_run->mmio.data,
> >>> + kvm_run->mmio.len);
> >>> + else
> >>> + return kvm->callbacks->mmio_read_le(kvm->opaque,
> >>> +
> >> kvm_run->mmio.phys_addr,
> >>> + kvm_run->mmio.data,
> >>> + kvm_run->mmio.len);
> >>> +}
> >>> +#endif
> >>> +
> >>> static int handle_mmio(kvm_context_t kvm, struct kvm_run *kvm_run)
> >>> { unsigned long addr = kvm_run->mmio.phys_addr;
> >>> - void *data = kvm_run->mmio.data;
> >>>
> >>> /* hack: Red Hat 7.1 generates these weird accesses. */
> >>> if ((addr > 0xa0000-4 && addr <= 0xa0000) && kvm_run->mmio.len
> ==
> >>> 3) return 0;
> >>>
> >>> - if (kvm_run->mmio.is_write)
> >>> - return kvm->callbacks->mmio_write(kvm->opaque, addr,
> data,
> >>> - kvm_run->mmio.len);
> >>> +#if defined(ARCH_MMIO_ENDIAN_BIG) &&
> >>> defined(ARCH_MMIO_ENDIAN_LITTLE) + if (kvm_run->mmio.is_bigendian)
> >>> + return handle_mmio_bigendian(kvm, kvm_run);
> >>> else
> >>> - return kvm->callbacks->mmio_read(kvm->opaque, addr,
> data,
> >>> - kvm_run->mmio.len);
> >>> + return handle_mmio_littleendian(kvm, kvm_run);
> >>> +#elif ARCH_MMIO_ENDIAN_LITTLE
> >>> + return handle_mmio_littleendian(kvm, kvm_run);
> >>> +#elif ARCH_MMIO_ENDIAN_BIG
> >>> + return handle_mmio_bigendian(kvm, kvm_run);
> >>> +#endif
> >>> }
> >>>
> >>> int handle_io_window(kvm_context_t kvm)
> >>> diff --git a/libkvm/libkvm.h b/libkvm/libkvm.h
> >>> --- a/libkvm/libkvm.h
> >>> +++ b/libkvm/libkvm.h
> >>> @@ -46,11 +46,11 @@ struct kvm_callbacks {
> >>> /// For 32bit IO writes from the guest (Usually when executing
> >>> 'outl') int (*outl)(void *opaque, uint16_t addr, uint32_t
> >>> data); /// generic memory reads to unmapped memory (For MMIO
> >>> devices)
> >>> - int (*mmio_read)(void *opaque, uint64_t addr, uint8_t *data,
> >>> - int len);
> >>> + int (*mmio_read_be)(void *opaque, uint64_t addr, uint8_t *data,
> >>> int len); + int (*mmio_read_le)(void *opaque, uint64_t addr,
> >>> uint8_t *data, int len); /// generic memory writes to unmapped
> >>> memory (For MMIO devices)
> >>> - int (*mmio_write)(void *opaque, uint64_t addr, uint8_t *data,
> >>> - int len);
> >>> + int (*mmio_write_be)(void *opaque, uint64_t addr, uint8_t
> >>> *data, int len); + int (*mmio_write_le)(void *opaque, uint64_t
> >>> addr, uint8_t *data, int len); int (*debug)(void *opaque, int
> >>> vcpu); /*! * \brief Called when the VCPU issues an
> 'hlt'
> >>> instruction.
> >>> diff --git a/qemu/qemu-kvm.c b/qemu/qemu-kvm.c
> >>> --- a/qemu/qemu-kvm.c
> >>> +++ b/qemu/qemu-kvm.c
> >>> @@ -21,6 +21,7 @@ int kvm_irqchip = 1;
> >>> #include <libkvm.h>
> >>> #include <pthread.h>
> >>> #include <sys/utsname.h>
> >>> +#include <endian.h>
> >>>
> >>> extern void perror(const char *s);
> >>>
> >>> @@ -533,8 +534,13 @@ static struct kvm_callbacks qemu_kvm_ops
> >>> .outb = kvm_outb, .outw = kvm_outw,
> >>> .outl = kvm_outl,
> >>> - .mmio_read = kvm_mmio_read,
> >>> - .mmio_write = kvm_mmio_write,
> >>> +#if __BYTE_ORDER == __LITTLE_ENDIAN
> >>> + .mmio_read_le = kvm_mmio_read,
> >>> + .mmio_write_le = kvm_mmio_write,
> >>> +#else
> >>> + .mmio_read_be = kvm_mmio_read,
> >>> + .mmio_write_be = kvm_mmio_write,
> >>> +#endif
> >>> .halt = kvm_halt,
> >>> .shutdown = kvm_shutdown,
> >>> .io_window = kvm_io_window,
> >>> diff --git a/user/main-ppc.c b/user/main-ppc.c
> >>> --- a/user/main-ppc.c
> >>> +++ b/user/main-ppc.c
> >>> @@ -92,14 +92,14 @@ static int test_pre_kvm_run(void *opaque
> >>> return 0; }
> >>>
> >>> -static int test_mem_read(void *opaque, uint64_t addr, uint8_t
> >>> *data, int len) +static int test_mem_read_be(void *opaque, uint64_t
> >>> addr, uint8_t *data, int len) { printf("%s: addr %"PRIx64" len
> >>> %d\n", __func__, addr, len); memset(data, 0, len); return 0;
> >>> }
> >>>
> >>> -static int test_mem_write(void *opaque, uint64_t addr, uint8_t
> >>> *data, int len) +static int test_mem_write_be(void *opaque, uint64_t
> >>> addr, uint8_t *data, int len) {
> >>> printf("%s: addr %"PRIx64" len %d data %"PRIx64"\n",
> >>> __func__, addr, len, *(uint64_t *)data);
> >>> @@ -120,8 +120,8 @@ static int test_dcr_write(kvm_context_t }
> >>>
> >>> static struct kvm_callbacks test_callbacks = {
> >>> - .mmio_read = test_mem_read,
> >>> - .mmio_write = test_mem_write,
> >>> + .mmio_read_be = test_mem_read_be,
> >>> + .mmio_write_be = test_mem_write_be,
> >>> .debug = test_debug,
> >>> .halt = test_halt,
> >>> .io_window = test_io_window,
> >>> diff --git a/user/main.c b/user/main.c
> >>> --- a/user/main.c
> >>> +++ b/user/main.c
> >>> @@ -389,8 +389,8 @@ static struct kvm_callbacks test_callbac
> >>> .outb = test_outb, .outw = test_outw,
> >>> .outl = test_outl,
> >>> - .mmio_read = test_mem_read,
> >>> - .mmio_write = test_mem_write,
> >>> + .mmio_read_le = test_mem_read,
> >>> + .mmio_write_le = test_mem_write,
> >>> .debug = test_debug,
> >>> .halt = test_halt,
> >>> .io_window = test_io_window,
> >>
> >>
> ------------------------------------------------------------------------
> -
> >> Check out the new SourceForge.net Marketplace.
> >> It's the best place to buy or sell services for
> >> just about anything Open Source.
> >>
> http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketp
> lace
> >> _______________________________________________
> >> kvm-devel mailing list
> >> [email protected]
> >> https://lists.sourceforge.net/lists/listinfo/kvm-devel
-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace
_______________________________________________
kvm-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/kvm-devel
