On Fri, Aug 24, 2018 at 12:14:47AM +0800, Peng Hao wrote:
> Signed-off-by: Peng Hao <peng.hao2@xxxxxxxxxx>
> ---
> 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
