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

[<peng.hao2@zte.com.cn>]

> 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 })