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@xxxxxxx> 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@xxxxxxx>
>>> ---
>>> 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;
>>> +}
