Re: [kvm-ppc-devel] RFC: MMIO endianness flag

[Hollis Blanchard <hollisb-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>]

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
> >> kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
> >> 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