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

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

Hi Andre,

On 2/17/21 5:43 PM, Andre Przywara wrote:
> On Thu, 11 Feb 2021 16:10:16 +0000
> Alexandru Elisei <alexandru.elisei@arm.com> wrote:
>
> Hi,
>
>> On 12/10/20 2:28 PM, 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.  
>> Maybe I'm missing something, but why two trees? Is it valid to have an overlap
>> between IO port and MMIO emulation? Or was it done to make the removal of ioport
>> emulation easier?
> So I thought about it as well, but figured it's easier this way:
> - x86 allows overlap, PIO is a totally separate address space from
>   memory/MMIO. Early x86 CPUs had pins to indicate a PIO bus cycle, but
>   using the same address and data pins otherwise. In practise there
>   might be no overlap when it comes to *MMIO* traps vs PIO on x86
>   (there is DRAM only at the lowest 64K of the IBM PC memory map),
>   but not sure we should rely on this.
> - For non-x86 this would indeed be non-overlapping, but this would need
>   to be translated at init time then? And then we can't move those
>   anymore, I guess? So I found it cleaner to keep this separate, and do
>   the translation at trap time.
> - As a consequence we would need to have a bit indicating the address
>   space. I haven't actually tried this, but my understanding is that
>   this would spoil the whole rb_tree functions, since they rely on a
>   linear addressing scheme, and adding another bit there would be at
>   least cumbersome?
>
> At the end I decided to go for separate trees, as also this was less
> change.
>
> I agree that it would be nice to have one tree, from a design point of
> view, but I am afraid that would require more changes.
> If need be, I think we can always unify them later on, on top of this
> series?

Definitely later, I forgot that x86 uses special instructions to access IO ports,
which means that port addresses can overlap with memory addresses. Let's keep 2
trees for now and we can decide later if we should unify them for the other
architectures.

>
>> If it's not valid to have that overlap, then I think having one tree for both
>> would better. Struct mmio_mapping would have to be augmented with a flags field
>> that holds the same flags given to kvm__register_iotrap to differentiate between
>> the two slightly different emulations. Saving the IOTRAP_COALESCE flag would also
>> make it trivial to call KVM_UNREGISTER_COALESCED_MMIO in kvm__deregister_iotrap,
>> which we currently don't do.
>>
>>> "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.  
>> I'm sorry, but I don't understand that last sentence. Do you mean that the ioport
>> emulation code has been modified to use kvm__emulate_pio() as a fallback for when
>> the port is not found in the ioport_tree?
> I meant that for the transition period we have all of traditional MMIO,
> traditional PIO, *and* just transformed PIO.
>
> That means there are still PIO devices registered through ioport.c's
> ioport__register(), *and* PIO devices registered through mmio.c's
> kvm__register_pio(). Which means they end up in two separate PIO trees.
> And only the traditional kvm__emulate_io() from ioport.c is called upon
> a trap, so it needs to check both trees, which it does by calling into
> kvm__emulate_pio(), shall a search in the local tree fail.

Thank you for the explanation, now it makes sense.

>
> Or did you mean something else?
>
>>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>>> ---
>>>  include/kvm/kvm.h | 42 +++++++++++++++++++++++++++++----
>>>  ioport.c          |  4 ++--
>>>  mmio.c            | 59 +++++++++++++++++++++++++++++++++++++++--------
>>>  3 files changed, 89 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/include/kvm/kvm.h b/include/kvm/kvm.h
>>> index ee99c28e..14f9d58b 100644
>>> --- a/include/kvm/kvm.h
>>> +++ b/include/kvm/kvm.h
>>> @@ -27,10 +27,16 @@
>>>  #define PAGE_SIZE (sysconf(_SC_PAGE_SIZE))
>>>  #endif
>>>  
>>> +#define IOTRAP_BUS_MASK		0xf  
>> It's not immediately obvious what this mask does. It turns out it's used to mask
>> the enum flags defined in the header devices.h, header which is not included in
>> this file.
>>
>> The flag names we pass to kvm__register_iotrap() are slightly inconsistent
>> (DEVICE_BUS_PCI, DEVICE_BUS_MMIO and IOTRAP_COALESCE), where DEVICE_BUS_{PCI,
>> MMIO} come from devices.h as an enum. I was wondering if I'm missing something and
>> there is a particular reason why we don't define our own flags for that here
>> (something like IOTRAP_PIO and IOTRAP_MMIO).
> I am not sure why this would be needed?
> We already define and use DEVICE_BUS_x elsewhere, so why not re-use it?

To check if we should coalesce the MMIO regions we check the IOTRAP_COALESCE bit,
but to check if we should use the mmio or io tree we use a mask over the first 4
bits and compare that to DEVICE_BUS_xxx. I find that confusing: the
IOTRAP_BUS_MASK and IOTRAP_COALESCE are defined here, but there is no evidence
where the other flags are coming from, and the header file devices.h isn't even
included.

>
>> If we do decide to keep the flags from devices.h, I think it would be worth it to
>> have a compile time check (with BUILD_BUG_ON) that IOTRAP_BUS_MASK is >=
>> DEVICES_BUS_MAX, which would also be a good indication of where those flags are
>> coming from.
> Well, if that makes you happy, I am not sure we gain another 13 bus
> types anytime soon, though ;-)

I was actually suggesting that more for documenting the code. If we compare
IOTRAP_BUS_MASK with DEVICE_BUS_MAX, then that will give us a hint where we're
expecting the flags to be defined.

But I had another look at the code and it seems that DEVICE_BUS_MMIO = 1 and
DEVICE_BUS_IOPORT = 2, which means that we don't need the mask and we can check
the bits instead (and IOTRAP_COALESCE can be redefined to be 1U << 2). I think
dropping the mask and replacing it with testing individual bits would make the
code easier to follow, what do you think?

>  
>>> +#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);
>>>  
>>>  enum {
>>> @@ -113,6 +119,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 +144,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 b98836d3..204d8103 100644
>>> --- a/ioport.c
>>> +++ b/ioport.c
>>> @@ -147,7 +147,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);  
>> I have to admit this gave me pause because this patch doesn't add any users for
>> kvm__register_pio() (although with this change the behaviour of kvm__emulate_io()
>> remains exactly the same). Do you think this change would fit better in patch #7,
>> where the first user for kvm__register_pio() is added, or do you prefer it here?
> I think it logically belongs here, as we introduce the
> kvm__emulate_pio() function here as well. Otherwise this function would
> have no caller. As it is now, it's just a "coincidence" that no one
> actually called kvm__register_pio() so far. This also makes the other
> patches movable and replaceable: this patch prepares the stage, the
> follow-up patches just fill it.

Sure, makes sense.

>
>>>  
>>>  	ops	= entry->ops;
>>>  
>>> @@ -162,7 +163,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..4cce1901 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,9 @@ 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)
>>> +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 +128,7 @@ int kvm__register_mmio(struct kvm *kvm, u64
>>> phys_addr, u64 phys_addr_len, bool c .remove		= false,
>>>  	};
>>>  
>>> -	if (coalesce) {
>>> +	if (flags & IOTRAP_COALESCE) {  
>> There is no such flag being used in ioport.c, is it valid to have the
>> flags DEVICE_BUS_IOPORT and IOTRAP_COALESCE set at the same time?
> Well, yes and no: Yes, as this maps to MMIO on non-x86, so
> theoretically could use the flag. No, as no one registering a trap
> handler through kvm__register_pio() would ever have the chance to set
> this flag.
> I can check for the registration being for the MMIO bus before entering
> the "if" branch, if that is what you mean?

Yes, that's what I mean, please return an error if a device tries to register an
I/O port and sets the COALESCE flag, since that's forbidden for architectures with
true I/O ports (like x86). I realize it might look peculiar since we don't have
any devices that do that, but I think it can be useful for catching errors when
writing new devices.

Thanks,

Alex

>
>>>  		zone = (struct kvm_coalesced_mmio_zone) {
>>>  			.addr	= phys_addr,
>>>  			.size	= phys_addr_len,
>>> @@ -139,18 +140,27 @@ int kvm__register_mmio(struct kvm *kvm, u64
>>> phys_addr, u64 phys_addr_len, bool c }
>>>  	}
>>>  	mutex_lock(&mmio_lock);
>>> -	ret = mmio_insert(&mmio_tree, mmio);
>>> +	if ((flags & IOTRAP_BUS_MASK) == DEVICE_BUS_IOPORT)
>>> +		ret = mmio_insert(&pio_tree, mmio);
>>> +	else
>>> +		ret = mmio_insert(&mmio_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;
>>>  
>>>  	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 +177,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 +185,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)  
>> I don't think style changes should be part of this patch, the patch
>> is large enough as it is.
> I see, I just figured it's not worth a separate patch either.
>
> Cheers,
> Andre
>
>>>  {
>>>  	struct mmio_mapping *mmio;
>>>  
>>> @@ -194,3 +205,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;
>>> +}