All of lore.kernel.org
 help / color / mirror / Atom feed
From: Sean Christopherson <seanjc@google.com>
To: Thanos Makatos <thanos.makatos@nutanix.com>
Cc: "pbonzini@redhat.com" <pbonzini@redhat.com>,
	John Levon <john.levon@nutanix.com>,
	 "kvm@vger.kernel.org" <kvm@vger.kernel.org>
Subject: Re: [PATCH] KVM: optionally post write on ioeventfd write
Date: Tue, 22 Sep 2026 10:03:32 -0700	[thread overview]
Message-ID: <arK05GrLtiBvbfmm@google.com> (raw)
In-Reply-To: <20260306125651.2485-1-thanos.makatos@nutanix.com>

On Fri, Mar 06, 2026, Thanos Makatos wrote:
> @@ -656,7 +658,8 @@ struct kvm_ioeventfd {
>  	__u32 len;         /* 1, 2, 4, or 8 bytes; or 0 to ignore length */
>  	__s32 fd;
>  	__u32 flags;
> -	__u8  pad[36];
> +	__aligned_u64 post_addr; /* address to write to if POST_WRITE is set */
> +	__u8  pad[24];
>  };

I would prefer to have explicit padding:

  struct kvm_ioeventfd {
	__u64 datamatch;
	__u64 addr;        /* legal pio/mmio address */
	__u32 len;         /* 1, 2, 4, or 8 bytes; or 0 to ignore length */
	__s32 fd;
	__u32 flags;
	__u32 pad0;
	__u64 post_addr;   /* address to write to if POST_WRITE is set */
	__u8  pad[24];
  };

that way it's more obvious there's a hole we can use in the future.


> diff --git a/tools/testing/selftests/kvm/Makefile.kvm b/tools/testing/selftests/kvm/Makefile.kvm
> index fdec90e85467..7ab470981c31 100644
> --- a/tools/testing/selftests/kvm/Makefile.kvm
> +++ b/tools/testing/selftests/kvm/Makefile.kvm
> @@ -64,6 +64,7 @@ TEST_GEN_PROGS_COMMON += kvm_binary_stats_test
>  TEST_GEN_PROGS_COMMON += kvm_create_max_vcpus
>  TEST_GEN_PROGS_COMMON += kvm_page_table_test
>  TEST_GEN_PROGS_COMMON += set_memory_region_test
> +TEST_GEN_PROGS_COMMON += ioeventfd_test

Please name this ioeventfd_posted_write_test, as it isn't a generic ioeventfd
test.  And maintain the alphabetical sort.

Does this actually work on s390?  Which doesn't have MMIO...

> diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
> index 0e8b8a2c5b79..22bc49a41503 100644
> --- a/virt/kvm/eventfd.c
> +++ b/virt/kvm/eventfd.c
> @@ -741,6 +741,7 @@ struct _ioeventfd {
>  	struct kvm_io_device dev;
>  	u8                   bus_idx;
>  	bool                 wildcard;
> +	void         __user *post_addr;
>  };
>  
>  static inline struct _ioeventfd *
> @@ -812,6 +813,9 @@ ioeventfd_write(struct kvm_vcpu *vcpu, struct kvm_io_device *this, gpa_t addr,
>  	if (!ioeventfd_in_range(p, addr, len, val))
>  		return -EOPNOTSUPP;
>  
> +	if (p->post_addr && len > 0 && __copy_to_user(p->post_addr, val, len))
> +		return -EFAULT;
> +
>  	eventfd_signal(p->eventfd);
>  	return 0;
>  }
> @@ -866,6 +870,7 @@ static int kvm_assign_ioeventfd_idx(struct kvm *kvm,
>  {
>  
>  	struct eventfd_ctx *eventfd;
> +	void __user *post_addr;
>  	struct _ioeventfd *p;
>  	int ret;
>  
> @@ -873,6 +878,16 @@ static int kvm_assign_ioeventfd_idx(struct kvm *kvm,
>  	if (IS_ERR(eventfd))
>  		return PTR_ERR(eventfd);
>  
> +	post_addr = u64_to_user_ptr(args->post_addr);
> +	if ((args->flags & KVM_IOEVENTFD_FLAG_POST_WRITE) &&
> +	    (!args->len || !post_addr ||
> +	     args->post_addr != untagged_addr(args->post_addr) ||
> +	     !access_ok(post_addr, args->len))) {
> +		/* In KVM’s ABI, post_addr must be non‑NULL. */

Maybe elaborate just a bit?  And I vote to drop the local "post_addr", even though
it's more "work", because it's super easy to overlook that KVM *must* do the cast
before checking for a NULL pointer.

E.g.

	/*
	 * Disallow posting to virtual address 0 so that KVM can treat
	 * a NULL user pointer as "no posted write".
	 */
	if ((args->flags & KVM_IOEVENTFD_FLAG_POST_WRITE) &&
	    (!args->len || !u64_to_user_ptr(args->post_addr) ||
	     args->post_addr != untagged_addr(args->post_addr) ||
	     !access_ok(u64_to_user_ptr(args->post_addr), args->len))) {
		ret = -EINVAL;
		goto fail;
	}


  parent reply	other threads:[~2026-09-22 17:03 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-03-06 12:56 [PATCH] KVM: optionally post write on ioeventfd write Thanos Makatos
2026-03-12 15:02 ` David Woodhouse
2026-03-12 16:12   ` Thanos Makatos
2026-04-21 14:44     ` Sean Christopherson
2026-09-19 15:59       ` David Woodhouse
2026-03-23 15:01 ` Thanos Makatos
2026-04-10 19:11   ` Thanos Makatos
2026-04-21 14:45     ` Sean Christopherson
2026-06-10  7:31       ` John Levon
2026-09-22 13:51 ` Stefan Hajnoczi
2026-09-22 15:28   ` David Woodhouse
2026-09-22 16:47     ` Sean Christopherson
2026-10-04 16:16   ` David Woodhouse
2026-10-05  5:11     ` Sean Christopherson
2026-09-22 17:03 ` Sean Christopherson [this message]
  -- strict thread matches above, loose matches on Subject: below --
2026-01-13 20:00 [RFC PATCH] KVM: optionally commit " Sean Christopherson
2026-03-02 12:28 ` [PATCH] KVM: optionally post " Thanos Makatos
2026-03-05  1:26   ` Sean Christopherson
2026-03-06 11:14     ` Thanos Makatos
2026-03-05  1:49   ` kernel test robot
2026-03-05  9:39   ` kernel test robot

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=arK05GrLtiBvbfmm@google.com \
    --to=seanjc@google.com \
    --cc=john.levon@nutanix.com \
    --cc=kvm@vger.kernel.org \
    --cc=pbonzini@redhat.com \
    --cc=thanos.makatos@nutanix.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.