On Sun, Feb 21, 2021 at 03:04:40PM +0300, Elena Afanasova wrote:
> +/* ioregions that share the same rfd are serialized so that only one vCPU
> + * thread sends a struct ioregionfd_cmd to userspace at a time. This
> + * ensures that the struct ioregionfd_resp received from userspace will
> + * be processed by the one and only vCPU thread that sent it.
> + *
> + * A waitqueue is used to wake up waiting vCPU threads in order. Most of
> + * the time the waitqueue is unused and the lock is not contended.
> + * For best performance userspace should set up ioregionfds so that there
> + * is no contention (e.g. dedicated ioregionfds for queue doorbell
> + * registers on multi-queue devices).
> + */
> +struct ioregionfd {
> + wait_queue_head_t wq;
> + struct file *rf;
> + struct kref kref;
> + bool busy;
> +};
> +
> +struct ioregion {
> + struct list_head list;
> + u64 paddr; /* guest physical address */
> + u64 size; /* size in bytes */
> + struct file *wf;
> + u64 user_data; /* opaque token used by userspace */
> + struct kvm_io_device dev;
> + bool posted_writes;
> + struct ioregionfd *ctx;
The name "ctx" is a little confusion since there is also an ioregion_ctx
in struct kvm_vcpu now. Maybe s/struct ioregionfd/struct
ioregion_resp_file/ and s/ctx/resp_file/?
> @@ -90,6 +118,30 @@ ioregion_save_ctx(struct kvm_vcpu *vcpu, bool in, gpa_t addr, u8 state, void *va
> vcpu->ioregion_ctx.in = in;
> }
>
> +static inline void
> +ioregion_lock_ctx(struct ioregionfd *ctx)
> +{
> + if (!ctx)
> + return;
> +
> + spin_lock(&ctx->wq.lock);
> + wait_event_interruptible_exclusive_locked(ctx->wq, !ctx->busy);
What happens if this function call is interrupted by a signal?
> @@ -129,14 +185,15 @@ ioregion_read(struct kvm_vcpu *vcpu, struct kvm_io_device *this, gpa_t addr,
> }
> if (ret != sizeof(buf.cmd)) {
> ret = (ret < 0) ? ret : -EIO;
> - return (ret == -EAGAIN || ret == -EWOULDBLOCK) ? -EINVAL : ret;
> + ret = (ret == -EAGAIN || ret == -EWOULDBLOCK) ? -EINVAL : ret;
> + goto out;
> }
> - if (!p->rf)
> + if (!p->ctx)
> return 0;
I think this should be goto out so that ioregion_unlock_ctx() gets
called.
> @@ -322,6 +420,12 @@ kvm_set_ioregion_idx(struct kvm *kvm, struct kvm_ioregion *args, enum kvm_bus bu
> ret = -EEXIST;
> goto unlock_fail;
> }
> +
> + if (rfile && !ioregion_get_ctx(kvm, p, rfile, bus_idx)) {
> + ret = -ENOMEM;
> + goto unlock_fail;
> + }
> +
> kvm_iodevice_init(&p->dev, &ioregion_ops);
> ret = kvm_io_bus_register_dev(kvm, bus_idx, p->paddr, p->size,
> &p->dev);
> @@ -335,6 +439,8 @@ kvm_set_ioregion_idx(struct kvm *kvm, struct kvm_ioregion *args, enum kvm_bus bu
>
> unlock_fail:
> mutex_unlock(&kvm->slots_lock);
> + if (p->ctx)
> + kref_put(&p->ctx->kref, ctx_free);
Please move the kref_put() before mutex_unlock(&kvm->slots_lock) since
ioregion_get_ctx() depends on kvm->slots_lock to avoid race conditions
with struct ioregionfds that are being created/destroyed.
Attachment:
signature.asc
Description: PGP signature
