Re: [Qemu-devel] [PATCH v2] target-i386: coalesced PIO support for RTC

[Eduardo Habkost]

Hi,

On Thu, Jul 12, 2018 at 10:00:57AM +0800, Wanpeng Li wrote:
> From: Peng Hao <address@hidden>
> 
> Windows I/O, such as the real-time clock. The address register (port
> 0x70 in the RTC case) can use coalesced I/O, cutting the number of
> userspace exits by half when reading or writing the RTC.
> 
> Guest access rtc like this: write register index to 0x70, then write or 
> read data from 0x71. writing 0x70 port is just as index and do nothing 
> else. So we can use coalesced mmio to handle this scene to reduce VM-EXIT 
> time.
> 
> In our environment, 12 windows guest running on a Skylake server:
> 
> Before patch:
> 
> IO Port Access  Samples  Samples%   Time%    Avg time
> 
> 0x70:POUT        20675    46.04%    92.72%   67.15us ( +-   7.93% )
> 
> After patch:
> 
> IO Port Access  Samples  Samples%   Time%    Avg time
> 
> 0x70:POUT        17509    45.42%    42.08%   6.37us ( +-  20.37% )
> 
> Cc: Paolo Bonzini <address@hidden>
> Cc: Radim Krčmář <address@hidden>
> Cc: Eduardo Habkost <address@hidden>
> Cc: Peng Hao <address@hidden>
> Signed-off-by: Peng Hao <address@hidden>
> Signed-off-by: Wanpeng Li <address@hidden>

Thanks for the patch, and sorry for taking so long to review it.
Comments below:


> ---
> v1 -> v2:
>  * add the original author
> 
>  accel/kvm/kvm-all.c       | 56 
> +++++++++++++++++++++++++++++++++++++++++++----
>  hw/timer/mc146818rtc.c    |  8 +++++++
>  include/exec/memattrs.h   |  1 +
>  include/exec/memory.h     |  5 +++++
>  include/sysemu/kvm.h      |  8 +++++++
>  linux-headers/linux/kvm.h |  5 +++--
>  memory.c                  |  5 +++++
>  7 files changed, 82 insertions(+), 6 deletions(-)
> 
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index eb7db92..7a12341 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -128,6 +128,7 @@ bool kvm_direct_msi_allowed;
>  bool kvm_ioeventfd_any_length_allowed;
>  bool kvm_msi_use_devid;
>  static bool kvm_immediate_exit;
> +bool kvm_coalesced_pio_allowed;

I suggest following the same pattern used for coalesced MMIO: a
KVMState::coalesced_pio field.

>  
>  static const KVMCapabilityInfo kvm_required_capabilites[] = {
>      KVM_CAP_INFO(USER_MEMORY),
> @@ -536,7 +537,7 @@ static void kvm_coalesce_mmio_region(MemoryListener 
> *listener,
>  
>          zone.addr = start;
>          zone.size = size;
> -        zone.pad = 0;
> +        zone.pio = 0;

I'm not sure the KVM header update will really be done in a way
that would break existing code (see Radim's reply on the KVM
patch).  But if this happens, please do the header update in a
separate patch, and implement PIO coalescing in another one.
This makes the patches easier to review.


>  
>          (void)kvm_vm_ioctl(s, KVM_REGISTER_COALESCED_MMIO, &zone);
>      }
> @@ -553,7 +554,7 @@ static void kvm_uncoalesce_mmio_region(MemoryListener 
> *listener,
>  
>          zone.addr = start;
>          zone.size = size;
> -        zone.pad = 0;
> +        zone.pio = 0;
>  
>          (void)kvm_vm_ioctl(s, KVM_UNREGISTER_COALESCED_MMIO, &zone);
>      }
> @@ -877,6 +878,45 @@ static void kvm_io_ioeventfd_del(MemoryListener 
> *listener,
>      }
>  }
>  
> +static void kvm_coalesce_io_add(MemoryListener *listener,
> +                                MemoryRegionSection *section,
> +                                hwaddr start, hwaddr size)
> +{
> +    KVMState *s = kvm_state;
> +
> +    if (kvm_coalesced_pio_allowed) {
> +        struct kvm_coalesced_mmio_zone zone;
> +
> +        zone.addr = start;
> +        zone.size = size;
> +        zone.pio = 1;
> +
> +        (void)kvm_vm_ioctl(s, KVM_REGISTER_COALESCED_MMIO, &zone);
> +    }
> +}
> +
> +static void kvm_coalesce_io_del(MemoryListener *listener,
> +                                MemoryRegionSection *section,
> +                                hwaddr start, hwaddr size)
> +{
> +    KVMState *s = kvm_state;
> +
> +    if (kvm_coalesced_pio_allowed) {
> +        struct kvm_coalesced_mmio_zone zone;
> +
> +        zone.addr = start;
> +        zone.size = size;
> +        zone.pio = 1;
> +
> +        (void)kvm_vm_ioctl(s, KVM_UNREGISTER_COALESCED_MMIO, &zone);
> +    }
> +}
> +
> +static MemoryListener kvm_coalesced_io_listener = {
> +    .coalesced_mmio_add = kvm_coalesce_io_add,
> +    .coalesced_mmio_del = kvm_coalesce_io_del,
> +    .priority = 10,

Why exactly we need .priority=10 here?  Maybe this could be
explained in a comment?  (Or even better, by macros that explain
the priority levels?)


> +};
>  void kvm_memory_listener_register(KVMState *s, KVMMemoryListener *kml,
>                                    AddressSpace *as, int as_id)
>  {
> @@ -1615,6 +1655,8 @@ static int kvm_init(MachineState *ms)
>      }
>  
>      s->coalesced_mmio = kvm_check_extension(s, KVM_CAP_COALESCED_MMIO);
> +    kvm_coalesced_pio_allowed = s->coalesced_mmio &&
> +                        kvm_check_extension(s, KVM_CAP_COALESCED_PIO);
>  
>  #ifdef KVM_CAP_VCPU_EVENTS
>      s->vcpu_events = kvm_check_extension(s, KVM_CAP_VCPU_EVENTS);
> @@ -1694,6 +1736,8 @@ static int kvm_init(MachineState *ms)
>                                   &address_space_memory, 0);
>      memory_listener_register(&kvm_io_listener,
>                               &address_space_io);
> +    memory_listener_register(&kvm_coalesced_io_listener,
> +                             &address_space_io);
>  
>      s->many_ioeventfds = kvm_check_many_ioeventfds();
>  
> @@ -1775,8 +1819,12 @@ void kvm_flush_coalesced_mmio_buffer(void)
>              struct kvm_coalesced_mmio *ent;
>  
>              ent = &ring->coalesced_mmio[ring->first];
> -
> -            cpu_physical_memory_write(ent->phys_addr, ent->data, ent->len);
> +            if (ent->pio) {
> +                address_space_rw(&address_space_io, ent->phys_addr,
> +                                 MEMTXATTRS_NONE, ent->data, ent->len, true);
> +            } else {
> +                cpu_physical_memory_write(ent->phys_addr, ent->data, 
> ent->len);
> +            }

If the coalesced_mmio structs are going to be reused for PIO too,
I would suggest renaming them to "coalesced_io" to avoid
confusion.


>              smp_wmb();
>              ring->first = (ring->first + 1) % KVM_COALESCED_MMIO_MAX;
>          }
> diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
> index 6f1f723..8bd8682 100644
> --- a/hw/timer/mc146818rtc.c
> +++ b/hw/timer/mc146818rtc.c
> @@ -70,6 +70,7 @@ typedef struct RTCState {
>      ISADevice parent_obj;
>  
>      MemoryRegion io;
> +    MemoryRegion coalesced_io;
>      uint8_t cmos_data[128];
>      uint8_t cmos_index;
>      int32_t base_year;
> @@ -990,6 +991,13 @@ static void rtc_realizefn(DeviceState *dev, Error **errp)
>      memory_region_init_io(&s->io, OBJECT(s), &cmos_ops, s, "rtc", 2);
>      isa_register_ioport(isadev, &s->io, base);
>  
> +    if (memory_allow_coalesced_pio()) {
> +        memory_region_set_flush_coalesced(&s->io);
> +        memory_region_init_io(&s->coalesced_io, OBJECT(s), &cmos_ops,
> +                              s, "rtc1", 1);
> +        isa_register_ioport(isadev, &s->coalesced_io, base);
> +        memory_region_add_coalescing(&s->coalesced_io, 0, 1);
> +    }

[1]

Why is the memory_allow_coalesced_pio() call needed?  Won't the
coalesced memory regions be simply ignored if coalesced PIO is
unavailable on the host?

All the existing users of memory_region_add_coalescing() and
memory_region_set_flush_coalesced() seem to be unconditional, why
these ones need to be conditional?

I also suggest moving the RTC device changes to a separate patch,
so this patch could be split in 3 parts:

1/3: header update
2/3: new coalesced PIO API
3/3: use coalesced PIO support in RTC


>      qdev_set_legacy_instance_id(dev, base, 3);
>      qemu_register_reset(rtc_reset, s);
>  
> diff --git a/include/exec/memattrs.h b/include/exec/memattrs.h
> index d4a1642..97884d8 100644
> --- a/include/exec/memattrs.h
> +++ b/include/exec/memattrs.h
> @@ -45,6 +45,7 @@ typedef struct MemTxAttrs {
>   * from "didn't specify" if necessary).
>   */
>  #define MEMTXATTRS_UNSPECIFIED ((MemTxAttrs) { .unspecified = 1 })
> +#define MEMTXATTRS_NONE ((MemTxAttrs) { 0 })

Another reason to move the header updates to a separate patch.

>  
>  /* New-style MMIO accessors can indicate that the transaction failed.
>   * A zero (MEMTX_OK) response means success; anything else is a failure
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index 448d41a..2854907 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -1405,6 +1405,11 @@ void memory_region_set_flush_coalesced(MemoryRegion 
> *mr);
>  void memory_region_clear_flush_coalesced(MemoryRegion *mr);
>  
>  /**
> + * memory_allow_coalesced_pio: Check whether coalesced pio allowed.
> + */
> +bool memory_allow_coalesced_pio(void);

See above[1].  I don't see why this function is necessary.


> +
> +/**
>   * memory_region_clear_global_locking: Declares that access processing does
>   *                                     not depend on the QEMU global lock.
>   *
> diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
> index 0b64b8e..08366b2 100644
> --- a/include/sysemu/kvm.h
> +++ b/include/sysemu/kvm.h
> @@ -45,6 +45,7 @@ extern bool kvm_readonly_mem_allowed;
>  extern bool kvm_direct_msi_allowed;
>  extern bool kvm_ioeventfd_any_length_allowed;
>  extern bool kvm_msi_use_devid;
> +extern bool kvm_coalesced_pio_allowed;
>  
>  #define kvm_enabled()           (kvm_allowed)
>  /**
> @@ -167,6 +168,12 @@ extern bool kvm_msi_use_devid;
>   */
>  #define kvm_msi_devid_required() (kvm_msi_use_devid)
>  
> +/**
> + * kvm_coalesced_pio_enabled:
> + * Returns: true if KVM allow coalesced pio
> + */
> +#define kvm_coalesced_pio_enabled() (kvm_coalesced_pio_allowed)

See [1].


> +
>  #else
>  
>  #define kvm_enabled()           (0)
> @@ -184,6 +191,7 @@ extern bool kvm_msi_use_devid;
>  #define kvm_direct_msi_enabled() (false)
>  #define kvm_ioeventfd_any_length_enabled() (false)
>  #define kvm_msi_devid_required() (false)
> +#define kvm_coalesced_pio_enabled() (false)
>  
>  #endif  /* CONFIG_KVM_IS_POSSIBLE */
>  
> diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h
> index 98f389a..747b473 100644
> --- a/linux-headers/linux/kvm.h
> +++ b/linux-headers/linux/kvm.h
> @@ -420,13 +420,13 @@ struct kvm_run {
>  struct kvm_coalesced_mmio_zone {
>       __u64 addr;
>       __u32 size;
> -     __u32 pad;
> +     __u32 pio;
>  };
>  
>  struct kvm_coalesced_mmio {
>       __u64 phys_addr;
>       __u32 len;
> -     __u32 pad;
> +     __u32 pio;
>       __u8  data[8];
>  };
>  
> @@ -949,6 +949,7 @@ struct kvm_ppc_resize_hpt {
>  #define KVM_CAP_GET_MSR_FEATURES 153
>  #define KVM_CAP_HYPERV_EVENTFD 154
>  #define KVM_CAP_HYPERV_TLBFLUSH 155
> +#define KVM_CAP_COALESCED_PIO 156
>  
>  #ifdef KVM_CAP_IRQ_ROUTING
>  
> diff --git a/memory.c b/memory.c
> index e9cd446..4a32817 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -2211,6 +2211,11 @@ void memory_region_clear_global_locking(MemoryRegion 
> *mr)
>      mr->global_locking = false;
>  }
>  
> +bool memory_allow_coalesced_pio(void)
> +{
> +    return kvm_enabled() && kvm_coalesced_pio_enabled();
> +}

See [1].

> +
>  static bool userspace_eventfd_warning;
>  
>  void memory_region_add_eventfd(MemoryRegion *mr,
> -- 
> 2.7.4
> 

-- 
Eduardo