Re: [PATCH v8 11/15] ARM: KVM: World-switch implementation

[Avi Kivity <avi@redhat.com>]

On 06/15/2012 10:08 PM, Christoffer Dall wrote:
> Provides complete world-switch implementation to switch to other guests
> running in non-secure modes. Includes Hyp exception handlers that
> capture necessary exception information and stores the information on
> the VCPU and KVM structures.
> 
> Switching to Hyp mode is done through a simple HVC instructions. The
> exception vector code will check that the HVC comes from VMID==0 and if
> so will store the necessary state on the Hyp stack, which will look like
> this (growing downwards, see the hyp_hvc handler):
>   ...
>   Hyp_Sp + 4: spsr (Host-SVC cpsr)
>   Hyp_Sp    : lr_usr
> 
> When returning from Hyp mode to SVC mode, another HVC instruction is
> executed from Hyp mode, which is taken in the Hyp_Svc handler. The Hyp
> stack pointer should be where it was left from the above initial call,
> since the values on the stack will be used to restore state (see
> hyp_svc).
> 
> Otherwise, the world-switch is pretty straight-forward. All state that
> can be modified by the guest is first backed up on the Hyp stack and the
> VCPU values is loaded onto the hardware. State, which is not loaded, but
> theoretically modifiable by the guest is protected through the
> virtualiation features to generate a trap and cause software emulation.
> Upon guest returns, all state is restored from hardware onto the VCPU
> struct and the original state is restored from the Hyp-stack onto the
> hardware.
> 
> One controversy may be the back-door call to __irq_svc (the host
> kernel's own physical IRQ handler) which is called when a physical IRQ
> exception is taken in Hyp mode while running in the guest.
> 
> SMP support using the VMPIDR calculated on the basis of the host MPIDR
> and overriding the low bits with KVM vcpu_id contributed by Marc Zyngier.
> 
> Reuse of VMIDs has been implemented by Antonios Motakis and adapated from
> a separate patch into the appropriate patches introducing the
> functionality. Note that the VMIDs are stored per VM as required by the ARM
> architecture reference manual.
> 
> +
> +/**
> + * update_vttbr - Update the VTTBR with a valid VMID before the guest runs
> + * @kvm	The guest that we are about to run
> + *
> + * Called from kvm_arch_vcpu_ioctl_run before entering the guest to ensure the
> + * VM has a valid VMID, otherwise assigns a new one and flushes corresponding
> + * caches and TLBs.
> + */
> +static void update_vttbr(struct kvm *kvm)
> +{
> +	phys_addr_t pgd_phys;
> +
> +	spin_lock(&kvm_vmid_lock);

Premature, but this is sad.  I suggest you split vmid generation from
next available vmid.  This allows you to make the generation counter
atomic so it may be read outside the lock.

You can do

    if (likely(kvm->arch.vmd_gen) == atomic_read(&kvm_vmid_gen)))
           return;

    spin_lock(...

> +
> +	/*
> +	 *  Check that the VMID is still valid.
> +	 *  (The hardware supports only 256 values with the value zero
> +	 *   reserved for the host, so we check if an assigned value has rolled
> +	 *   over a sequence, which requires us to assign a new value and flush
> +	 *   necessary caches and TLBs on all CPUs.)
> +	 */
> +	if (unlikely((kvm->arch.vmid ^ next_vmid) >> VMID_BITS)) {
> +		/* Check for a new VMID generation */
> +		if (unlikely((next_vmid & VMID_MASK) == 0)) {
> +			/* Check for the (very unlikely) 64-bit wrap around */

Unlikely?  it's impossible.

> +
> +/**
> + * kvm_arch_vcpu_ioctl_run - the main VCPU run function to execute guest code
> + * @vcpu:	The VCPU pointer
> + * @run:	The kvm_run structure pointer used for userspace state exchange
> + *
> + * This function is called through the VCPU_RUN ioctl called from user space. It
> + * will execute VM code in a loop until the time slice for the process is used
> + * or some emulation is needed from user space in which case the function will
> + * return with return value 0 and with the kvm_run structure filled in with the
> + * required data for the requested emulation.
> + */
>  int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  {
> -	return -EINVAL;
> +	int ret = 0;
> +	sigset_t sigsaved;
> +
> +	if (vcpu->sigset_active)
> +		sigprocmask(SIG_SETMASK, &vcpu->sigset, &sigsaved);
> +
> +	run->exit_reason = KVM_EXIT_UNKNOWN;
> +	while (run->exit_reason == KVM_EXIT_UNKNOWN) {

It's not a good idea to read stuff from run unless it's part of the ABI,
since userspace can play with it while you're reading it.  It's harmless
here but in other places it can lead to a vulnerability.

> +		/*
> +		 * Check conditions before entering the guest
> +		 */
> +		if (need_resched())
> +			kvm_resched(vcpu);

I think cond_resched() is all that's needed these days (kvm_resched()
predates preempt notifiers).

> +
> +		if (signal_pending(current)) {
> +			ret = -EINTR;
> +			run->exit_reason = KVM_EXIT_INTR;
> +			break;
> +		}
> +
> +		/*
> +		 * Enter the guest
> +		 */
> +		trace_kvm_entry(vcpu->arch.regs.pc);
> +
> +		update_vttbr(vcpu->kvm);
> +
> +		local_irq_disable();
> +		kvm_guest_enter();
> +		vcpu->mode = IN_GUEST_MODE;
> +
> +		ret = __kvm_vcpu_run(vcpu);
> +
> +		vcpu->mode = OUTSIDE_GUEST_MODE;
> +		kvm_guest_exit();
> +		local_irq_enable();
> +
> +		trace_kvm_exit(vcpu->arch.regs.pc);
> +	}
> +
> +	if (vcpu->sigset_active)
> +		sigprocmask(SIG_SETMASK, &sigsaved, NULL);
> +
> +	return ret;
>  }
>  

-- 
error compiling committee.c: too many arguments to function