Re: [Qemu-devel] [RFC v1 14/22] memory: manage coalesced mmio via a MemoryListener

[Avi Kivity <avi@redhat.com>]

On 10/04/2012 04:08 PM, Anthony Liguori wrote:
> Avi Kivity <avi@redhat.com> writes:
> 
>> Instead of calling a global function on coalesced mmio changes, which
>> routes the call to kvm if enabled, add coalesced mmio hooks to
>> MemoryListener and make kvm use that instead.
>>
>> The motivation is support for multiple address spaces (which means we
>> we need to filter the call on the right address space) but the result
>> is cleaner as well.
>>
>> -int kvm_coalesce_mmio_region(target_phys_addr_t start, ram_addr_t size)
>> +static void kvm_coalesce_mmio_region(MemoryListener *listener,
>> +                                     MemoryRegionSection *secion,
>> +                                     target_phys_addr_t start, ram_addr_t size)
>>  {
>> -    int ret = -ENOSYS;
>>      KVMState *s = kvm_state;
>>  
>>      if (s->coalesced_mmio) {
>> @@ -466,15 +467,14 @@ int kvm_coalesce_mmio_region(target_phys_addr_t start, ram_addr_t size)
>>          zone.size = size;
>>          zone.pad = 0;
>>  
>> -        ret = kvm_vm_ioctl(s, KVM_REGISTER_COALESCED_MMIO, &zone);
>> +        (void)kvm_vm_ioctl(s, KVM_REGISTER_COALESCED_MMIO, &zone);
> 
> g_assert on error instead of ignoring it.

I'll do that in a separate patch, since the existing behaviour is to
ignore the error.  If errors really do happen, I don't want a
refactoring patch to trigger them.

>>  
>> diff --git a/memory.c b/memory.c
>> index efefcb8..eb75349 100644
>> --- a/memory.c
>> +++ b/memory.c
>> @@ -1130,11 +1130,19 @@ static void memory_region_update_coalesced_range_as(MemoryRegion *mr, AddressSpa
>>      FlatRange *fr;
>>      CoalescedMemoryRange *cmr;
>>      AddrRange tmp;
>> +    MemoryRegionSection section;
>>  
>>      FOR_EACH_FLAT_RANGE(fr, as->current_map) {
>>          if (fr->mr == mr) {
>> -            qemu_unregister_coalesced_mmio(int128_get64(fr->addr.start),
>> -                                           int128_get64(fr->addr.size));
>> +            section = (MemoryRegionSection) {
>> +                .address_space = as->root,
>> +                .offset_within_address_space = int128_get64(fr->addr.start),
>> +                .size = int128_get64(fr->addr.size),
>> +            };
> 
> I think this is a bit too clever.  You can move the definition of
> section into this block with a zero initializer, and then just set the
> fields directly.  You'll end up losing 2 lines of code in the process
> too.

I happen to like it (I dislike repeating the variable name each line)
but have no problem changing it.


-- 
error compiling committee.c: too many arguments to function