Re: [RFC v3 2/5] KVM: x86: add support for ioregionfd signal handling

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3045 bytes --]

On Sun, Feb 21, 2021 at 03:04:38PM +0300, Elena Afanasova wrote:
> The vCPU thread may receive a signal during ioregionfd communication,
> ioctl(KVM_RUN) needs to return to userspace and then ioctl(KVM_RUN)
> must resume ioregionfd.
> 
> Signed-off-by: Elena Afanasova <eafanasova@gmail.com>

Functionally what this patch does makes sense to me. I'm not familiar
enough with the arch/x86/kvm mmio/pio code to say whether it's possible
to unify mmio/pio/ioregionfd state somehow so that this code can be
simplified.

> +static int complete_ioregion_io(struct kvm_vcpu *vcpu)
> +{
> +	if (vcpu->mmio_needed)
> +		return complete_ioregion_mmio(vcpu);
> +	if (vcpu->arch.pio.count)
> +		return complete_ioregion_pio(vcpu);

Can this be written as:

  if ... {
  } else if ... {
  } else {
      BUG();
  }

?

In other words, I'm not sure if ever get here without either
vcpu->mmio_needed or vcpu->arch.pio.count set.

> +}
> +#endif /* CONFIG_KVM_IOREGION */
> +
>  static void kvm_save_current_fpu(struct fpu *fpu)
>  {
>  	/*
> @@ -9309,6 +9549,12 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu)
>  	else
>  		r = vcpu_run(vcpu);
>  
> +#ifdef CONFIG_KVM_IOREGION
> +	if (vcpu->ioregion_ctx.is_interrupted &&
> +	    vcpu->run->exit_reason == KVM_EXIT_INTR)
> +		r = -EINTR;
> +#endif

Userspace can write to vcpu->run->exit_reason memory so its value cannot
be trusted. It's not obvious to me what happens when is_interrupted ==
true if userspace has corrupted vcpu->run->exit_reason. Since I can't
easily tell if it's safe, I suggest finding another way to perform this
check that does not rely on vcpu->run->exit_reason. Is just checking
vcpu->ioregion_ctx.is_interrupted enough?

(The same applies to other instances of vcpu->run->exit_reason in this
patch.)

> +
>  out:
>  	kvm_put_guest_fpu(vcpu);
>  	if (kvm_run->kvm_valid_regs)
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index f35f0976f5cf..84f07597d131 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -318,6 +318,16 @@ struct kvm_vcpu {
>  #endif
>  	bool preempted;
>  	bool ready;
> +#ifdef CONFIG_KVM_IOREGION
> +	struct {

A comment would be nice to explain the purpose of this struct:

/*
 * MMIO/PIO state kept when a signal interrupts ioregionfd. When
 * ioctl(KVM_RUN) is invoked again we resume from this state.
 */

> +		u64 addr;
> +		void *val;
> +		int pio;

"int pio" could be a boolean indicating whether this is pio or mmio.
Calling it cur_pio or pio_offset might be clearer.

> +		u8 state; /* SEND_CMD/GET_REPLY */
> +		bool in;

/* true - read, false - write */

> +		bool is_interrupted;
> +	} ioregion_ctx;
> +#endif

The ioregion_ctx fields are not set in this patch. Either they are
unused or a later patch will set them. You can help reviewers by noting
this in the commit description. That way they won't need to worry about
whether there are unused fields that were accidentally left in the code.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]