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

Aurelien Jarno
2014-06-19T16:29:04Z

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 <ledu@kymasys.com>
> > - Add get/set register functionality from Yann Le Du <ledu@kymasys.com>
> > - Use new 64 bit compatible ABI from Cavium from Sanjay Lal
> > <sanjayl@kymasys.com>
> > - 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
aurelien@aurel32.net http://www.aurel32.net