[Qemu-devel] [PATCH V3 0/4] introduce coalesced pio support

[Qemu-devel] [PATCH V3 0/4] introduce coalesced pio support

[Peng Hao]

Coalesced pio is base on coalesced mmio and can be used for some port
like rtc port, pci-host config port, virtio-pci config port and so on.

Specially in case of rtc as coalesced pio, some versions of windows guest 
access rtc frequently because of rtc as system tick. 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 pio to handle this scene to reduce VM-EXIT time.

without my patch, get the vm-exit time of accessing rtc 0x70 and piix 0xcf8
using perf tools: (guest OS : windows 7 64bit)
IO Port Access  Samples Samples%  Time%  Min Time  Max Time  Avg time
0x70:POUT        86     30.99%    74.59%   9us      29us    10.75us (+- 3.41%)
0xcf8:POUT     1119     2.60%     2.12%   2.79us    56.83us 3.41us (+- 2.23%)

with my patch
IO Port Access  Samples Samples%  Time%   Min Time  Max Time   Avg time
 0x70:POUT       106    32.02%    29.47%    0us      10us     1.57us (+- 7.38%)
0xcf8:POUT      1065    1.67%     0.28%   0.41us    65.44us   0.66us (+- 10.55%)

These are just qemu's patches, another patches are for kernel.
I think there is a need for further consideration in changing coalesecd_mmio's 
naming in kernel and qemu together.So maybe another patch does it.

Peng Hao (4):
  target-i386: introduce coalesced_pio kvm header update
  target-i386: add coalesced_pio API
  target-i386: add rtc 0x70 port as coalesced_pio
  target-i386: add i440fx 0xcf8 port as coalesced_pio

 accel/kvm/kvm-all.c       | 58 ++++++++++++++++++++++++++++++++++++++++++-----
 hw/pci-host/piix.c        |  4 ++++
 hw/timer/mc146818rtc.c    |  8 +++++++
 include/exec/memattrs.h   |  2 +-
 linux-headers/linux/kvm.h |  5 ++--
 5 files changed, 68 insertions(+), 9 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH V3 1/4] target-i386: introduce coalesced_pio kvm header update

[Peng Hao]

add coalesced_pio's struct and KVM_CAP_COALESCED_PIO.

Signed-off-by: Peng Hao <peng.hao2@zte.com.cn>
---
 linux-headers/linux/kvm.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

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
 
-- 
1.8.3.1

[Qemu-devel] [PATCH V3 2/4] target-i386:add coalesced_pio API

[Peng Hao]

Signed-off-by: Peng Hao <peng.hao2@zte.com.cn>
---
 accel/kvm/kvm-all.c     | 58 ++++++++++++++++++++++++++++++++++++++++++++-----
 include/exec/memattrs.h |  2 +-
 2 files changed, 53 insertions(+), 7 deletions(-)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index eb7db92..830745e 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -78,6 +78,7 @@ struct KVMState
     int fd;
     int vmfd;
     int coalesced_mmio;
+    int coalesced_pio;
     struct kvm_coalesced_mmio_ring *coalesced_mmio_ring;
     bool coalesced_flush_in_progress;
     int vcpu_events;
@@ -536,7 +537,7 @@ static void kvm_coalesce_mmio_region(MemoryListener *listener,
 
         zone.addr = start;
         zone.size = size;
-        zone.pad = 0;
+        zone.pio = 0;
 
         (void)kvm_vm_ioctl(s, KVM_REGISTER_COALESCED_MMIO, &zone);
     }
@@ -553,12 +554,51 @@ 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);
     }
 }
 
+static void kvm_coalesce_io_add(MemoryListener *listener,
+                                MemoryRegionSection *section,
+                                hwaddr start, hwaddr size)
+{
+    KVMState *s = kvm_state;
+
+    if (s->coalesced_pio) {
+        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 (s->coalesced_pio) {
+        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,
+};
+
 int kvm_check_extension(KVMState *s, unsigned int extension)
 {
     int ret;
@@ -1615,7 +1655,8 @@ static int kvm_init(MachineState *ms)
     }
 
     s->coalesced_mmio = kvm_check_extension(s, KVM_CAP_COALESCED_MMIO);
-
+    s->coalesced_pio = 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);
 #endif
@@ -1694,7 +1735,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();
 
     s->sync_mmu = !!kvm_vm_check_extension(kvm_state, KVM_CAP_SYNC_MMU);
@@ -1775,8 +1817,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 == 1) {
+                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);
+            }
             smp_wmb();
             ring->first = (ring->first + 1) % KVM_COALESCED_MMIO_MAX;
         }
diff --git a/include/exec/memattrs.h b/include/exec/memattrs.h
index d4a1642..12fd64f 100644
--- a/include/exec/memattrs.h
+++ b/include/exec/memattrs.h
@@ -45,7 +45,7 @@ typedef struct MemTxAttrs {
  * from "didn't specify" if necessary).
  */
 #define MEMTXATTRS_UNSPECIFIED ((MemTxAttrs) { .unspecified = 1 })
-
+#define MEMTXATTRS_NONE ((MemTxAttrs) { 0 })
 /* New-style MMIO accessors can indicate that the transaction failed.
  * A zero (MEMTX_OK) response means success; anything else is a failure
  * of some kind. The memory subsystem will bitwise-OR together results
-- 
1.8.3.1

[Qemu-devel] [PATCH V3 3/4] target-i386: add rtc 0x70 port as coalesced_pio

[Peng Hao]

Signed-off-by: Peng Hao <peng.hao2@zte.com.cn>
---
 hw/timer/mc146818rtc.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
index 6f1f723..1ef5f77 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);
 
+    /* register rtc 0x70 port for 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);
+
     qdev_set_legacy_instance_id(dev, base, 3);
     qemu_register_reset(rtc_reset, s);
 
-- 
1.8.3.1

[Qemu-devel] [PATCH V3 4/4] target-i386: add i440fx 0xcf8 port as coalesced_pio

[Peng Hao]

Signed-off-by: Peng Hao <peng.hao2@zte.com.cn>
---
 hw/pci-host/piix.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
index 0e60834..da73743 100644
--- a/hw/pci-host/piix.c
+++ b/hw/pci-host/piix.c
@@ -327,6 +327,10 @@ static void i440fx_pcihost_realize(DeviceState *dev, Error **errp)
 
     sysbus_add_io(sbd, 0xcfc, &s->data_mem);
     sysbus_init_ioports(sbd, 0xcfc, 4);
+
+    /* register i440fx 0xcf8 port as coalesced pio */
+    memory_region_set_flush_coalesced(&s->data_mem);
+    memory_region_add_coalescing(&s->conf_mem, 0, 4);
 }
 
 static void i440fx_realize(PCIDevice *dev, Error **errp)
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH V3 2/4] target-i386:add coalesced_pio API

[Eduardo Habkost]

On Fri, Aug 24, 2018 at 12:14:47AM +0800, Peng Hao wrote:
> Signed-off-by: Peng Hao <peng.hao2@zte.com.cn>
> ---
>  accel/kvm/kvm-all.c     | 58 ++++++++++++++++++++++++++++++++++++++++++++-----
>  include/exec/memattrs.h |  2 +-
>  2 files changed, 53 insertions(+), 7 deletions(-)
> 
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index eb7db92..830745e 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -78,6 +78,7 @@ struct KVMState
>      int fd;
>      int vmfd;
>      int coalesced_mmio;
> +    int coalesced_pio;
>      struct kvm_coalesced_mmio_ring *coalesced_mmio_ring;
>      bool coalesced_flush_in_progress;
>      int vcpu_events;
> @@ -536,7 +537,7 @@ static void kvm_coalesce_mmio_region(MemoryListener *listener,
>  
>          zone.addr = start;
>          zone.size = size;
> -        zone.pad = 0;
> +        zone.pio = 0;
>  
>          (void)kvm_vm_ioctl(s, KVM_REGISTER_COALESCED_MMIO, &zone);
>      }
> @@ -553,12 +554,51 @@ 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);
>      }
>  }

The 2 hunks above need to go to patch 1/4 to avoid breaking the
build after applying 1/4.

However, patch 1/4 is just a header update and I don't think
running update-linux-headers.sh was supposed to break the build.

Radim asked about this at:

 Date: Wed, 18 Jul 2018 16:43:11 +0200
 Subject: Re: [PATCH v2] KVM: Add coalesced PIO support
 Message-ID: <20180718144311.GB6348@flask>

Paolo, do you have a suggestion on how to update the struct
without making a header update break the build?


>  
> +static void kvm_coalesce_io_add(MemoryListener *listener,
> +                                MemoryRegionSection *section,
> +                                hwaddr start, hwaddr size)
> +{
> +    KVMState *s = kvm_state;
> +
> +    if (s->coalesced_pio) {
> +        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 (s->coalesced_pio) {
> +        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,

Nit: I would rename the fields to "coalesced_mmio_{add,del}" to
coalesced_io_*, because they are not specific to MMIO anymore.

I would also name the functions kvm_coalesce_pio_{add,del}
because the are specific to pio.


> +};
> +
>  int kvm_check_extension(KVMState *s, unsigned int extension)
>  {
>      int ret;
> @@ -1615,7 +1655,8 @@ static int kvm_init(MachineState *ms)
>      }
>  
>      s->coalesced_mmio = kvm_check_extension(s, KVM_CAP_COALESCED_MMIO);
> -
> +    s->coalesced_pio = s->coalesced_mmio &&
> +                       kvm_check_extension(s, KVM_CAP_COALESCED_PIO);

Nit: I would keep the blank line here.

>  #ifdef KVM_CAP_VCPU_EVENTS
>      s->vcpu_events = kvm_check_extension(s, KVM_CAP_VCPU_EVENTS);
>  #endif
> @@ -1694,7 +1735,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);

Nit: I would keep the blank line here.

>      s->many_ioeventfds = kvm_check_many_ioeventfds();
>  
>      s->sync_mmu = !!kvm_vm_check_extension(kvm_state, KVM_CAP_SYNC_MMU);
> @@ -1775,8 +1817,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 == 1) {
> +                address_space_rw(&address_space_io, ent->phys_addr,
> +                                 MEMTXATTRS_NONE, ent->data, ent->len, true);

Why exactly MEMTXATTRS_NONE is the right attrs argument here?
Why MEMTXATTRS_UNSPECIFIED wouldn't work?


> +            } else {
> +                cpu_physical_memory_write(ent->phys_addr, ent->data, ent->len);
> +            }
>              smp_wmb();
>              ring->first = (ring->first + 1) % KVM_COALESCED_MMIO_MAX;
>          }
> diff --git a/include/exec/memattrs.h b/include/exec/memattrs.h
> index d4a1642..12fd64f 100644
> --- a/include/exec/memattrs.h
> +++ b/include/exec/memattrs.h
> @@ -45,7 +45,7 @@ typedef struct MemTxAttrs {
>   * from "didn't specify" if necessary).
>   */
>  #define MEMTXATTRS_UNSPECIFIED ((MemTxAttrs) { .unspecified = 1 })
> -
> +#define MEMTXATTRS_NONE ((MemTxAttrs) { 0 })

Nit: I would keep the blank line here.

>  /* New-style MMIO accessors can indicate that the transaction failed.
>   * A zero (MEMTX_OK) response means success; anything else is a failure
>   * of some kind. The memory subsystem will bitwise-OR together results
> -- 
> 1.8.3.1
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH V3 2/4] target-i386:add coalesced_pio API

[peng.hao2]

> On Fri, Aug 24, 2018 at 12:14:47AM +0800, Peng Hao wrote:
> > Signed-off-by: Peng Hao <peng.hao2@zte.com.cn>
> > ---
> >  accel/kvm/kvm-all.c     | 58 ++++++++++++++++++++++++++++++++++++++++++++-----
> >  include/exec/memattrs.h |  2 +-
> >  2 files changed, 53 insertions(+), 7 deletions(-)
> > 

>> @@ -536,7 +537,7 @@ static void kvm_coalesce_mmio_region(MemoryListener *listener,
> >  
> >          zone.addr = start;
> >          zone.size = size;
> > -        zone.pad = 0;
> > +        zone.pio = 0;
> >  
> >          (void)kvm_vm_ioctl(s, KVM_REGISTER_COALESCED_MMIO, &zone);
> >      }
> > @@ -553,12 +554,51 @@ 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);
> >      }
> >  }
> 
> The 2 hunks above need to go to patch 1/4 to avoid breaking the
> build after applying 1/4.
> 
> However, patch 1/4 is just a header update and I don't think
> running update-linux-headers.sh was supposed to break the build.
> 
> Radim asked about this at:
>  Date: Wed, 18 Jul 2018 16:43:11 +0200
 > Subject: Re: [PATCH v2] KVM: Add coalesced PIO support
 > Message-ID: <20180718144311.GB6348@flask>
> Paolo, do you have a suggestion on how to update the struct
> without making a header update break the build?
> 

> > +static MemoryListener kvm_coalesced_io_listener = {
> > +    .coalesced_mmio_add = kvm_coalesce_io_add,
> > +    .coalesced_mmio_del = kvm_coalesce_io_del,

> Nit: I would rename the fields to "coalesced_mmio_{add,del}" to
> coalesced_io_*, because they are not specific to MMIO anymore.

> I would also name the functions kvm_coalesce_pio_{add,del}
> because the are specific to pio.

  I will modify in next patch. 

> > +};
> > +
> >  int kvm_check_extension(KVMState *s, unsigned int extension)
> >  {
> >      int ret;
> > @@ -1615,7 +1655,8 @@ static int kvm_init(MachineState *ms)

> >  
> >              ent = &ring->coalesced_mmio[ring->first];
> > -
> > -            cpu_physical_memory_write(ent->phys_addr, ent->data, ent->len);
> > +            if (ent->pio == 1) {
> > +                address_space_rw(&address_space_io, ent->phys_addr,
> > +                                 MEMTXATTRS_NONE, ent->data, ent->len, true);

> Why exactly MEMTXATTRS_NONE is the right attrs argument here?
> Why MEMTXATTRS_UNSPECIFIED wouldn't work?

I didn't notice MEMTXATTRS_NONE is the same as MEMTXATTRS_UNSPECIFIED.
MEMTXATTRS_NONE is redundant.
>> +            } else {
>> +                cpu_physical_memory_write(ent->phys_addr, ent->data, ent->len);
>> +            }
>>              smp_wmb();
>>              ring->first = (ring->first + 1) % KVM_COALESCED_MMIO_MAX;
>>          }
>> diff --git a/include/exec/memattrs.h b/include/exec/memattrs.h
>> index d4a1642..12fd64f 100644
>> --- a/include/exec/memattrs.h
>> +++ b/include/exec/memattrs.h
>> @@ -45,7 +45,7 @@ typedef struct MemTxAttrs {
>>   * from "didn't specify" if necessary).
>>   */
>>  #define MEMTXATTRS_UNSPECIFIED ((MemTxAttrs) { .unspecified = 1 })
>> -
>> +#define MEMTXATTRS_NONE ((MemTxAttrs) { 0 })

Re: [Qemu-devel] [PATCH V3 2/4] target-i386:add coalesced_pio API

[Eduardo Habkost]

On Fri, Aug 24, 2018 at 05:18:45PM +0800, peng.hao2@zte.com.cn wrote:
[...]
> > > -            cpu_physical_memory_write(ent->phys_addr, ent->data, ent->len);
> > > +            if (ent->pio == 1) {
> > > +                address_space_rw(&address_space_io, ent->phys_addr,
> > > +                                 MEMTXATTRS_NONE, ent->data, ent->len, true);
> 
> > Why exactly MEMTXATTRS_NONE is the right attrs argument here?
> > Why MEMTXATTRS_UNSPECIFIED wouldn't work?
> 
> I didn't notice MEMTXATTRS_NONE is the same as MEMTXATTRS_UNSPECIFIED.
> MEMTXATTRS_NONE is redundant.

They are not exactly the same (see below), but in either case I'm
not sure it would make any difference for PIO.


> >> +            } else {
> >> +                cpu_physical_memory_write(ent->phys_addr, ent->data, ent->len);
> >> +            }
> >>              smp_wmb();
> >>              ring->first = (ring->first + 1) % KVM_COALESCED_MMIO_MAX;
> >>          }
> >> diff --git a/include/exec/memattrs.h b/include/exec/memattrs.h
> >> index d4a1642..12fd64f 100644
> >> --- a/include/exec/memattrs.h
> >> +++ b/include/exec/memattrs.h
> >> @@ -45,7 +45,7 @@ typedef struct MemTxAttrs {
> >>   * from "didn't specify" if necessary).
> >>   */
> >>  #define MEMTXATTRS_UNSPECIFIED ((MemTxAttrs) { .unspecified = 1 })
> >> -
> >> +#define MEMTXATTRS_NONE ((MemTxAttrs) { 0 })


-- 
Eduardo