Re: [PATCH kvmtool v2 04/22] mmio: Extend handling to include ioport emulation

[Alexandru Elisei <alexandru.elisei@arm.com>]

Hi Andre,

On 2/25/21 12:58 AM, Andre Przywara wrote:
> In their core functionality MMIO and I/O port traps are not really
> different, yet we still have two totally separate code paths for
> handling them. Devices need to decide on one conduit or need to provide
> different handler functions for each of them.
>
> Extend the existing MMIO emulation to also cover ioport handlers.
> This just adds another RB tree root for holding the I/O port handlers,
> but otherwise uses the same tree population and lookup code.
> "ioport" or "mmio" just become a flag in the registration function.
> Provide wrappers to not break existing users, and allow an easy
> transition for the existing ioport handlers.
>
> This also means that ioport handlers now can use the same emulation
> callback prototype as MMIO handlers, which means we have to migrate them
> over. To allow a smooth transition, we hook up the new I/O emulate
> function to the end of the existing ioport emulation code.
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  include/kvm/kvm.h | 49 ++++++++++++++++++++++++++++++++---
>  ioport.c          |  4 +--
>  mmio.c            | 65 +++++++++++++++++++++++++++++++++++++++--------
>  3 files changed, 102 insertions(+), 16 deletions(-)
>
> diff --git a/include/kvm/kvm.h b/include/kvm/kvm.h
> index f1f0afd7..306b258a 100644
> --- a/include/kvm/kvm.h
> +++ b/include/kvm/kvm.h
> @@ -27,10 +27,23 @@
>  #define PAGE_SIZE (sysconf(_SC_PAGE_SIZE))
>  #endif
>  
> +/*
> + * We are reusing the existing DEVICE_BUS_MMIO and DEVICE_BUS_IOPORT constants
> + * from kvm/devices.h to differentiate between registering an I/O port and an
> + * MMIO region.
> + * To avoid collisions with future additions of more bus types, we reserve
> + * a generous 4 bits for the bus mask here.
> + */
> +#define IOTRAP_BUS_MASK		0xf
> +#define IOTRAP_COALESCE		(1U << 4)
> +
>  #define DEFINE_KVM_EXT(ext)		\
>  	.name = #ext,			\
>  	.code = ext
>  
> +struct kvm_cpu;
> +typedef void (*mmio_handler_fn)(struct kvm_cpu *vcpu, u64 addr, u8 *data,
> +				u32 len, u8 is_write, void *ptr);
>  typedef void (*fdt_irq_fn)(void *fdt, u8 irq, enum irq_type irq_type);
>  
>  enum {
> @@ -113,6 +126,8 @@ void kvm__irq_line(struct kvm *kvm, int irq, int level);
>  void kvm__irq_trigger(struct kvm *kvm, int irq);
>  bool kvm__emulate_io(struct kvm_cpu *vcpu, u16 port, void *data, int direction, int size, u32 count);
>  bool kvm__emulate_mmio(struct kvm_cpu *vcpu, u64 phys_addr, u8 *data, u32 len, u8 is_write);
> +bool kvm__emulate_pio(struct kvm_cpu *vcpu, u16 port, void *data,
> +		      int direction, int size, u32 count);
>  int kvm__destroy_mem(struct kvm *kvm, u64 guest_phys, u64 size, void *userspace_addr);
>  int kvm__register_mem(struct kvm *kvm, u64 guest_phys, u64 size, void *userspace_addr,
>  		      enum kvm_mem_type type);
> @@ -136,10 +151,36 @@ static inline int kvm__reserve_mem(struct kvm *kvm, u64 guest_phys, u64 size)
>  				 KVM_MEM_TYPE_RESERVED);
>  }
>  
> -int __must_check kvm__register_mmio(struct kvm *kvm, u64 phys_addr, u64 phys_addr_len, bool coalesce,
> -				    void (*mmio_fn)(struct kvm_cpu *vcpu, u64 addr, u8 *data, u32 len, u8 is_write, void *ptr),
> -				    void *ptr);
> -bool kvm__deregister_mmio(struct kvm *kvm, u64 phys_addr);
> +int __must_check kvm__register_iotrap(struct kvm *kvm, u64 phys_addr, u64 len,
> +				      mmio_handler_fn mmio_fn, void *ptr,
> +				      unsigned int flags);
> +
> +static inline
> +int __must_check kvm__register_mmio(struct kvm *kvm, u64 phys_addr,
> +				    u64 phys_addr_len, bool coalesce,
> +				    mmio_handler_fn mmio_fn, void *ptr)
> +{
> +	return kvm__register_iotrap(kvm, phys_addr, phys_addr_len, mmio_fn, ptr,
> +			DEVICE_BUS_MMIO | (coalesce ? IOTRAP_COALESCE : 0));
> +}
> +static inline
> +int __must_check kvm__register_pio(struct kvm *kvm, u16 port, u16 len,
> +				   mmio_handler_fn mmio_fn, void *ptr)
> +{
> +	return kvm__register_iotrap(kvm, port, len, mmio_fn, ptr,
> +				    DEVICE_BUS_IOPORT);
> +}
> +
> +bool kvm__deregister_iotrap(struct kvm *kvm, u64 phys_addr, unsigned int flags);
> +static inline bool kvm__deregister_mmio(struct kvm *kvm, u64 phys_addr)
> +{
> +	return kvm__deregister_iotrap(kvm, phys_addr, DEVICE_BUS_MMIO);
> +}
> +static inline bool kvm__deregister_pio(struct kvm *kvm, u16 port)
> +{
> +	return kvm__deregister_iotrap(kvm, port, DEVICE_BUS_IOPORT);
> +}
> +
>  void kvm__reboot(struct kvm *kvm);
>  void kvm__pause(struct kvm *kvm);
>  void kvm__continue(struct kvm *kvm);
> diff --git a/ioport.c b/ioport.c
> index e0123f27..ce29e7e7 100644
> --- a/ioport.c
> +++ b/ioport.c
> @@ -162,7 +162,8 @@ bool kvm__emulate_io(struct kvm_cpu *vcpu, u16 port, void *data, int direction,
>  
>  	entry = ioport_get(&ioport_tree, port);
>  	if (!entry)
> -		goto out;
> +		return kvm__emulate_pio(vcpu, port, data, direction,
> +					size, count);
>  
>  	ops	= entry->ops;
>  
> @@ -177,7 +178,6 @@ bool kvm__emulate_io(struct kvm_cpu *vcpu, u16 port, void *data, int direction,
>  
>  	ioport_put(&ioport_tree, entry);
>  
> -out:
>  	if (ret)
>  		return true;
>  
> diff --git a/mmio.c b/mmio.c
> index cd141cd3..75de04ad 100644
> --- a/mmio.c
> +++ b/mmio.c
> @@ -19,13 +19,14 @@ static DEFINE_MUTEX(mmio_lock);
>  
>  struct mmio_mapping {
>  	struct rb_int_node	node;
> -	void			(*mmio_fn)(struct kvm_cpu *vcpu, u64 addr, u8 *data, u32 len, u8 is_write, void *ptr);
> +	mmio_handler_fn		mmio_fn;
>  	void			*ptr;
>  	u32			refcount;
>  	bool			remove;
>  };
>  
>  static struct rb_root mmio_tree = RB_ROOT;
> +static struct rb_root pio_tree = RB_ROOT;
>  
>  static struct mmio_mapping *mmio_search(struct rb_root *root, u64 addr, u64 len)
>  {
> @@ -103,9 +104,14 @@ static void mmio_put(struct kvm *kvm, struct rb_root *root, struct mmio_mapping
>  	mutex_unlock(&mmio_lock);
>  }
>  
> -int kvm__register_mmio(struct kvm *kvm, u64 phys_addr, u64 phys_addr_len, bool coalesce,
> -		       void (*mmio_fn)(struct kvm_cpu *vcpu, u64 addr, u8 *data, u32 len, u8 is_write, void *ptr),
> -			void *ptr)
> +static bool trap_is_mmio(unsigned int flags)
> +{
> +	return (flags & IOTRAP_BUS_MASK) == DEVICE_BUS_MMIO;
> +}
> +
> +int kvm__register_iotrap(struct kvm *kvm, u64 phys_addr, u64 phys_addr_len,
> +			 mmio_handler_fn mmio_fn, void *ptr,
> +			 unsigned int flags)
>  {
>  	struct mmio_mapping *mmio;
>  	struct kvm_coalesced_mmio_zone zone;
> @@ -127,7 +133,7 @@ int kvm__register_mmio(struct kvm *kvm, u64 phys_addr, u64 phys_addr_len, bool c
>  		.remove		= false,
>  	};
>  
> -	if (coalesce) {
> +	if (trap_is_mmio(flags) && (flags & IOTRAP_COALESCE)) {
>  		zone = (struct kvm_coalesced_mmio_zone) {
>  			.addr	= phys_addr,
>  			.size	= phys_addr_len,
> @@ -138,19 +144,29 @@ int kvm__register_mmio(struct kvm *kvm, u64 phys_addr, u64 phys_addr_len, bool c
>  			return -errno;
>  		}
>  	}
> +
>  	mutex_lock(&mmio_lock);
> -	ret = mmio_insert(&mmio_tree, mmio);
> +	if (trap_is_mmio(flags))
> +		ret = mmio_insert(&mmio_tree, mmio);
> +	else
> +		ret = mmio_insert(&pio_tree, mmio);
>  	mutex_unlock(&mmio_lock);
>  
>  	return ret;
>  }
>  
> -bool kvm__deregister_mmio(struct kvm *kvm, u64 phys_addr)
> +bool kvm__deregister_iotrap(struct kvm *kvm, u64 phys_addr, unsigned int flags)
>  {
>  	struct mmio_mapping *mmio;
> +	struct rb_root *tree;
> +
> +	if ((flags & IOTRAP_BUS_MASK) == DEVICE_BUS_IOPORT)
> +		tree = &pio_tree;
> +	else
> +		tree = &mmio_tree;

We could swap the conditional branches and use trap_is_mmio(flags) like we do above.

Regardless, the patch looks really good:

Reviewed-by: Alexandru Elisei <alexandru.elisei@arm.com>

Thanks,

Alex

>  
>  	mutex_lock(&mmio_lock);
> -	mmio = mmio_search_single(&mmio_tree, phys_addr);
> +	mmio = mmio_search_single(tree, phys_addr);
>  	if (mmio == NULL) {
>  		mutex_unlock(&mmio_lock);
>  		return false;
> @@ -167,7 +183,7 @@ bool kvm__deregister_mmio(struct kvm *kvm, u64 phys_addr)
>  	 * called mmio_put(). This will trigger use-after-free errors on VCPU0.
>  	 */
>  	if (mmio->refcount == 0)
> -		mmio_deregister(kvm, &mmio_tree, mmio);
> +		mmio_deregister(kvm, tree, mmio);
>  	else
>  		mmio->remove = true;
>  	mutex_unlock(&mmio_lock);
> @@ -175,7 +191,8 @@ bool kvm__deregister_mmio(struct kvm *kvm, u64 phys_addr)
>  	return true;
>  }
>  
> -bool kvm__emulate_mmio(struct kvm_cpu *vcpu, u64 phys_addr, u8 *data, u32 len, u8 is_write)
> +bool kvm__emulate_mmio(struct kvm_cpu *vcpu, u64 phys_addr, u8 *data,
> +		       u32 len, u8 is_write)
>  {
>  	struct mmio_mapping *mmio;
>  
> @@ -194,3 +211,31 @@ bool kvm__emulate_mmio(struct kvm_cpu *vcpu, u64 phys_addr, u8 *data, u32 len, u
>  out:
>  	return true;
>  }
> +
> +bool kvm__emulate_pio(struct kvm_cpu *vcpu, u16 port, void *data,
> +		     int direction, int size, u32 count)
> +{
> +	struct mmio_mapping *mmio;
> +	bool is_write = direction == KVM_EXIT_IO_OUT;
> +
> +	mmio = mmio_get(&pio_tree, port, size);
> +	if (!mmio) {
> +		if (vcpu->kvm->cfg.ioport_debug) {
> +			fprintf(stderr, "IO error: %s port=%x, size=%d, count=%u\n",
> +				to_direction(direction), port, size, count);
> +
> +			return false;
> +		}
> +		return true;
> +	}
> +
> +	while (count--) {
> +		mmio->mmio_fn(vcpu, port, data, size, is_write, mmio->ptr);
> +
> +		data += size;
> +	}
> +
> +	mmio_put(vcpu->kvm, &pio_tree, mmio);
> +
> +	return true;
> +}