Re: [Qemu-devel] [PATCH v5 00/12] KVM Support for MIPS32 Processors

[Aurelien Jarno]

On Wed, Jun 18, 2014 at 05:00:47PM +0200, Paolo Bonzini wrote:
> Il 18/06/2014 00:10, James Hogan ha scritto:
> >The patchset depends on v4 of "target-mips: implement UserLocal
> >Register". I'm aiming for QEMU 2.1, hopefully it isn't too late to get
> >some final review.
> >
> >Thanks to everybody who has already taken part in review.
> >
> >This patchset implements KVM support for MIPS32 processors, using Trap &
> >Emulation.
> >
> >In KVM mode, CPU virtualization is handled via the kvm kernel module,
> >while system and I/O virtualization leverage the Malta model already
> >present in QEMU.
> >
> >Both Guest kernel and Guest Userspace execute in UM. The Guest address
> >space is as folows:
> >Guest User address space:   0x00000000 -> 0x40000000
> >Guest Kernel Unmapped:      0x40000000 -> 0x60000000
> >Guest Kernel Mapped:        0x60000000 -> 0x80000000
> >
> >As a result, Guest Usermode virtual memory is limited to 1GB.
> >
> >KVM support (by trap and emulate) was added to the Linux kernel in
> >v3.10. This patchset partly depends on MIPS KVM work which will land in
> >v3.16 (for example to save/restore the state of various registers and
> >the KVM Count/Compare timer).
> >
> >Changes in v5:
> >
> >Changes addressing review comments from v4 patchset, and to use the MIPS
> >KVM timer API added in v3.16.
> >
> >A git tag for this version of the patchset can also be found on github:
> >https://github.com/jahogan/qemu-kvm-mips.git kvm-mips-v5
> >
> > - Rebase on master + v4 of "target-mips: implement UserLocal Register".
> > - New patch ([01/12] target-mips: Reset CPU timer consistently) to
> >   address timer reset behaviour (reported by Paolo Bonzini).
> > - New patch ([08/12] target-mips: Call kvm_mips_reset_vcpu() from
> >   mips_cpu_reset()) and rename kvm_arch_reset_vcpu to
> >   kvm_mips_reset_vcpu, based on commit 50a2c6e55fa2 (kvm: reset state
> >   from the CPU's reset method).
> > - KSEG0 doesn't actually change size, so fix mask in
> >   cpu_mips_kseg0_to_phys() (patch 3) and use that instead of having the
> >   KVM specific cpu_mips_kvm_um_kseg0_to_phys() (patch 10).
> > - Fix typo in patch 9 subject (s/interupts/interrupts/).
> > - Rename kvm_mips_te_{put,get}_cp0_registers() functions to drop the
> >   "te_" since they're not really specific to T&E.
> > - Pass level through from kvm_arch_put_registers() to
> >   kvm_mips_put_cp0_registers() rather than hard coding it to
> >   KVM_PUT_FULL_STATE.
> > - Fix KVM_REG_MIPS_CP0_* definitions to set KVM_REG_MIPS and
> >   KVM_REG_SIZE_U32/KVM_REG_SIZE_U64 (using a macro).
> > - Remove unused KVM_REG_MIPS_CP0_* definitions for now.
> > - Correct type of kvm_mips_{get,put}_one_{,ul}reg() reg_id argument to
> >   uint64_t. Various high bits must be set to disambiguate the
> >   architecture and register size.
> > - Simplify register access functions slightly.
> > - Add register accessors for always-64-bit registers (rather than ulong
> >   registers). These are needed for virtual KVM registers for
> >   controlling the KVM Compare/Count timer.
> > - Save and restore KVM timer state with the rest of the state, and also
> >   when VM clock is started or stopped. When the KVM timer state is
> >   restored (or VM clock restarted) it is resumed with the stored count
> >   at the monotonic time when the VM clock was last stopped. If the VM
> >   clock hasn't been stopped it resumes from the monotonic time when the
> >   state was saved (i.e. as if the timer was never stopped).
> >   Changes since RFC patch on kernel KVM thread "[PATCH v2 00/23] MIPS:
> >                 KVM: Fixes and guest timer rewrite"):
> >    - Simplified, removing extra state for storing VM time of
> >      save/restore, at the cost of losing/gaining time when VM gets
> >      stopped and started (Paolo Bonzini).
> > - Save and restore the UserLocal and HWREna CP0 registers.
> > - Improve get/put KVM register error handling with DPRINTFs and fall
> >   through so that getting/putting of all the registers is attempted
> >   even if one of them fails due to being unimplemented in the kernel.
> >
> >Changes in v4:
> >
> >Changes mostly addressing a few review comments from v3 patchset.
> >
> >A git tag for this version of the patchset can also be found on github:
> >https://github.com/jahogan/qemu-kvm-mips.git kvm-mips-v4
> >
> > - Rebase on v2.0.0-rc0.
> > - Use int32_t instead of int32 (which is for softfloat) in kvm register
> >   accessors (Andreas Färber).
> > - Use uint64_t instead of __u64 (which is really just for kernel
> >   headers) in the kvm register accessors (Andreas Färber).
> > - Cast pointer to uintptr_t rather than target_ulong in kvm register
> >   accessors.
> > - Remove some redundant casts in kvm register accessors.
> > - Add MAINTAINERS entry for MIPS KVM.
> >
> >Changes in v3:
> >
> >Changes mostly addressing review comments from v2 patchset.
> >
> >A git tag for this version of the patchset can also be found on github:
> >https://github.com/jahogan/qemu-kvm-mips.git kvm-mips-v3
> >
> > - Remove "target-mips: Set target page size to 16K in KVM mode". It
> >   should actually work fine with 4k TARGET_PAGE_SIZE as long as there
> >   is no cache aliasing or both host and guest kernels are configured to
> >   a sufficient page size to avoid aliasing (which the kernel
> >   arch/mips/kvm/00README.txt alludes to anyway).
> > - Rewrote kvm sigmask patch to allow sigmask length to be set by
> >   kvm_arch_init(), so that MIPS can set it to 16 as it has 128 signals.
> >   This is better than cluttering kvm-all.c with TARGET_* ifdefs (Peter
> >   Maydell).
> > - Set sigmask length to 16 from kvm_arch_init() since MIPS Linux has
> >   128 signals. This is better than cluttering kvm_all.c with TARGET_*
> >   ifdefs (Peter Maydell).
> > - s/dprintf/DPRINTF/ (Andreas Färber).
> > - Use "cs" rather than "cpu" or "env" for CPUState variable names
> >   (Andreas Färber).
> > - Use CPUMIPSState rather than CPUArchState (Andreas Färber).
> > - Pass MIPSCPU to cpu_mips_io_interrupts_pending() rather than
> >   CPUMIPSState (Andreas Färber).
> > - Remove spurious parentheses around cpu_mips_io_interrupts_pending()
> >   call (Andreas Färber).
> > - Pass MIPSCPU to kvm_mips_set_[ipi_]interrupt (Andreas Färber).
> > - Make use of error_report (Andreas Färber) and clean up error messages
> >   a little to include __func__.
> > - Remove inline kvm_mips_{put,get}_one_[ul]reg() declarations from
> >   kvm_mips.h. They're only used in target-mips/kvm.c anyway.
> > - Make kvm_arch_{put,get}_registers static within target-mips/kvm.c and
> >   remove from kvm_mips.h.
> > - Remove unnecessary includes from Malta patch, especially linux/kvm.h
> >   which isn't a good idea on non-Linux (Peter Maydell).
> >
> >Changes in v2:
> >
> >This patchset is based on Sanjay Lal's V1 patchset from 2nd March 2013:
> >https://patchwork.kernel.org/project/kvm/list/?submitter=51991&state=*&q=qemu-devel
> >
> >I think I've addressed all the V1 feedback. The other main change is the
> >removal of the boot-CPS ROM code binary blob and GIC/SMP support since
> >it's all slightly orthogonal to KVM support. Instead the existing
> >minimal bootloader code for Malta has been updated to work with KVM T&E.
> >
> >A git tag for this version of the patchset can also be found on github:
> >https://github.com/jahogan/qemu-kvm-mips.git kvm-mips-v2
> >
> > - Expand commit messages
> > - Rebase on v1.7.0
> > - Misc checkpatch and other cleanups
> > - Some interrupt bug fixes from Yann Le Du <address@hidden>
> > - Add get/set register functionality from Yann Le Du <address@hidden>
> > - Use new 64 bit compatible ABI from Cavium from Sanjay Lal
> >   <address@hidden>
> > - Add dummy kvm_arch_init_irq_routing()
> >   The common KVM code insists on calling kvm_arch_init_irq_routing() as
> >   soon as it sees kernel header support for it (regardless of whether
> >   QEMU supports it). Provide a dummy function to satisfy this.
> > - Remove request_interrupt_window code (Peter Maydell)
> > - Remove #ifdef CONFIG_KVM where guarded by kvm_enabled() already
> > - Removal of cps / GIC / SMP support
> > - Minimal bootloader modified to execute safely from RAM
> > - Create asm-mips symlink using generic code and move above default
> >   case (Peter Maydell)
> > - Remove redundant check of target_name = cpu = mips
> > - Remove mipsel cross compilation fix, which is now fixed by commit
> >   61cc919f73ea (configure: detect endian via compile test)
> > - Add translation of guest kernel segments to allow an attached gdb to
> >   see kernel memory correctly
> >
> >James Hogan (7):
> >  target-mips: Reset CPU timer consistently
> >  target-mips: get_physical_address: Add defines for segment bases
> >  target-mips: get_physical_address: Add KVM awareness
> >  kvm: Allow arch to set sigmask length
> >  target-mips: Call kvm_mips_reset_vcpu() from mips_cpu_reset()
> >  hw/mips: malta: Add KVM support
> >  MAINTAINERS: Add entry for MIPS KVM
> >
> >Sanjay Lal (5):
> >  hw/mips/cputimer: Don't start periodic timer in KVM mode
> >  hw/mips: Add API to convert KVM guest KSEG0 <-> GPA
> >  target-mips: kvm: Add main KVM support for MIPS
> >  hw/mips: In KVM mode, inject IRQ2 (I/O) interrupts via ioctls
> >  target-mips: Enable KVM support in build system
> >
> > MAINTAINERS               |   5 +
> > configure                 |   6 +-
> > hw/mips/addr.c            |   7 +-
> > hw/mips/cputimer.c        |  18 +-
> > hw/mips/mips_int.c        |  11 +
> > hw/mips/mips_malta.c      |  73 +++--
> > include/hw/mips/cpudevs.h |   2 +
> > include/sysemu/kvm.h      |   2 +
> > kvm-all.c                 |  11 +-
> > target-mips/Makefile.objs |   1 +
> > target-mips/cpu.c         |   8 +
> > target-mips/helper.c      |  51 +++-
> > target-mips/kvm.c         | 683 
> > ++++++++++++++++++++++++++++++++++++++++++++++
> > target-mips/kvm_mips.h    |  26 ++
> > target-mips/translate.c   |   2 +
> > 15 files changed, 866 insertions(+), 40 deletions(-)
> > create mode 100644 target-mips/kvm.c
> > create mode 100644 target-mips/kvm_mips.h
> >
> 
> Thanks, it's a very clean patch set.  I'll leave a few days for
> Aurelien to comment, and then apply to uq/master.

Except the minor comment on patch 10 it looks fine to me. That said I
haven't reviewed the KVM interface part very deeply, I trust you for
that part.

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
address@hidden                 http://www.aurel32.net