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 <address@hidden>
> ---
>  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: <address@hidden>

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