[qemu,v2] hmp: Print if memory section is registered with an accelerator

This adds an accelerator name to the "into mtree -f" to tell the user if
a particular memory section is registered with the accelerator;
the primary user for this is KVM and such information is useful
for debugging purposes.

This adds a has_memory() callback to the accelerator class allowing any
accelerator to have a label in that memory tree dump.

Since memory sections are passed to memory listeners and get registered
in accelerators (rather than memory regions), this only prints new labels
for flatviews attached to the system address space.

An example:
 Root memory region: system
  0000000000000000-0000002fffffffff (prio 0, ram): /objects/mem0 kvm
  0000003000000000-0000005fffffffff (prio 0, ram): /objects/mem1 kvm
  0000200000000020-000020000000003f (prio 1, i/o): virtio-pci
  0000200080000000-000020008000003f (prio 0, i/o): capabilities

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---

This supercedes "[PATCH qemu] hmp: Print if memory section is registered in KVM"

---
Changes:
v2:
* added an accelerator callback instead of hardcoding it to kvm only
---
 include/sysemu/accel.h |  2 ++
 accel/kvm/kvm-all.c    | 10 ++++++++++
 memory.c               | 22 ++++++++++++++++++++++
 3 files changed, 34 insertions(+)

Hi Alexey,

On 12/14/18 3:58 AM, Alexey Kardashevskiy wrote:
> This adds an accelerator name to the "into mtree -f" to tell the user if
> a particular memory section is registered with the accelerator;
> the primary user for this is KVM and such information is useful
> for debugging purposes.
> 
> This adds a has_memory() callback to the accelerator class allowing any
> accelerator to have a label in that memory tree dump.
> 
> Since memory sections are passed to memory listeners and get registered
> in accelerators (rather than memory regions), this only prints new labels
> for flatviews attached to the system address space.
> 
> An example:
>  Root memory region: system
>   0000000000000000-0000002fffffffff (prio 0, ram): /objects/mem0 kvm
>   0000003000000000-0000005fffffffff (prio 0, ram): /objects/mem1 kvm
>   0000200000000020-000020000000003f (prio 1, i/o): virtio-pci
>   0000200080000000-000020008000003f (prio 0, i/o): capabilities
> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
> 
> This supercedes "[PATCH qemu] hmp: Print if memory section is registered in KVM"
> 
> ---
> Changes:
> v2:
> * added an accelerator callback instead of hardcoding it to kvm only
> ---
>  include/sysemu/accel.h |  2 ++
>  accel/kvm/kvm-all.c    | 10 ++++++++++
>  memory.c               | 22 ++++++++++++++++++++++
>  3 files changed, 34 insertions(+)
> 
> diff --git a/include/sysemu/accel.h b/include/sysemu/accel.h
> index 637358f..30b456d 100644
> --- a/include/sysemu/accel.h
> +++ b/include/sysemu/accel.h
> @@ -25,6 +25,7 @@
>  
>  #include "qom/object.h"
>  #include "hw/qdev-properties.h"
> +#include "exec/hwaddr.h"
>  
>  typedef struct AccelState {
>      /*< private >*/
> @@ -41,6 +42,7 @@ typedef struct AccelClass {
>      int (*available)(void);
>      int (*init_machine)(MachineState *ms);
>      void (*setup_post)(MachineState *ms, AccelState *accel);
> +    bool (*has_memory)(MachineState *ms, hwaddr start_addr, hwaddr size);
>      bool *allowed;
>      /*
>       * Array of global properties that would be applied when specific
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index 4880a05..634f386 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -2589,11 +2589,21 @@ int kvm_get_one_reg(CPUState *cs, uint64_t id, void *target)
>      return r;
>  }
>  
> +static bool kvm_accel_has_memory(MachineState *ms, hwaddr start_addr,
> +                                 hwaddr size)
> +{
> +    KVMState *kvm = KVM_STATE(ms->accelerator);
> +    KVMMemoryListener *kml = &kvm->memory_listener;
> +
> +    return NULL != kvm_lookup_matching_slot(kml, start_addr, size);
> +}
> +
>  static void kvm_accel_class_init(ObjectClass *oc, void *data)
>  {
>      AccelClass *ac = ACCEL_CLASS(oc);
>      ac->name = "KVM";
>      ac->init_machine = kvm_init;
> +    ac->has_memory = kvm_accel_has_memory;
>      ac->allowed = &kvm_allowed;
>  }
>  
> diff --git a/memory.c b/memory.c
> index d14c6de..61e758a 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -29,7 +29,9 @@
>  #include "exec/ram_addr.h"
>  #include "sysemu/kvm.h"
>  #include "sysemu/sysemu.h"
> +#include "sysemu/accel.h"
>  #include "hw/qdev-properties.h"
> +#include "hw/boards.h"
>  #include "migration/vmstate.h"
>  
>  //#define DEBUG_UNASSIGNED
> @@ -2924,6 +2926,8 @@ struct FlatViewInfo {
>      int counter;
>      bool dispatch_tree;
>      bool owner;
> +    AccelClass *ac;
> +    const char *ac_name;
>  };
>  
>  static void mtree_print_flatview(gpointer key, gpointer value,
> @@ -2939,6 +2943,7 @@ static void mtree_print_flatview(gpointer key, gpointer value,
>      int n = view->nr;
>      int i;
>      AddressSpace *as;
> +    bool system_as = false;
>  
>      p(f, "FlatView #%d\n", fvi->counter);
>      ++fvi->counter;
> @@ -2950,6 +2955,9 @@ static void mtree_print_flatview(gpointer key, gpointer value,
>              p(f, ", alias %s", memory_region_name(as->root->alias));
>          }
>          p(f, "\n");
> +        if (as == &address_space_memory) {
> +            system_as = true;
> +        }
>      }
>  
>      p(f, " Root memory region: %s\n",
> @@ -2985,6 +2993,13 @@ static void mtree_print_flatview(gpointer key, gpointer value,
>          if (fvi->owner) {
>              mtree_print_mr_owner(p, f, mr);
>          }
> +
> +        if (system_as && fvi->ac &&
> +            fvi->ac->has_memory(current_machine,
> +                                int128_get64(range->addr.start),
> +                                MR_SIZE(range->addr.size) + 1)) {
> +            p(f, " %s", fvi->ac_name);

Why not simply display fvi->ac->name?
You could avoid to add the ac_name field.

> +        }
>          p(f, "\n");
>          range++;
>      }
> @@ -3028,6 +3043,13 @@ void mtree_info(fprintf_function mon_printf, void *f, bool flatview,
>          };
>          GArray *fv_address_spaces;
>          GHashTable *views = g_hash_table_new(g_direct_hash, g_direct_equal);
> +        AccelClass *ac = ACCEL_GET_CLASS(current_machine->accelerator);
> +
> +        if (ac->has_memory) {
> +            fvi.ac = ac;
> +            fvi.ac_name = current_machine->accel ? current_machine->accel :
> +                object_class_get_name(OBJECT_CLASS(ac));
> +        }
>  
>          /* Gather all FVs in one table */
>          QTAILQ_FOREACH(as, &address_spaces, address_spaces_link) {
>

On 14/12/2018 22:07, Philippe Mathieu-Daudé wrote:
> Hi Alexey,
> 
> On 12/14/18 3:58 AM, Alexey Kardashevskiy wrote:
>> This adds an accelerator name to the "into mtree -f" to tell the user if
>> a particular memory section is registered with the accelerator;
>> the primary user for this is KVM and such information is useful
>> for debugging purposes.
>>
>> This adds a has_memory() callback to the accelerator class allowing any
>> accelerator to have a label in that memory tree dump.
>>
>> Since memory sections are passed to memory listeners and get registered
>> in accelerators (rather than memory regions), this only prints new labels
>> for flatviews attached to the system address space.
>>
>> An example:
>>  Root memory region: system
>>   0000000000000000-0000002fffffffff (prio 0, ram): /objects/mem0 kvm
>>   0000003000000000-0000005fffffffff (prio 0, ram): /objects/mem1 kvm
>>   0000200000000020-000020000000003f (prio 1, i/o): virtio-pci
>>   0000200080000000-000020008000003f (prio 0, i/o): capabilities
>>
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> ---
>>
>> This supercedes "[PATCH qemu] hmp: Print if memory section is registered in KVM"
>>
>> ---
>> Changes:
>> v2:
>> * added an accelerator callback instead of hardcoding it to kvm only
>> ---
>>  include/sysemu/accel.h |  2 ++
>>  accel/kvm/kvm-all.c    | 10 ++++++++++
>>  memory.c               | 22 ++++++++++++++++++++++
>>  3 files changed, 34 insertions(+)
>>
>> diff --git a/include/sysemu/accel.h b/include/sysemu/accel.h
>> index 637358f..30b456d 100644
>> --- a/include/sysemu/accel.h
>> +++ b/include/sysemu/accel.h
>> @@ -25,6 +25,7 @@
>>  
>>  #include "qom/object.h"
>>  #include "hw/qdev-properties.h"
>> +#include "exec/hwaddr.h"
>>  
>>  typedef struct AccelState {
>>      /*< private >*/
>> @@ -41,6 +42,7 @@ typedef struct AccelClass {
>>      int (*available)(void);
>>      int (*init_machine)(MachineState *ms);
>>      void (*setup_post)(MachineState *ms, AccelState *accel);
>> +    bool (*has_memory)(MachineState *ms, hwaddr start_addr, hwaddr size);
>>      bool *allowed;
>>      /*
>>       * Array of global properties that would be applied when specific
>> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
>> index 4880a05..634f386 100644
>> --- a/accel/kvm/kvm-all.c
>> +++ b/accel/kvm/kvm-all.c
>> @@ -2589,11 +2589,21 @@ int kvm_get_one_reg(CPUState *cs, uint64_t id, void *target)
>>      return r;
>>  }
>>  
>> +static bool kvm_accel_has_memory(MachineState *ms, hwaddr start_addr,
>> +                                 hwaddr size)
>> +{
>> +    KVMState *kvm = KVM_STATE(ms->accelerator);
>> +    KVMMemoryListener *kml = &kvm->memory_listener;
>> +
>> +    return NULL != kvm_lookup_matching_slot(kml, start_addr, size);
>> +}
>> +
>>  static void kvm_accel_class_init(ObjectClass *oc, void *data)
>>  {
>>      AccelClass *ac = ACCEL_CLASS(oc);
>>      ac->name = "KVM";
>>      ac->init_machine = kvm_init;
>> +    ac->has_memory = kvm_accel_has_memory;
>>      ac->allowed = &kvm_allowed;
>>  }
>>  
>> diff --git a/memory.c b/memory.c
>> index d14c6de..61e758a 100644
>> --- a/memory.c
>> +++ b/memory.c
>> @@ -29,7 +29,9 @@
>>  #include "exec/ram_addr.h"
>>  #include "sysemu/kvm.h"
>>  #include "sysemu/sysemu.h"
>> +#include "sysemu/accel.h"
>>  #include "hw/qdev-properties.h"
>> +#include "hw/boards.h"
>>  #include "migration/vmstate.h"
>>  
>>  //#define DEBUG_UNASSIGNED
>> @@ -2924,6 +2926,8 @@ struct FlatViewInfo {
>>      int counter;
>>      bool dispatch_tree;
>>      bool owner;
>> +    AccelClass *ac;
>> +    const char *ac_name;
>>  };
>>  
>>  static void mtree_print_flatview(gpointer key, gpointer value,
>> @@ -2939,6 +2943,7 @@ static void mtree_print_flatview(gpointer key, gpointer value,
>>      int n = view->nr;
>>      int i;
>>      AddressSpace *as;
>> +    bool system_as = false;
>>  
>>      p(f, "FlatView #%d\n", fvi->counter);
>>      ++fvi->counter;
>> @@ -2950,6 +2955,9 @@ static void mtree_print_flatview(gpointer key, gpointer value,
>>              p(f, ", alias %s", memory_region_name(as->root->alias));
>>          }
>>          p(f, "\n");
>> +        if (as == &address_space_memory) {
>> +            system_as = true;
>> +        }
>>      }
>>  
>>      p(f, " Root memory region: %s\n",
>> @@ -2985,6 +2993,13 @@ static void mtree_print_flatview(gpointer key, gpointer value,
>>          if (fvi->owner) {
>>              mtree_print_mr_owner(p, f, mr);
>>          }
>> +
>> +        if (system_as && fvi->ac &&
>> +            fvi->ac->has_memory(current_machine,
>> +                                int128_get64(range->addr.start),
>> +                                MR_SIZE(range->addr.size) + 1)) {
>> +            p(f, " %s", fvi->ac_name);
> 
> Why not simply display fvi->ac->name?
> You could avoid to add the ac_name field.


Well, I thought I better print whatever the user passed via the command
line (which is current_machine->accel and equals to "kvm" in my case)
rather than robotic, dry and excessive "kvm-accel".



> 
>> +        }
>>          p(f, "\n");
>>          range++;
>>      }
>> @@ -3028,6 +3043,13 @@ void mtree_info(fprintf_function mon_printf, void *f, bool flatview,
>>          };
>>          GArray *fv_address_spaces;
>>          GHashTable *views = g_hash_table_new(g_direct_hash, g_direct_equal);
>> +        AccelClass *ac = ACCEL_GET_CLASS(current_machine->accelerator);
>> +
>> +        if (ac->has_memory) {
>> +            fvi.ac = ac;
>> +            fvi.ac_name = current_machine->accel ? current_machine->accel :
>> +                object_class_get_name(OBJECT_CLASS(ac));
>> +        }
>>  
>>          /* Gather all FVs in one table */
>>          QTAILQ_FOREACH(as, &address_spaces, address_spaces_link) {
>>

On 12/17/18 2:27 AM, Alexey Kardashevskiy wrote:
> On 14/12/2018 22:07, Philippe Mathieu-Daudé wrote:
>> Hi Alexey,
>>
>> On 12/14/18 3:58 AM, Alexey Kardashevskiy wrote:
>>> This adds an accelerator name to the "into mtree -f" to tell the user if
>>> a particular memory section is registered with the accelerator;
>>> the primary user for this is KVM and such information is useful
>>> for debugging purposes.
>>>
>>> This adds a has_memory() callback to the accelerator class allowing any
>>> accelerator to have a label in that memory tree dump.
>>>
>>> Since memory sections are passed to memory listeners and get registered
>>> in accelerators (rather than memory regions), this only prints new labels
>>> for flatviews attached to the system address space.
>>>
>>> An example:
>>>  Root memory region: system
>>>   0000000000000000-0000002fffffffff (prio 0, ram): /objects/mem0 kvm
>>>   0000003000000000-0000005fffffffff (prio 0, ram): /objects/mem1 kvm
>>>   0000200000000020-000020000000003f (prio 1, i/o): virtio-pci
>>>   0000200080000000-000020008000003f (prio 0, i/o): capabilities
>>>
>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>> ---
>>>
>>> This supercedes "[PATCH qemu] hmp: Print if memory section is registered in KVM"
>>>
>>> ---
>>> Changes:
>>> v2:
>>> * added an accelerator callback instead of hardcoding it to kvm only
>>> ---
>>>  include/sysemu/accel.h |  2 ++
>>>  accel/kvm/kvm-all.c    | 10 ++++++++++
>>>  memory.c               | 22 ++++++++++++++++++++++
>>>  3 files changed, 34 insertions(+)
>>>
>>> diff --git a/include/sysemu/accel.h b/include/sysemu/accel.h
>>> index 637358f..30b456d 100644
>>> --- a/include/sysemu/accel.h
>>> +++ b/include/sysemu/accel.h
>>> @@ -25,6 +25,7 @@
>>>  
>>>  #include "qom/object.h"
>>>  #include "hw/qdev-properties.h"
>>> +#include "exec/hwaddr.h"
>>>  
>>>  typedef struct AccelState {
>>>      /*< private >*/
>>> @@ -41,6 +42,7 @@ typedef struct AccelClass {
>>>      int (*available)(void);
>>>      int (*init_machine)(MachineState *ms);
>>>      void (*setup_post)(MachineState *ms, AccelState *accel);
>>> +    bool (*has_memory)(MachineState *ms, hwaddr start_addr, hwaddr size);
>>>      bool *allowed;
>>>      /*
>>>       * Array of global properties that would be applied when specific
>>> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
>>> index 4880a05..634f386 100644
>>> --- a/accel/kvm/kvm-all.c
>>> +++ b/accel/kvm/kvm-all.c
>>> @@ -2589,11 +2589,21 @@ int kvm_get_one_reg(CPUState *cs, uint64_t id, void *target)
>>>      return r;
>>>  }
>>>  
>>> +static bool kvm_accel_has_memory(MachineState *ms, hwaddr start_addr,
>>> +                                 hwaddr size)
>>> +{
>>> +    KVMState *kvm = KVM_STATE(ms->accelerator);
>>> +    KVMMemoryListener *kml = &kvm->memory_listener;
>>> +
>>> +    return NULL != kvm_lookup_matching_slot(kml, start_addr, size);
>>> +}
>>> +
>>>  static void kvm_accel_class_init(ObjectClass *oc, void *data)
>>>  {
>>>      AccelClass *ac = ACCEL_CLASS(oc);
>>>      ac->name = "KVM";
>>>      ac->init_machine = kvm_init;
>>> +    ac->has_memory = kvm_accel_has_memory;
>>>      ac->allowed = &kvm_allowed;
>>>  }
>>>  
>>> diff --git a/memory.c b/memory.c
>>> index d14c6de..61e758a 100644
>>> --- a/memory.c
>>> +++ b/memory.c
>>> @@ -29,7 +29,9 @@
>>>  #include "exec/ram_addr.h"
>>>  #include "sysemu/kvm.h"
>>>  #include "sysemu/sysemu.h"
>>> +#include "sysemu/accel.h"
>>>  #include "hw/qdev-properties.h"
>>> +#include "hw/boards.h"
>>>  #include "migration/vmstate.h"
>>>  
>>>  //#define DEBUG_UNASSIGNED
>>> @@ -2924,6 +2926,8 @@ struct FlatViewInfo {
>>>      int counter;
>>>      bool dispatch_tree;
>>>      bool owner;
>>> +    AccelClass *ac;
>>> +    const char *ac_name;
>>>  };
>>>  
>>>  static void mtree_print_flatview(gpointer key, gpointer value,
>>> @@ -2939,6 +2943,7 @@ static void mtree_print_flatview(gpointer key, gpointer value,
>>>      int n = view->nr;
>>>      int i;
>>>      AddressSpace *as;
>>> +    bool system_as = false;
>>>  
>>>      p(f, "FlatView #%d\n", fvi->counter);
>>>      ++fvi->counter;
>>> @@ -2950,6 +2955,9 @@ static void mtree_print_flatview(gpointer key, gpointer value,
>>>              p(f, ", alias %s", memory_region_name(as->root->alias));
>>>          }
>>>          p(f, "\n");
>>> +        if (as == &address_space_memory) {
>>> +            system_as = true;
>>> +        }
>>>      }
>>>  
>>>      p(f, " Root memory region: %s\n",
>>> @@ -2985,6 +2993,13 @@ static void mtree_print_flatview(gpointer key, gpointer value,
>>>          if (fvi->owner) {
>>>              mtree_print_mr_owner(p, f, mr);
>>>          }
>>> +
>>> +        if (system_as && fvi->ac &&
>>> +            fvi->ac->has_memory(current_machine,
>>> +                                int128_get64(range->addr.start),
>>> +                                MR_SIZE(range->addr.size) + 1)) {
>>> +            p(f, " %s", fvi->ac_name);
>>
>> Why not simply display fvi->ac->name?
>> You could avoid to add the ac_name field.
> 
> 
> Well, I thought I better print whatever the user passed via the command
> line (which is current_machine->accel and equals to "kvm" in my case)
> rather than robotic, dry and excessive "kvm-accel".

I have no hit for 'kvm-accel':

$ git grep kvm-accel
$

Names looks human friendly:

$ git grep 'ac->name ='
accel/kvm/kvm-all.c:2595:    ac->name = "KVM";
accel/tcg/tcg-all.c:74:    ac->name = "tcg";
hw/xen/xen-common.c:184:    ac->name = "Xen";
target/i386/hax-all.c:1088:    ac->name = "HAX";
target/i386/hvf/hvf.c:959:    ac->name = "HVF";
target/i386/whpx-all.c:1477:    ac->name = "WHPX";
qtest.c:755:    ac->name = "QTest";

>>
>>> +        }
>>>          p(f, "\n");
>>>          range++;
>>>      }
>>> @@ -3028,6 +3043,13 @@ void mtree_info(fprintf_function mon_printf, void *f, bool flatview,
>>>          };
>>>          GArray *fv_address_spaces;
>>>          GHashTable *views = g_hash_table_new(g_direct_hash, g_direct_equal);
>>> +        AccelClass *ac = ACCEL_GET_CLASS(current_machine->accelerator);
>>> +
>>> +        if (ac->has_memory) {
>>> +            fvi.ac = ac;
>>> +            fvi.ac_name = current_machine->accel ? current_machine->accel :
>>> +                object_class_get_name(OBJECT_CLASS(ac));
>>> +        }
>>>  
>>>          /* Gather all FVs in one table */
>>>          QTAILQ_FOREACH(as, &address_spaces, address_spaces_link) {
>>>

On 17/12/2018 23:47, Philippe Mathieu-Daudé wrote:
> On 12/17/18 2:27 AM, Alexey Kardashevskiy wrote:
>> On 14/12/2018 22:07, Philippe Mathieu-Daudé wrote:
>>> Hi Alexey,
>>>
>>> On 12/14/18 3:58 AM, Alexey Kardashevskiy wrote:
>>>> This adds an accelerator name to the "into mtree -f" to tell the user if
>>>> a particular memory section is registered with the accelerator;
>>>> the primary user for this is KVM and such information is useful
>>>> for debugging purposes.
>>>>
>>>> This adds a has_memory() callback to the accelerator class allowing any
>>>> accelerator to have a label in that memory tree dump.
>>>>
>>>> Since memory sections are passed to memory listeners and get registered
>>>> in accelerators (rather than memory regions), this only prints new labels
>>>> for flatviews attached to the system address space.
>>>>
>>>> An example:
>>>>  Root memory region: system
>>>>   0000000000000000-0000002fffffffff (prio 0, ram): /objects/mem0 kvm
>>>>   0000003000000000-0000005fffffffff (prio 0, ram): /objects/mem1 kvm
>>>>   0000200000000020-000020000000003f (prio 1, i/o): virtio-pci
>>>>   0000200080000000-000020008000003f (prio 0, i/o): capabilities
>>>>
>>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>>> ---
>>>>
>>>> This supercedes "[PATCH qemu] hmp: Print if memory section is registered in KVM"
>>>>
>>>> ---
>>>> Changes:
>>>> v2:
>>>> * added an accelerator callback instead of hardcoding it to kvm only
>>>> ---
>>>>  include/sysemu/accel.h |  2 ++
>>>>  accel/kvm/kvm-all.c    | 10 ++++++++++
>>>>  memory.c               | 22 ++++++++++++++++++++++
>>>>  3 files changed, 34 insertions(+)
>>>>
>>>> diff --git a/include/sysemu/accel.h b/include/sysemu/accel.h
>>>> index 637358f..30b456d 100644
>>>> --- a/include/sysemu/accel.h
>>>> +++ b/include/sysemu/accel.h
>>>> @@ -25,6 +25,7 @@
>>>>  
>>>>  #include "qom/object.h"
>>>>  #include "hw/qdev-properties.h"
>>>> +#include "exec/hwaddr.h"
>>>>  
>>>>  typedef struct AccelState {
>>>>      /*< private >*/
>>>> @@ -41,6 +42,7 @@ typedef struct AccelClass {
>>>>      int (*available)(void);
>>>>      int (*init_machine)(MachineState *ms);
>>>>      void (*setup_post)(MachineState *ms, AccelState *accel);
>>>> +    bool (*has_memory)(MachineState *ms, hwaddr start_addr, hwaddr size);
>>>>      bool *allowed;
>>>>      /*
>>>>       * Array of global properties that would be applied when specific
>>>> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
>>>> index 4880a05..634f386 100644
>>>> --- a/accel/kvm/kvm-all.c
>>>> +++ b/accel/kvm/kvm-all.c
>>>> @@ -2589,11 +2589,21 @@ int kvm_get_one_reg(CPUState *cs, uint64_t id, void *target)
>>>>      return r;
>>>>  }
>>>>  
>>>> +static bool kvm_accel_has_memory(MachineState *ms, hwaddr start_addr,
>>>> +                                 hwaddr size)
>>>> +{
>>>> +    KVMState *kvm = KVM_STATE(ms->accelerator);
>>>> +    KVMMemoryListener *kml = &kvm->memory_listener;
>>>> +
>>>> +    return NULL != kvm_lookup_matching_slot(kml, start_addr, size);
>>>> +}
>>>> +
>>>>  static void kvm_accel_class_init(ObjectClass *oc, void *data)
>>>>  {
>>>>      AccelClass *ac = ACCEL_CLASS(oc);
>>>>      ac->name = "KVM";
>>>>      ac->init_machine = kvm_init;
>>>> +    ac->has_memory = kvm_accel_has_memory;
>>>>      ac->allowed = &kvm_allowed;
>>>>  }
>>>>  
>>>> diff --git a/memory.c b/memory.c
>>>> index d14c6de..61e758a 100644
>>>> --- a/memory.c
>>>> +++ b/memory.c
>>>> @@ -29,7 +29,9 @@
>>>>  #include "exec/ram_addr.h"
>>>>  #include "sysemu/kvm.h"
>>>>  #include "sysemu/sysemu.h"
>>>> +#include "sysemu/accel.h"
>>>>  #include "hw/qdev-properties.h"
>>>> +#include "hw/boards.h"
>>>>  #include "migration/vmstate.h"
>>>>  
>>>>  //#define DEBUG_UNASSIGNED
>>>> @@ -2924,6 +2926,8 @@ struct FlatViewInfo {
>>>>      int counter;
>>>>      bool dispatch_tree;
>>>>      bool owner;
>>>> +    AccelClass *ac;
>>>> +    const char *ac_name;
>>>>  };
>>>>  
>>>>  static void mtree_print_flatview(gpointer key, gpointer value,
>>>> @@ -2939,6 +2943,7 @@ static void mtree_print_flatview(gpointer key, gpointer value,
>>>>      int n = view->nr;
>>>>      int i;
>>>>      AddressSpace *as;
>>>> +    bool system_as = false;
>>>>  
>>>>      p(f, "FlatView #%d\n", fvi->counter);
>>>>      ++fvi->counter;
>>>> @@ -2950,6 +2955,9 @@ static void mtree_print_flatview(gpointer key, gpointer value,
>>>>              p(f, ", alias %s", memory_region_name(as->root->alias));
>>>>          }
>>>>          p(f, "\n");
>>>> +        if (as == &address_space_memory) {
>>>> +            system_as = true;
>>>> +        }
>>>>      }
>>>>  
>>>>      p(f, " Root memory region: %s\n",
>>>> @@ -2985,6 +2993,13 @@ static void mtree_print_flatview(gpointer key, gpointer value,
>>>>          if (fvi->owner) {
>>>>              mtree_print_mr_owner(p, f, mr);
>>>>          }
>>>> +
>>>> +        if (system_as && fvi->ac &&
>>>> +            fvi->ac->has_memory(current_machine,
>>>> +                                int128_get64(range->addr.start),
>>>> +                                MR_SIZE(range->addr.size) + 1)) {
>>>> +            p(f, " %s", fvi->ac_name);
>>>
>>> Why not simply display fvi->ac->name?
>>> You could avoid to add the ac_name field.
>>
>>
>> Well, I thought I better print whatever the user passed via the command
>> line (which is current_machine->accel and equals to "kvm" in my case)
>> rather than robotic, dry and excessive "kvm-accel".
> 
> I have no hit for 'kvm-accel':
> 
> $ git grep kvm-accel
> $
> 
> Names looks human friendly:


Ah, I confused with object_class_get_name(). Anyway, I'd still show the
user provided accelerator name than some internal name.


> 
> $ git grep 'ac->name ='
> accel/kvm/kvm-all.c:2595:    ac->name = "KVM";
> accel/tcg/tcg-all.c:74:    ac->name = "tcg";
> hw/xen/xen-common.c:184:    ac->name = "Xen";
> target/i386/hax-all.c:1088:    ac->name = "HAX";
> target/i386/hvf/hvf.c:959:    ac->name = "HVF";
> target/i386/whpx-all.c:1477:    ac->name = "WHPX";
> qtest.c:755:    ac->name = "QTest";
> 
>>>
>>>> +        }
>>>>          p(f, "\n");
>>>>          range++;
>>>>      }
>>>> @@ -3028,6 +3043,13 @@ void mtree_info(fprintf_function mon_printf, void *f, bool flatview,
>>>>          };
>>>>          GArray *fv_address_spaces;
>>>>          GHashTable *views = g_hash_table_new(g_direct_hash, g_direct_equal);
>>>> +        AccelClass *ac = ACCEL_GET_CLASS(current_machine->accelerator);
>>>> +
>>>> +        if (ac->has_memory) {
>>>> +            fvi.ac = ac;
>>>> +            fvi.ac_name = current_machine->accel ? current_machine->accel :
>>>> +                object_class_get_name(OBJECT_CLASS(ac));
>>>> +        }
>>>>  
>>>>          /* Gather all FVs in one table */
>>>>          QTAILQ_FOREACH(as, &address_spaces, address_spaces_link) {
>>>>

* Alexey Kardashevskiy (aik@ozlabs.ru) wrote:
> 
> 
> On 17/12/2018 23:47, Philippe Mathieu-Daudé wrote:
> > On 12/17/18 2:27 AM, Alexey Kardashevskiy wrote:
> >> On 14/12/2018 22:07, Philippe Mathieu-Daudé wrote:
> >>> Hi Alexey,
> >>>
> >>> On 12/14/18 3:58 AM, Alexey Kardashevskiy wrote:
> >>>> This adds an accelerator name to the "into mtree -f" to tell the user if
> >>>> a particular memory section is registered with the accelerator;
> >>>> the primary user for this is KVM and such information is useful
> >>>> for debugging purposes.
> >>>>
> >>>> This adds a has_memory() callback to the accelerator class allowing any
> >>>> accelerator to have a label in that memory tree dump.
> >>>>
> >>>> Since memory sections are passed to memory listeners and get registered
> >>>> in accelerators (rather than memory regions), this only prints new labels
> >>>> for flatviews attached to the system address space.
> >>>>
> >>>> An example:
> >>>>  Root memory region: system
> >>>>   0000000000000000-0000002fffffffff (prio 0, ram): /objects/mem0 kvm
> >>>>   0000003000000000-0000005fffffffff (prio 0, ram): /objects/mem1 kvm
> >>>>   0000200000000020-000020000000003f (prio 1, i/o): virtio-pci
> >>>>   0000200080000000-000020008000003f (prio 0, i/o): capabilities
> >>>>
> >>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> >>>> ---
> >>>>
> >>>> This supercedes "[PATCH qemu] hmp: Print if memory section is registered in KVM"
> >>>>
> >>>> ---
> >>>> Changes:
> >>>> v2:
> >>>> * added an accelerator callback instead of hardcoding it to kvm only
> >>>> ---
> >>>>  include/sysemu/accel.h |  2 ++
> >>>>  accel/kvm/kvm-all.c    | 10 ++++++++++
> >>>>  memory.c               | 22 ++++++++++++++++++++++
> >>>>  3 files changed, 34 insertions(+)
> >>>>
> >>>> diff --git a/include/sysemu/accel.h b/include/sysemu/accel.h
> >>>> index 637358f..30b456d 100644
> >>>> --- a/include/sysemu/accel.h
> >>>> +++ b/include/sysemu/accel.h
> >>>> @@ -25,6 +25,7 @@
> >>>>  
> >>>>  #include "qom/object.h"
> >>>>  #include "hw/qdev-properties.h"
> >>>> +#include "exec/hwaddr.h"
> >>>>  
> >>>>  typedef struct AccelState {
> >>>>      /*< private >*/
> >>>> @@ -41,6 +42,7 @@ typedef struct AccelClass {
> >>>>      int (*available)(void);
> >>>>      int (*init_machine)(MachineState *ms);
> >>>>      void (*setup_post)(MachineState *ms, AccelState *accel);
> >>>> +    bool (*has_memory)(MachineState *ms, hwaddr start_addr, hwaddr size);
> >>>>      bool *allowed;
> >>>>      /*
> >>>>       * Array of global properties that would be applied when specific
> >>>> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> >>>> index 4880a05..634f386 100644
> >>>> --- a/accel/kvm/kvm-all.c
> >>>> +++ b/accel/kvm/kvm-all.c
> >>>> @@ -2589,11 +2589,21 @@ int kvm_get_one_reg(CPUState *cs, uint64_t id, void *target)
> >>>>      return r;
> >>>>  }
> >>>>  
> >>>> +static bool kvm_accel_has_memory(MachineState *ms, hwaddr start_addr,
> >>>> +                                 hwaddr size)
> >>>> +{
> >>>> +    KVMState *kvm = KVM_STATE(ms->accelerator);
> >>>> +    KVMMemoryListener *kml = &kvm->memory_listener;
> >>>> +
> >>>> +    return NULL != kvm_lookup_matching_slot(kml, start_addr, size);
> >>>> +}
> >>>> +
> >>>>  static void kvm_accel_class_init(ObjectClass *oc, void *data)
> >>>>  {
> >>>>      AccelClass *ac = ACCEL_CLASS(oc);
> >>>>      ac->name = "KVM";
> >>>>      ac->init_machine = kvm_init;
> >>>> +    ac->has_memory = kvm_accel_has_memory;
> >>>>      ac->allowed = &kvm_allowed;
> >>>>  }
> >>>>  
> >>>> diff --git a/memory.c b/memory.c
> >>>> index d14c6de..61e758a 100644
> >>>> --- a/memory.c
> >>>> +++ b/memory.c
> >>>> @@ -29,7 +29,9 @@
> >>>>  #include "exec/ram_addr.h"
> >>>>  #include "sysemu/kvm.h"
> >>>>  #include "sysemu/sysemu.h"
> >>>> +#include "sysemu/accel.h"
> >>>>  #include "hw/qdev-properties.h"
> >>>> +#include "hw/boards.h"
> >>>>  #include "migration/vmstate.h"
> >>>>  
> >>>>  //#define DEBUG_UNASSIGNED
> >>>> @@ -2924,6 +2926,8 @@ struct FlatViewInfo {
> >>>>      int counter;
> >>>>      bool dispatch_tree;
> >>>>      bool owner;
> >>>> +    AccelClass *ac;
> >>>> +    const char *ac_name;
> >>>>  };
> >>>>  
> >>>>  static void mtree_print_flatview(gpointer key, gpointer value,
> >>>> @@ -2939,6 +2943,7 @@ static void mtree_print_flatview(gpointer key, gpointer value,
> >>>>      int n = view->nr;
> >>>>      int i;
> >>>>      AddressSpace *as;
> >>>> +    bool system_as = false;
> >>>>  
> >>>>      p(f, "FlatView #%d\n", fvi->counter);
> >>>>      ++fvi->counter;
> >>>> @@ -2950,6 +2955,9 @@ static void mtree_print_flatview(gpointer key, gpointer value,
> >>>>              p(f, ", alias %s", memory_region_name(as->root->alias));
> >>>>          }
> >>>>          p(f, "\n");
> >>>> +        if (as == &address_space_memory) {
> >>>> +            system_as = true;
> >>>> +        }
> >>>>      }
> >>>>  
> >>>>      p(f, " Root memory region: %s\n",
> >>>> @@ -2985,6 +2993,13 @@ static void mtree_print_flatview(gpointer key, gpointer value,
> >>>>          if (fvi->owner) {
> >>>>              mtree_print_mr_owner(p, f, mr);
> >>>>          }
> >>>> +
> >>>> +        if (system_as && fvi->ac &&
> >>>> +            fvi->ac->has_memory(current_machine,
> >>>> +                                int128_get64(range->addr.start),
> >>>> +                                MR_SIZE(range->addr.size) + 1)) {
> >>>> +            p(f, " %s", fvi->ac_name);
> >>>
> >>> Why not simply display fvi->ac->name?
> >>> You could avoid to add the ac_name field.
> >>
> >>
> >> Well, I thought I better print whatever the user passed via the command
> >> line (which is current_machine->accel and equals to "kvm" in my case)
> >> rather than robotic, dry and excessive "kvm-accel".
> > 
> > I have no hit for 'kvm-accel':
> > 
> > $ git grep kvm-accel
> > $
> > 
> > Names looks human friendly:
> 
> 
> Ah, I confused with object_class_get_name(). Anyway, I'd still show the
> user provided accelerator name than some internal name.

Aren't they exactly the same? I think the accel= thing from the user is
parsed by comparing it with the ACCEL_CLASS_NAME declarations:

./target/i386/whpx-all.c:    .name = ACCEL_CLASS_NAME("whpx"),
./target/i386/hax-all.c:    .name = ACCEL_CLASS_NAME("hax"),
./hw/xen/xen-common.c:#define TYPE_XEN_ACCEL ACCEL_CLASS_NAME("xen")
./qtest.c:#define TYPE_QTEST_ACCEL ACCEL_CLASS_NAME("qtest")
./accel/tcg/tcg-all.c:#define TYPE_TCG_ACCEL ACCEL_CLASS_NAME("tcg")
./include/sysemu/hvf.h:#define TYPE_HVF_ACCEL ACCEL_CLASS_NAME("hvf")
./include/sysemu/kvm_int.h:#define TYPE_KVM_ACCEL ACCEL_CLASS_NAME("kvm")

and the code in accel/accel.c accel_find

Dave

> 
> > 
> > $ git grep 'ac->name ='
> > accel/kvm/kvm-all.c:2595:    ac->name = "KVM";
> > accel/tcg/tcg-all.c:74:    ac->name = "tcg";
> > hw/xen/xen-common.c:184:    ac->name = "Xen";
> > target/i386/hax-all.c:1088:    ac->name = "HAX";
> > target/i386/hvf/hvf.c:959:    ac->name = "HVF";
> > target/i386/whpx-all.c:1477:    ac->name = "WHPX";
> > qtest.c:755:    ac->name = "QTest";
> > 
> >>>
> >>>> +        }
> >>>>          p(f, "\n");
> >>>>          range++;
> >>>>      }
> >>>> @@ -3028,6 +3043,13 @@ void mtree_info(fprintf_function mon_printf, void *f, bool flatview,
> >>>>          };
> >>>>          GArray *fv_address_spaces;
> >>>>          GHashTable *views = g_hash_table_new(g_direct_hash, g_direct_equal);
> >>>> +        AccelClass *ac = ACCEL_GET_CLASS(current_machine->accelerator);
> >>>> +
> >>>> +        if (ac->has_memory) {
> >>>> +            fvi.ac = ac;
> >>>> +            fvi.ac_name = current_machine->accel ? current_machine->accel :
> >>>> +                object_class_get_name(OBJECT_CLASS(ac));
> >>>> +        }
> >>>>  
> >>>>          /* Gather all FVs in one table */
> >>>>          QTAILQ_FOREACH(as, &address_spaces, address_spaces_link) {
> >>>>
> 
> -- 
> Alexey
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

On 04/01/2019 04:37, Dr. David Alan Gilbert wrote:
> * Alexey Kardashevskiy (aik@ozlabs.ru) wrote:
>>
>>
>> On 17/12/2018 23:47, Philippe Mathieu-Daudé wrote:
>>> On 12/17/18 2:27 AM, Alexey Kardashevskiy wrote:
>>>> On 14/12/2018 22:07, Philippe Mathieu-Daudé wrote:
>>>>> Hi Alexey,
>>>>>
>>>>> On 12/14/18 3:58 AM, Alexey Kardashevskiy wrote:
>>>>>> This adds an accelerator name to the "into mtree -f" to tell the user if
>>>>>> a particular memory section is registered with the accelerator;
>>>>>> the primary user for this is KVM and such information is useful
>>>>>> for debugging purposes.
>>>>>>
>>>>>> This adds a has_memory() callback to the accelerator class allowing any
>>>>>> accelerator to have a label in that memory tree dump.
>>>>>>
>>>>>> Since memory sections are passed to memory listeners and get registered
>>>>>> in accelerators (rather than memory regions), this only prints new labels
>>>>>> for flatviews attached to the system address space.
>>>>>>
>>>>>> An example:
>>>>>>  Root memory region: system
>>>>>>   0000000000000000-0000002fffffffff (prio 0, ram): /objects/mem0 kvm
>>>>>>   0000003000000000-0000005fffffffff (prio 0, ram): /objects/mem1 kvm
>>>>>>   0000200000000020-000020000000003f (prio 1, i/o): virtio-pci
>>>>>>   0000200080000000-000020008000003f (prio 0, i/o): capabilities
>>>>>>
>>>>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>>>>> ---
>>>>>>
>>>>>> This supercedes "[PATCH qemu] hmp: Print if memory section is registered in KVM"
>>>>>>
>>>>>> ---
>>>>>> Changes:
>>>>>> v2:
>>>>>> * added an accelerator callback instead of hardcoding it to kvm only
>>>>>> ---
>>>>>>  include/sysemu/accel.h |  2 ++
>>>>>>  accel/kvm/kvm-all.c    | 10 ++++++++++
>>>>>>  memory.c               | 22 ++++++++++++++++++++++
>>>>>>  3 files changed, 34 insertions(+)
>>>>>>
>>>>>> diff --git a/include/sysemu/accel.h b/include/sysemu/accel.h
>>>>>> index 637358f..30b456d 100644
>>>>>> --- a/include/sysemu/accel.h
>>>>>> +++ b/include/sysemu/accel.h
>>>>>> @@ -25,6 +25,7 @@
>>>>>>  
>>>>>>  #include "qom/object.h"
>>>>>>  #include "hw/qdev-properties.h"
>>>>>> +#include "exec/hwaddr.h"
>>>>>>  
>>>>>>  typedef struct AccelState {
>>>>>>      /*< private >*/
>>>>>> @@ -41,6 +42,7 @@ typedef struct AccelClass {
>>>>>>      int (*available)(void);
>>>>>>      int (*init_machine)(MachineState *ms);
>>>>>>      void (*setup_post)(MachineState *ms, AccelState *accel);
>>>>>> +    bool (*has_memory)(MachineState *ms, hwaddr start_addr, hwaddr size);
>>>>>>      bool *allowed;
>>>>>>      /*
>>>>>>       * Array of global properties that would be applied when specific
>>>>>> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
>>>>>> index 4880a05..634f386 100644
>>>>>> --- a/accel/kvm/kvm-all.c
>>>>>> +++ b/accel/kvm/kvm-all.c
>>>>>> @@ -2589,11 +2589,21 @@ int kvm_get_one_reg(CPUState *cs, uint64_t id, void *target)
>>>>>>      return r;
>>>>>>  }
>>>>>>  
>>>>>> +static bool kvm_accel_has_memory(MachineState *ms, hwaddr start_addr,
>>>>>> +                                 hwaddr size)
>>>>>> +{
>>>>>> +    KVMState *kvm = KVM_STATE(ms->accelerator);
>>>>>> +    KVMMemoryListener *kml = &kvm->memory_listener;
>>>>>> +
>>>>>> +    return NULL != kvm_lookup_matching_slot(kml, start_addr, size);
>>>>>> +}
>>>>>> +
>>>>>>  static void kvm_accel_class_init(ObjectClass *oc, void *data)
>>>>>>  {
>>>>>>      AccelClass *ac = ACCEL_CLASS(oc);
>>>>>>      ac->name = "KVM";
>>>>>>      ac->init_machine = kvm_init;
>>>>>> +    ac->has_memory = kvm_accel_has_memory;
>>>>>>      ac->allowed = &kvm_allowed;
>>>>>>  }
>>>>>>  
>>>>>> diff --git a/memory.c b/memory.c
>>>>>> index d14c6de..61e758a 100644
>>>>>> --- a/memory.c
>>>>>> +++ b/memory.c
>>>>>> @@ -29,7 +29,9 @@
>>>>>>  #include "exec/ram_addr.h"
>>>>>>  #include "sysemu/kvm.h"
>>>>>>  #include "sysemu/sysemu.h"
>>>>>> +#include "sysemu/accel.h"
>>>>>>  #include "hw/qdev-properties.h"
>>>>>> +#include "hw/boards.h"
>>>>>>  #include "migration/vmstate.h"
>>>>>>  
>>>>>>  //#define DEBUG_UNASSIGNED
>>>>>> @@ -2924,6 +2926,8 @@ struct FlatViewInfo {
>>>>>>      int counter;
>>>>>>      bool dispatch_tree;
>>>>>>      bool owner;
>>>>>> +    AccelClass *ac;
>>>>>> +    const char *ac_name;
>>>>>>  };
>>>>>>  
>>>>>>  static void mtree_print_flatview(gpointer key, gpointer value,
>>>>>> @@ -2939,6 +2943,7 @@ static void mtree_print_flatview(gpointer key, gpointer value,
>>>>>>      int n = view->nr;
>>>>>>      int i;
>>>>>>      AddressSpace *as;
>>>>>> +    bool system_as = false;
>>>>>>  
>>>>>>      p(f, "FlatView #%d\n", fvi->counter);
>>>>>>      ++fvi->counter;
>>>>>> @@ -2950,6 +2955,9 @@ static void mtree_print_flatview(gpointer key, gpointer value,
>>>>>>              p(f, ", alias %s", memory_region_name(as->root->alias));
>>>>>>          }
>>>>>>          p(f, "\n");
>>>>>> +        if (as == &address_space_memory) {
>>>>>> +            system_as = true;
>>>>>> +        }
>>>>>>      }
>>>>>>  
>>>>>>      p(f, " Root memory region: %s\n",
>>>>>> @@ -2985,6 +2993,13 @@ static void mtree_print_flatview(gpointer key, gpointer value,
>>>>>>          if (fvi->owner) {
>>>>>>              mtree_print_mr_owner(p, f, mr);
>>>>>>          }
>>>>>> +
>>>>>> +        if (system_as && fvi->ac &&
>>>>>> +            fvi->ac->has_memory(current_machine,
>>>>>> +                                int128_get64(range->addr.start),
>>>>>> +                                MR_SIZE(range->addr.size) + 1)) {
>>>>>> +            p(f, " %s", fvi->ac_name);
>>>>>
>>>>> Why not simply display fvi->ac->name?
>>>>> You could avoid to add the ac_name field.
>>>>
>>>>
>>>> Well, I thought I better print whatever the user passed via the command
>>>> line (which is current_machine->accel and equals to "kvm" in my case)
>>>> rather than robotic, dry and excessive "kvm-accel".
>>>
>>> I have no hit for 'kvm-accel':
>>>
>>> $ git grep kvm-accel
>>> $
>>>
>>> Names looks human friendly:
>>
>>
>> Ah, I confused with object_class_get_name(). Anyway, I'd still show the
>> user provided accelerator name than some internal name.
> 
> Aren't they exactly the same?

Nope. "kvm" vs. "kvm-accel".

> I think the accel= thing from the user is
> parsed by comparing it with the ACCEL_CLASS_NAME declarations:
> 
> ./target/i386/whpx-all.c:    .name = ACCEL_CLASS_NAME("whpx"),
> ./target/i386/hax-all.c:    .name = ACCEL_CLASS_NAME("hax"),
> ./hw/xen/xen-common.c:#define TYPE_XEN_ACCEL ACCEL_CLASS_NAME("xen")
> ./qtest.c:#define TYPE_QTEST_ACCEL ACCEL_CLASS_NAME("qtest")
> ./accel/tcg/tcg-all.c:#define TYPE_TCG_ACCEL ACCEL_CLASS_NAME("tcg")
> ./include/sysemu/hvf.h:#define TYPE_HVF_ACCEL ACCEL_CLASS_NAME("hvf")
> ./include/sysemu/kvm_int.h:#define TYPE_KVM_ACCEL ACCEL_CLASS_NAME("kvm")
> 
> and the code in accel/accel.c accel_find


https://git.qemu.org/?p=qemu.git;a=blob;f=include/sysemu/accel.h;h=5565e00a96136bc5024c17c401116ef6a497138a;hb=HEAD#l58

  55 #define TYPE_ACCEL "accel"
  56
  57 #define ACCEL_CLASS_SUFFIX  "-" TYPE_ACCEL
  58 #define ACCEL_CLASS_NAME(a) (a ACCEL_CLASS_SUFFIX)



> 
> Dave
> 
>>
>>>
>>> $ git grep 'ac->name ='
>>> accel/kvm/kvm-all.c:2595:    ac->name = "KVM";
>>> accel/tcg/tcg-all.c:74:    ac->name = "tcg";
>>> hw/xen/xen-common.c:184:    ac->name = "Xen";
>>> target/i386/hax-all.c:1088:    ac->name = "HAX";
>>> target/i386/hvf/hvf.c:959:    ac->name = "HVF";
>>> target/i386/whpx-all.c:1477:    ac->name = "WHPX";
>>> qtest.c:755:    ac->name = "QTest";
>>>
>>>>>
>>>>>> +        }
>>>>>>          p(f, "\n");
>>>>>>          range++;
>>>>>>      }
>>>>>> @@ -3028,6 +3043,13 @@ void mtree_info(fprintf_function mon_printf, void *f, bool flatview,
>>>>>>          };
>>>>>>          GArray *fv_address_spaces;
>>>>>>          GHashTable *views = g_hash_table_new(g_direct_hash, g_direct_equal);
>>>>>> +        AccelClass *ac = ACCEL_GET_CLASS(current_machine->accelerator);
>>>>>> +
>>>>>> +        if (ac->has_memory) {
>>>>>> +            fvi.ac = ac;
>>>>>> +            fvi.ac_name = current_machine->accel ? current_machine->accel :
>>>>>> +                object_class_get_name(OBJECT_CLASS(ac));
>>>>>> +        }
>>>>>>  
>>>>>>          /* Gather all FVs in one table */
>>>>>>          QTAILQ_FOREACH(as, &address_spaces, address_spaces_link) {
>>>>>>
>>
>> -- 
>> Alexey
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>

On 14/01/2019 12:43, Alexey Kardashevskiy wrote:
> 
> 
> On 04/01/2019 04:37, Dr. David Alan Gilbert wrote:
>> * Alexey Kardashevskiy (aik@ozlabs.ru) wrote:
>>>
>>>
>>> On 17/12/2018 23:47, Philippe Mathieu-Daudé wrote:
>>>> On 12/17/18 2:27 AM, Alexey Kardashevskiy wrote:
>>>>> On 14/12/2018 22:07, Philippe Mathieu-Daudé wrote:
>>>>>> Hi Alexey,
>>>>>>
>>>>>> On 12/14/18 3:58 AM, Alexey Kardashevskiy wrote:
>>>>>>> This adds an accelerator name to the "into mtree -f" to tell the user if
>>>>>>> a particular memory section is registered with the accelerator;
>>>>>>> the primary user for this is KVM and such information is useful
>>>>>>> for debugging purposes.
>>>>>>>
>>>>>>> This adds a has_memory() callback to the accelerator class allowing any
>>>>>>> accelerator to have a label in that memory tree dump.
>>>>>>>
>>>>>>> Since memory sections are passed to memory listeners and get registered
>>>>>>> in accelerators (rather than memory regions), this only prints new labels
>>>>>>> for flatviews attached to the system address space.
>>>>>>>
>>>>>>> An example:
>>>>>>>  Root memory region: system
>>>>>>>   0000000000000000-0000002fffffffff (prio 0, ram): /objects/mem0 kvm
>>>>>>>   0000003000000000-0000005fffffffff (prio 0, ram): /objects/mem1 kvm
>>>>>>>   0000200000000020-000020000000003f (prio 1, i/o): virtio-pci
>>>>>>>   0000200080000000-000020008000003f (prio 0, i/o): capabilities
>>>>>>>
>>>>>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>>>>>> ---
>>>>>>>
>>>>>>> This supercedes "[PATCH qemu] hmp: Print if memory section is registered in KVM"
>>>>>>>
>>>>>>> ---
>>>>>>> Changes:
>>>>>>> v2:
>>>>>>> * added an accelerator callback instead of hardcoding it to kvm only
>>>>>>> ---
>>>>>>>  include/sysemu/accel.h |  2 ++
>>>>>>>  accel/kvm/kvm-all.c    | 10 ++++++++++
>>>>>>>  memory.c               | 22 ++++++++++++++++++++++
>>>>>>>  3 files changed, 34 insertions(+)
>>>>>>>
>>>>>>> diff --git a/include/sysemu/accel.h b/include/sysemu/accel.h
>>>>>>> index 637358f..30b456d 100644
>>>>>>> --- a/include/sysemu/accel.h
>>>>>>> +++ b/include/sysemu/accel.h
>>>>>>> @@ -25,6 +25,7 @@
>>>>>>>  
>>>>>>>  #include "qom/object.h"
>>>>>>>  #include "hw/qdev-properties.h"
>>>>>>> +#include "exec/hwaddr.h"
>>>>>>>  
>>>>>>>  typedef struct AccelState {
>>>>>>>      /*< private >*/
>>>>>>> @@ -41,6 +42,7 @@ typedef struct AccelClass {
>>>>>>>      int (*available)(void);
>>>>>>>      int (*init_machine)(MachineState *ms);
>>>>>>>      void (*setup_post)(MachineState *ms, AccelState *accel);
>>>>>>> +    bool (*has_memory)(MachineState *ms, hwaddr start_addr, hwaddr size);
>>>>>>>      bool *allowed;
>>>>>>>      /*
>>>>>>>       * Array of global properties that would be applied when specific
>>>>>>> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
>>>>>>> index 4880a05..634f386 100644
>>>>>>> --- a/accel/kvm/kvm-all.c
>>>>>>> +++ b/accel/kvm/kvm-all.c
>>>>>>> @@ -2589,11 +2589,21 @@ int kvm_get_one_reg(CPUState *cs, uint64_t id, void *target)
>>>>>>>      return r;
>>>>>>>  }
>>>>>>>  
>>>>>>> +static bool kvm_accel_has_memory(MachineState *ms, hwaddr start_addr,
>>>>>>> +                                 hwaddr size)
>>>>>>> +{
>>>>>>> +    KVMState *kvm = KVM_STATE(ms->accelerator);
>>>>>>> +    KVMMemoryListener *kml = &kvm->memory_listener;
>>>>>>> +
>>>>>>> +    return NULL != kvm_lookup_matching_slot(kml, start_addr, size);
>>>>>>> +}
>>>>>>> +
>>>>>>>  static void kvm_accel_class_init(ObjectClass *oc, void *data)
>>>>>>>  {
>>>>>>>      AccelClass *ac = ACCEL_CLASS(oc);
>>>>>>>      ac->name = "KVM";
>>>>>>>      ac->init_machine = kvm_init;
>>>>>>> +    ac->has_memory = kvm_accel_has_memory;
>>>>>>>      ac->allowed = &kvm_allowed;
>>>>>>>  }
>>>>>>>  
>>>>>>> diff --git a/memory.c b/memory.c
>>>>>>> index d14c6de..61e758a 100644
>>>>>>> --- a/memory.c
>>>>>>> +++ b/memory.c
>>>>>>> @@ -29,7 +29,9 @@
>>>>>>>  #include "exec/ram_addr.h"
>>>>>>>  #include "sysemu/kvm.h"
>>>>>>>  #include "sysemu/sysemu.h"
>>>>>>> +#include "sysemu/accel.h"
>>>>>>>  #include "hw/qdev-properties.h"
>>>>>>> +#include "hw/boards.h"
>>>>>>>  #include "migration/vmstate.h"
>>>>>>>  
>>>>>>>  //#define DEBUG_UNASSIGNED
>>>>>>> @@ -2924,6 +2926,8 @@ struct FlatViewInfo {
>>>>>>>      int counter;
>>>>>>>      bool dispatch_tree;
>>>>>>>      bool owner;
>>>>>>> +    AccelClass *ac;
>>>>>>> +    const char *ac_name;
>>>>>>>  };
>>>>>>>  
>>>>>>>  static void mtree_print_flatview(gpointer key, gpointer value,
>>>>>>> @@ -2939,6 +2943,7 @@ static void mtree_print_flatview(gpointer key, gpointer value,
>>>>>>>      int n = view->nr;
>>>>>>>      int i;
>>>>>>>      AddressSpace *as;
>>>>>>> +    bool system_as = false;
>>>>>>>  
>>>>>>>      p(f, "FlatView #%d\n", fvi->counter);
>>>>>>>      ++fvi->counter;
>>>>>>> @@ -2950,6 +2955,9 @@ static void mtree_print_flatview(gpointer key, gpointer value,
>>>>>>>              p(f, ", alias %s", memory_region_name(as->root->alias));
>>>>>>>          }
>>>>>>>          p(f, "\n");
>>>>>>> +        if (as == &address_space_memory) {
>>>>>>> +            system_as = true;
>>>>>>> +        }
>>>>>>>      }
>>>>>>>  
>>>>>>>      p(f, " Root memory region: %s\n",
>>>>>>> @@ -2985,6 +2993,13 @@ static void mtree_print_flatview(gpointer key, gpointer value,
>>>>>>>          if (fvi->owner) {
>>>>>>>              mtree_print_mr_owner(p, f, mr);
>>>>>>>          }
>>>>>>> +
>>>>>>> +        if (system_as && fvi->ac &&
>>>>>>> +            fvi->ac->has_memory(current_machine,
>>>>>>> +                                int128_get64(range->addr.start),
>>>>>>> +                                MR_SIZE(range->addr.size) + 1)) {
>>>>>>> +            p(f, " %s", fvi->ac_name);
>>>>>>
>>>>>> Why not simply display fvi->ac->name?
>>>>>> You could avoid to add the ac_name field.
>>>>>
>>>>>
>>>>> Well, I thought I better print whatever the user passed via the command
>>>>> line (which is current_machine->accel and equals to "kvm" in my case)
>>>>> rather than robotic, dry and excessive "kvm-accel".
>>>>
>>>> I have no hit for 'kvm-accel':
>>>>
>>>> $ git grep kvm-accel
>>>> $
>>>>
>>>> Names looks human friendly:
>>>
>>>
>>> Ah, I confused with object_class_get_name(). Anyway, I'd still show the
>>> user provided accelerator name than some internal name.
>>
>> Aren't they exactly the same?
> 
> Nope. "kvm" vs. "kvm-accel".
> 
>> I think the accel= thing from the user is
>> parsed by comparing it with the ACCEL_CLASS_NAME declarations:
>>
>> ./target/i386/whpx-all.c:    .name = ACCEL_CLASS_NAME("whpx"),
>> ./target/i386/hax-all.c:    .name = ACCEL_CLASS_NAME("hax"),
>> ./hw/xen/xen-common.c:#define TYPE_XEN_ACCEL ACCEL_CLASS_NAME("xen")
>> ./qtest.c:#define TYPE_QTEST_ACCEL ACCEL_CLASS_NAME("qtest")
>> ./accel/tcg/tcg-all.c:#define TYPE_TCG_ACCEL ACCEL_CLASS_NAME("tcg")
>> ./include/sysemu/hvf.h:#define TYPE_HVF_ACCEL ACCEL_CLASS_NAME("hvf")
>> ./include/sysemu/kvm_int.h:#define TYPE_KVM_ACCEL ACCEL_CLASS_NAME("kvm")
>>
>> and the code in accel/accel.c accel_find
> 
> 
> https://git.qemu.org/?p=qemu.git;a=blob;f=include/sysemu/accel.h;h=5565e00a96136bc5024c17c401116ef6a497138a;hb=HEAD#l58
> 
>   55 #define TYPE_ACCEL "accel"
>   56
>   57 #define ACCEL_CLASS_SUFFIX  "-" TYPE_ACCEL
>   58 #define ACCEL_CLASS_NAME(a) (a ACCEL_CLASS_SUFFIX)


Ping, anybody?


> 
> 
> 
>>
>> Dave
>>
>>>
>>>>
>>>> $ git grep 'ac->name ='
>>>> accel/kvm/kvm-all.c:2595:    ac->name = "KVM";
>>>> accel/tcg/tcg-all.c:74:    ac->name = "tcg";
>>>> hw/xen/xen-common.c:184:    ac->name = "Xen";
>>>> target/i386/hax-all.c:1088:    ac->name = "HAX";
>>>> target/i386/hvf/hvf.c:959:    ac->name = "HVF";
>>>> target/i386/whpx-all.c:1477:    ac->name = "WHPX";
>>>> qtest.c:755:    ac->name = "QTest";
>>>>
>>>>>>
>>>>>>> +        }
>>>>>>>          p(f, "\n");
>>>>>>>          range++;
>>>>>>>      }
>>>>>>> @@ -3028,6 +3043,13 @@ void mtree_info(fprintf_function mon_printf, void *f, bool flatview,
>>>>>>>          };
>>>>>>>          GArray *fv_address_spaces;
>>>>>>>          GHashTable *views = g_hash_table_new(g_direct_hash, g_direct_equal);
>>>>>>> +        AccelClass *ac = ACCEL_GET_CLASS(current_machine->accelerator);
>>>>>>> +
>>>>>>> +        if (ac->has_memory) {
>>>>>>> +            fvi.ac = ac;
>>>>>>> +            fvi.ac_name = current_machine->accel ? current_machine->accel :
>>>>>>> +                object_class_get_name(OBJECT_CLASS(ac));
>>>>>>> +        }
>>>>>>>  
>>>>>>>          /* Gather all FVs in one table */
>>>>>>>          QTAILQ_FOREACH(as, &address_spaces, address_spaces_link) {
>>>>>>>
>>>
>>> -- 
>>> Alexey
>> --
>> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>>
>

On 29/01/2019 13:30, Alexey Kardashevskiy wrote:
> 
> 
> On 14/01/2019 12:43, Alexey Kardashevskiy wrote:
>>
>>
>> On 04/01/2019 04:37, Dr. David Alan Gilbert wrote:
>>> * Alexey Kardashevskiy (aik@ozlabs.ru) wrote:
>>>>
>>>>
>>>> On 17/12/2018 23:47, Philippe Mathieu-Daudé wrote:
>>>>> On 12/17/18 2:27 AM, Alexey Kardashevskiy wrote:
>>>>>> On 14/12/2018 22:07, Philippe Mathieu-Daudé wrote:
>>>>>>> Hi Alexey,
>>>>>>>
>>>>>>> On 12/14/18 3:58 AM, Alexey Kardashevskiy wrote:
>>>>>>>> This adds an accelerator name to the "into mtree -f" to tell the user if
>>>>>>>> a particular memory section is registered with the accelerator;
>>>>>>>> the primary user for this is KVM and such information is useful
>>>>>>>> for debugging purposes.
>>>>>>>>
>>>>>>>> This adds a has_memory() callback to the accelerator class allowing any
>>>>>>>> accelerator to have a label in that memory tree dump.
>>>>>>>>
>>>>>>>> Since memory sections are passed to memory listeners and get registered
>>>>>>>> in accelerators (rather than memory regions), this only prints new labels
>>>>>>>> for flatviews attached to the system address space.
>>>>>>>>
>>>>>>>> An example:
>>>>>>>>  Root memory region: system
>>>>>>>>   0000000000000000-0000002fffffffff (prio 0, ram): /objects/mem0 kvm
>>>>>>>>   0000003000000000-0000005fffffffff (prio 0, ram): /objects/mem1 kvm
>>>>>>>>   0000200000000020-000020000000003f (prio 1, i/o): virtio-pci
>>>>>>>>   0000200080000000-000020008000003f (prio 0, i/o): capabilities
>>>>>>>>
>>>>>>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>>>>>>> ---
>>>>>>>>
>>>>>>>> This supercedes "[PATCH qemu] hmp: Print if memory section is registered in KVM"
>>>>>>>>
>>>>>>>> ---
>>>>>>>> Changes:
>>>>>>>> v2:
>>>>>>>> * added an accelerator callback instead of hardcoding it to kvm only
>>>>>>>> ---
>>>>>>>>  include/sysemu/accel.h |  2 ++
>>>>>>>>  accel/kvm/kvm-all.c    | 10 ++++++++++
>>>>>>>>  memory.c               | 22 ++++++++++++++++++++++
>>>>>>>>  3 files changed, 34 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/include/sysemu/accel.h b/include/sysemu/accel.h
>>>>>>>> index 637358f..30b456d 100644
>>>>>>>> --- a/include/sysemu/accel.h
>>>>>>>> +++ b/include/sysemu/accel.h
>>>>>>>> @@ -25,6 +25,7 @@
>>>>>>>>  
>>>>>>>>  #include "qom/object.h"
>>>>>>>>  #include "hw/qdev-properties.h"
>>>>>>>> +#include "exec/hwaddr.h"
>>>>>>>>  
>>>>>>>>  typedef struct AccelState {
>>>>>>>>      /*< private >*/
>>>>>>>> @@ -41,6 +42,7 @@ typedef struct AccelClass {
>>>>>>>>      int (*available)(void);
>>>>>>>>      int (*init_machine)(MachineState *ms);
>>>>>>>>      void (*setup_post)(MachineState *ms, AccelState *accel);
>>>>>>>> +    bool (*has_memory)(MachineState *ms, hwaddr start_addr, hwaddr size);
>>>>>>>>      bool *allowed;
>>>>>>>>      /*
>>>>>>>>       * Array of global properties that would be applied when specific
>>>>>>>> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
>>>>>>>> index 4880a05..634f386 100644
>>>>>>>> --- a/accel/kvm/kvm-all.c
>>>>>>>> +++ b/accel/kvm/kvm-all.c
>>>>>>>> @@ -2589,11 +2589,21 @@ int kvm_get_one_reg(CPUState *cs, uint64_t id, void *target)
>>>>>>>>      return r;
>>>>>>>>  }
>>>>>>>>  
>>>>>>>> +static bool kvm_accel_has_memory(MachineState *ms, hwaddr start_addr,
>>>>>>>> +                                 hwaddr size)
>>>>>>>> +{
>>>>>>>> +    KVMState *kvm = KVM_STATE(ms->accelerator);
>>>>>>>> +    KVMMemoryListener *kml = &kvm->memory_listener;
>>>>>>>> +
>>>>>>>> +    return NULL != kvm_lookup_matching_slot(kml, start_addr, size);
>>>>>>>> +}
>>>>>>>> +
>>>>>>>>  static void kvm_accel_class_init(ObjectClass *oc, void *data)
>>>>>>>>  {
>>>>>>>>      AccelClass *ac = ACCEL_CLASS(oc);
>>>>>>>>      ac->name = "KVM";
>>>>>>>>      ac->init_machine = kvm_init;
>>>>>>>> +    ac->has_memory = kvm_accel_has_memory;
>>>>>>>>      ac->allowed = &kvm_allowed;
>>>>>>>>  }
>>>>>>>>  
>>>>>>>> diff --git a/memory.c b/memory.c
>>>>>>>> index d14c6de..61e758a 100644
>>>>>>>> --- a/memory.c
>>>>>>>> +++ b/memory.c
>>>>>>>> @@ -29,7 +29,9 @@
>>>>>>>>  #include "exec/ram_addr.h"
>>>>>>>>  #include "sysemu/kvm.h"
>>>>>>>>  #include "sysemu/sysemu.h"
>>>>>>>> +#include "sysemu/accel.h"
>>>>>>>>  #include "hw/qdev-properties.h"
>>>>>>>> +#include "hw/boards.h"
>>>>>>>>  #include "migration/vmstate.h"
>>>>>>>>  
>>>>>>>>  //#define DEBUG_UNASSIGNED
>>>>>>>> @@ -2924,6 +2926,8 @@ struct FlatViewInfo {
>>>>>>>>      int counter;
>>>>>>>>      bool dispatch_tree;
>>>>>>>>      bool owner;
>>>>>>>> +    AccelClass *ac;
>>>>>>>> +    const char *ac_name;
>>>>>>>>  };
>>>>>>>>  
>>>>>>>>  static void mtree_print_flatview(gpointer key, gpointer value,
>>>>>>>> @@ -2939,6 +2943,7 @@ static void mtree_print_flatview(gpointer key, gpointer value,
>>>>>>>>      int n = view->nr;
>>>>>>>>      int i;
>>>>>>>>      AddressSpace *as;
>>>>>>>> +    bool system_as = false;
>>>>>>>>  
>>>>>>>>      p(f, "FlatView #%d\n", fvi->counter);
>>>>>>>>      ++fvi->counter;
>>>>>>>> @@ -2950,6 +2955,9 @@ static void mtree_print_flatview(gpointer key, gpointer value,
>>>>>>>>              p(f, ", alias %s", memory_region_name(as->root->alias));
>>>>>>>>          }
>>>>>>>>          p(f, "\n");
>>>>>>>> +        if (as == &address_space_memory) {
>>>>>>>> +            system_as = true;
>>>>>>>> +        }
>>>>>>>>      }
>>>>>>>>  
>>>>>>>>      p(f, " Root memory region: %s\n",
>>>>>>>> @@ -2985,6 +2993,13 @@ static void mtree_print_flatview(gpointer key, gpointer value,
>>>>>>>>          if (fvi->owner) {
>>>>>>>>              mtree_print_mr_owner(p, f, mr);
>>>>>>>>          }
>>>>>>>> +
>>>>>>>> +        if (system_as && fvi->ac &&
>>>>>>>> +            fvi->ac->has_memory(current_machine,
>>>>>>>> +                                int128_get64(range->addr.start),
>>>>>>>> +                                MR_SIZE(range->addr.size) + 1)) {
>>>>>>>> +            p(f, " %s", fvi->ac_name);
>>>>>>>
>>>>>>> Why not simply display fvi->ac->name?
>>>>>>> You could avoid to add the ac_name field.
>>>>>>
>>>>>>
>>>>>> Well, I thought I better print whatever the user passed via the command
>>>>>> line (which is current_machine->accel and equals to "kvm" in my case)
>>>>>> rather than robotic, dry and excessive "kvm-accel".
>>>>>
>>>>> I have no hit for 'kvm-accel':
>>>>>
>>>>> $ git grep kvm-accel
>>>>> $
>>>>>
>>>>> Names looks human friendly:
>>>>
>>>>
>>>> Ah, I confused with object_class_get_name(). Anyway, I'd still show the
>>>> user provided accelerator name than some internal name.
>>>
>>> Aren't they exactly the same?
>>
>> Nope. "kvm" vs. "kvm-accel".
>>
>>> I think the accel= thing from the user is
>>> parsed by comparing it with the ACCEL_CLASS_NAME declarations:
>>>
>>> ./target/i386/whpx-all.c:    .name = ACCEL_CLASS_NAME("whpx"),
>>> ./target/i386/hax-all.c:    .name = ACCEL_CLASS_NAME("hax"),
>>> ./hw/xen/xen-common.c:#define TYPE_XEN_ACCEL ACCEL_CLASS_NAME("xen")
>>> ./qtest.c:#define TYPE_QTEST_ACCEL ACCEL_CLASS_NAME("qtest")
>>> ./accel/tcg/tcg-all.c:#define TYPE_TCG_ACCEL ACCEL_CLASS_NAME("tcg")
>>> ./include/sysemu/hvf.h:#define TYPE_HVF_ACCEL ACCEL_CLASS_NAME("hvf")
>>> ./include/sysemu/kvm_int.h:#define TYPE_KVM_ACCEL ACCEL_CLASS_NAME("kvm")
>>>
>>> and the code in accel/accel.c accel_find
>>
>>
>> https://git.qemu.org/?p=qemu.git;a=blob;f=include/sysemu/accel.h;h=5565e00a96136bc5024c17c401116ef6a497138a;hb=HEAD#l58
>>
>>   55 #define TYPE_ACCEL "accel"
>>   56
>>   57 #define ACCEL_CLASS_SUFFIX  "-" TYPE_ACCEL
>>   58 #define ACCEL_CLASS_NAME(a) (a ACCEL_CLASS_SUFFIX)
> 
> 
> Ping, anybody?


Yet another friendly ping. Any comments? Thanks.


> 
> 
>>
>>
>>
>>>
>>> Dave
>>>
>>>>
>>>>>
>>>>> $ git grep 'ac->name ='
>>>>> accel/kvm/kvm-all.c:2595:    ac->name = "KVM";
>>>>> accel/tcg/tcg-all.c:74:    ac->name = "tcg";
>>>>> hw/xen/xen-common.c:184:    ac->name = "Xen";
>>>>> target/i386/hax-all.c:1088:    ac->name = "HAX";
>>>>> target/i386/hvf/hvf.c:959:    ac->name = "HVF";
>>>>> target/i386/whpx-all.c:1477:    ac->name = "WHPX";
>>>>> qtest.c:755:    ac->name = "QTest";
>>>>>
>>>>>>>
>>>>>>>> +        }
>>>>>>>>          p(f, "\n");
>>>>>>>>          range++;
>>>>>>>>      }
>>>>>>>> @@ -3028,6 +3043,13 @@ void mtree_info(fprintf_function mon_printf, void *f, bool flatview,
>>>>>>>>          };
>>>>>>>>          GArray *fv_address_spaces;
>>>>>>>>          GHashTable *views = g_hash_table_new(g_direct_hash, g_direct_equal);
>>>>>>>> +        AccelClass *ac = ACCEL_GET_CLASS(current_machine->accelerator);
>>>>>>>> +
>>>>>>>> +        if (ac->has_memory) {
>>>>>>>> +            fvi.ac = ac;
>>>>>>>> +            fvi.ac_name = current_machine->accel ? current_machine->accel :
>>>>>>>> +                object_class_get_name(OBJECT_CLASS(ac));
>>>>>>>> +        }
>>>>>>>>  
>>>>>>>>          /* Gather all FVs in one table */
>>>>>>>>          QTAILQ_FOREACH(as, &address_spaces, address_spaces_link) {
>>>>>>>>
>>>>
>>>> -- 
>>>> Alexey
>>> --
>>> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>>>
>>
>

* Alexey Kardashevskiy (aik@ozlabs.ru) wrote:
> 
> 
> On 04/01/2019 04:37, Dr. David Alan Gilbert wrote:
> > * Alexey Kardashevskiy (aik@ozlabs.ru) wrote:
> >>
> >>
> >> On 17/12/2018 23:47, Philippe Mathieu-Daudé wrote:
> >>> On 12/17/18 2:27 AM, Alexey Kardashevskiy wrote:
> >>>> On 14/12/2018 22:07, Philippe Mathieu-Daudé wrote:
> >>>>> Hi Alexey,
> >>>>>
> >>>>> On 12/14/18 3:58 AM, Alexey Kardashevskiy wrote:
> >>>>>> This adds an accelerator name to the "into mtree -f" to tell the user if
> >>>>>> a particular memory section is registered with the accelerator;
> >>>>>> the primary user for this is KVM and such information is useful
> >>>>>> for debugging purposes.
> >>>>>>
> >>>>>> This adds a has_memory() callback to the accelerator class allowing any
> >>>>>> accelerator to have a label in that memory tree dump.
> >>>>>>
> >>>>>> Since memory sections are passed to memory listeners and get registered
> >>>>>> in accelerators (rather than memory regions), this only prints new labels
> >>>>>> for flatviews attached to the system address space.
> >>>>>>
> >>>>>> An example:
> >>>>>>  Root memory region: system
> >>>>>>   0000000000000000-0000002fffffffff (prio 0, ram): /objects/mem0 kvm
> >>>>>>   0000003000000000-0000005fffffffff (prio 0, ram): /objects/mem1 kvm
> >>>>>>   0000200000000020-000020000000003f (prio 1, i/o): virtio-pci
> >>>>>>   0000200080000000-000020008000003f (prio 0, i/o): capabilities
> >>>>>>
> >>>>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> >>>>>> ---
> >>>>>>
> >>>>>> This supercedes "[PATCH qemu] hmp: Print if memory section is registered in KVM"
> >>>>>>
> >>>>>> ---
> >>>>>> Changes:
> >>>>>> v2:
> >>>>>> * added an accelerator callback instead of hardcoding it to kvm only
> >>>>>> ---
> >>>>>>  include/sysemu/accel.h |  2 ++
> >>>>>>  accel/kvm/kvm-all.c    | 10 ++++++++++
> >>>>>>  memory.c               | 22 ++++++++++++++++++++++
> >>>>>>  3 files changed, 34 insertions(+)
> >>>>>>
> >>>>>> diff --git a/include/sysemu/accel.h b/include/sysemu/accel.h
> >>>>>> index 637358f..30b456d 100644
> >>>>>> --- a/include/sysemu/accel.h
> >>>>>> +++ b/include/sysemu/accel.h
> >>>>>> @@ -25,6 +25,7 @@
> >>>>>>  
> >>>>>>  #include "qom/object.h"
> >>>>>>  #include "hw/qdev-properties.h"
> >>>>>> +#include "exec/hwaddr.h"
> >>>>>>  
> >>>>>>  typedef struct AccelState {
> >>>>>>      /*< private >*/
> >>>>>> @@ -41,6 +42,7 @@ typedef struct AccelClass {
> >>>>>>      int (*available)(void);
> >>>>>>      int (*init_machine)(MachineState *ms);
> >>>>>>      void (*setup_post)(MachineState *ms, AccelState *accel);
> >>>>>> +    bool (*has_memory)(MachineState *ms, hwaddr start_addr, hwaddr size);
> >>>>>>      bool *allowed;
> >>>>>>      /*
> >>>>>>       * Array of global properties that would be applied when specific
> >>>>>> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> >>>>>> index 4880a05..634f386 100644
> >>>>>> --- a/accel/kvm/kvm-all.c
> >>>>>> +++ b/accel/kvm/kvm-all.c
> >>>>>> @@ -2589,11 +2589,21 @@ int kvm_get_one_reg(CPUState *cs, uint64_t id, void *target)
> >>>>>>      return r;
> >>>>>>  }
> >>>>>>  
> >>>>>> +static bool kvm_accel_has_memory(MachineState *ms, hwaddr start_addr,
> >>>>>> +                                 hwaddr size)
> >>>>>> +{
> >>>>>> +    KVMState *kvm = KVM_STATE(ms->accelerator);
> >>>>>> +    KVMMemoryListener *kml = &kvm->memory_listener;
> >>>>>> +
> >>>>>> +    return NULL != kvm_lookup_matching_slot(kml, start_addr, size);
> >>>>>> +}
> >>>>>> +
> >>>>>>  static void kvm_accel_class_init(ObjectClass *oc, void *data)
> >>>>>>  {
> >>>>>>      AccelClass *ac = ACCEL_CLASS(oc);
> >>>>>>      ac->name = "KVM";
> >>>>>>      ac->init_machine = kvm_init;
> >>>>>> +    ac->has_memory = kvm_accel_has_memory;
> >>>>>>      ac->allowed = &kvm_allowed;
> >>>>>>  }
> >>>>>>  
> >>>>>> diff --git a/memory.c b/memory.c
> >>>>>> index d14c6de..61e758a 100644
> >>>>>> --- a/memory.c
> >>>>>> +++ b/memory.c
> >>>>>> @@ -29,7 +29,9 @@
> >>>>>>  #include "exec/ram_addr.h"
> >>>>>>  #include "sysemu/kvm.h"
> >>>>>>  #include "sysemu/sysemu.h"
> >>>>>> +#include "sysemu/accel.h"
> >>>>>>  #include "hw/qdev-properties.h"
> >>>>>> +#include "hw/boards.h"
> >>>>>>  #include "migration/vmstate.h"
> >>>>>>  
> >>>>>>  //#define DEBUG_UNASSIGNED
> >>>>>> @@ -2924,6 +2926,8 @@ struct FlatViewInfo {
> >>>>>>      int counter;
> >>>>>>      bool dispatch_tree;
> >>>>>>      bool owner;
> >>>>>> +    AccelClass *ac;
> >>>>>> +    const char *ac_name;
> >>>>>>  };
> >>>>>>  
> >>>>>>  static void mtree_print_flatview(gpointer key, gpointer value,
> >>>>>> @@ -2939,6 +2943,7 @@ static void mtree_print_flatview(gpointer key, gpointer value,
> >>>>>>      int n = view->nr;
> >>>>>>      int i;
> >>>>>>      AddressSpace *as;
> >>>>>> +    bool system_as = false;
> >>>>>>  
> >>>>>>      p(f, "FlatView #%d\n", fvi->counter);
> >>>>>>      ++fvi->counter;
> >>>>>> @@ -2950,6 +2955,9 @@ static void mtree_print_flatview(gpointer key, gpointer value,
> >>>>>>              p(f, ", alias %s", memory_region_name(as->root->alias));
> >>>>>>          }
> >>>>>>          p(f, "\n");
> >>>>>> +        if (as == &address_space_memory) {
> >>>>>> +            system_as = true;
> >>>>>> +        }
> >>>>>>      }
> >>>>>>  
> >>>>>>      p(f, " Root memory region: %s\n",
> >>>>>> @@ -2985,6 +2993,13 @@ static void mtree_print_flatview(gpointer key, gpointer value,
> >>>>>>          if (fvi->owner) {
> >>>>>>              mtree_print_mr_owner(p, f, mr);
> >>>>>>          }
> >>>>>> +
> >>>>>> +        if (system_as && fvi->ac &&
> >>>>>> +            fvi->ac->has_memory(current_machine,
> >>>>>> +                                int128_get64(range->addr.start),
> >>>>>> +                                MR_SIZE(range->addr.size) + 1)) {
> >>>>>> +            p(f, " %s", fvi->ac_name);
> >>>>>
> >>>>> Why not simply display fvi->ac->name?
> >>>>> You could avoid to add the ac_name field.
> >>>>
> >>>>
> >>>> Well, I thought I better print whatever the user passed via the command
> >>>> line (which is current_machine->accel and equals to "kvm" in my case)
> >>>> rather than robotic, dry and excessive "kvm-accel".
> >>>
> >>> I have no hit for 'kvm-accel':
> >>>
> >>> $ git grep kvm-accel
> >>> $
> >>>
> >>> Names looks human friendly:
> >>
> >>
> >> Ah, I confused with object_class_get_name(). Anyway, I'd still show the
> >> user provided accelerator name than some internal name.
> > 
> > Aren't they exactly the same?
> 
> Nope. "kvm" vs. "kvm-accel".
> 
> > I think the accel= thing from the user is
> > parsed by comparing it with the ACCEL_CLASS_NAME declarations:
> > 
> > ./target/i386/whpx-all.c:    .name = ACCEL_CLASS_NAME("whpx"),
> > ./target/i386/hax-all.c:    .name = ACCEL_CLASS_NAME("hax"),
> > ./hw/xen/xen-common.c:#define TYPE_XEN_ACCEL ACCEL_CLASS_NAME("xen")
> > ./qtest.c:#define TYPE_QTEST_ACCEL ACCEL_CLASS_NAME("qtest")
> > ./accel/tcg/tcg-all.c:#define TYPE_TCG_ACCEL ACCEL_CLASS_NAME("tcg")
> > ./include/sysemu/hvf.h:#define TYPE_HVF_ACCEL ACCEL_CLASS_NAME("hvf")
> > ./include/sysemu/kvm_int.h:#define TYPE_KVM_ACCEL ACCEL_CLASS_NAME("kvm")
> > 
> > and the code in accel/accel.c accel_find
> 
> 
> https://git.qemu.org/?p=qemu.git;a=blob;f=include/sysemu/accel.h;h=5565e00a96136bc5024c17c401116ef6a497138a;hb=HEAD#l58
> 
>   55 #define TYPE_ACCEL "accel"
>   56
>   57 #define ACCEL_CLASS_SUFFIX  "-" TYPE_ACCEL
>   58 #define ACCEL_CLASS_NAME(a) (a ACCEL_CLASS_SUFFIX)

(Following up from your pings).
I'm OK from the HMP side, but I agree it would be nice to avoid adding
the ac_name field in FlatViewInfo if it's only reason is to get rid of
the '-accel'.   If that's the only reason why don't you just truncate
the fvi->ac->name when you display it?  You could do a build-time
check that TYPE_ACCEL and ACCEL_CLASS_SUFFIX haven't changed.

Dave

> 
> 
> > 
> > Dave
> > 
> >>
> >>>
> >>> $ git grep 'ac->name ='
> >>> accel/kvm/kvm-all.c:2595:    ac->name = "KVM";
> >>> accel/tcg/tcg-all.c:74:    ac->name = "tcg";
> >>> hw/xen/xen-common.c:184:    ac->name = "Xen";
> >>> target/i386/hax-all.c:1088:    ac->name = "HAX";
> >>> target/i386/hvf/hvf.c:959:    ac->name = "HVF";
> >>> target/i386/whpx-all.c:1477:    ac->name = "WHPX";
> >>> qtest.c:755:    ac->name = "QTest";
> >>>
> >>>>>
> >>>>>> +        }
> >>>>>>          p(f, "\n");
> >>>>>>          range++;
> >>>>>>      }
> >>>>>> @@ -3028,6 +3043,13 @@ void mtree_info(fprintf_function mon_printf, void *f, bool flatview,
> >>>>>>          };
> >>>>>>          GArray *fv_address_spaces;
> >>>>>>          GHashTable *views = g_hash_table_new(g_direct_hash, g_direct_equal);
> >>>>>> +        AccelClass *ac = ACCEL_GET_CLASS(current_machine->accelerator);
> >>>>>> +
> >>>>>> +        if (ac->has_memory) {
> >>>>>> +            fvi.ac = ac;
> >>>>>> +            fvi.ac_name = current_machine->accel ? current_machine->accel :
> >>>>>> +                object_class_get_name(OBJECT_CLASS(ac));
> >>>>>> +        }
> >>>>>>  
> >>>>>>          /* Gather all FVs in one table */
> >>>>>>          QTAILQ_FOREACH(as, &address_spaces, address_spaces_link) {
> >>>>>>
> >>
> >> -- 
> >> Alexey
> > --
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> > 
> 
> -- 
> Alexey
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

On 07/02/19 12:49, Dr. David Alan Gilbert wrote:
>  //#define DEBUG_UNASSIGNED
> @@ -2924,6 +2926,8 @@ struct FlatViewInfo {
>      int counter;
>      bool dispatch_tree;
>      bool owner;
> +    AccelClass *ac;
> +    const char *ac_name;
>  };
>  
>  static void mtree_print_flatview(gpointer key, gpointer value,
> @@ -2939,6 +2943,7 @@ static void mtree_print_flatview(gpointer key, gpointer value,
>      int n = view->nr;
>      int i;
>      AddressSpace *as;
> +    bool system_as = false;
>  
>      p(f, "FlatView #%d\n", fvi->counter);
>      ++fvi->counter;
> @@ -2950,6 +2955,9 @@ static void mtree_print_flatview(gpointer key, gpointer value,
>              p(f, ", alias %s", memory_region_name(as->root->alias));
>          }
>          p(f, "\n");
> +        if (as == &address_space_memory) {
> +            system_as = true;
> +        }
>      }
>  
>      p(f, " Root memory region: %s\n",
> @@ -2985,6 +2993,13 @@ static void mtree_print_flatview(gpointer key, gpointer value,
>          if (fvi->owner) {
>              mtree_print_mr_owner(p, f, mr);
>          }
> +
> +        if (system_as && fvi->ac &&
> +            fvi->ac->has_memory(current_machine,
> +                                int128_get64(range->addr.start),
> +                                MR_SIZE(range->addr.size) + 1)) {
> +            p(f, " %s", fvi->ac_name);

I don't understand this.  This doesn't check that the memory range
actually matches the memory registered with the accelerator, only that
there is something in that range.  Why isn't it enough to use "info
mtree" and look at the KVM address space?

Perhaps you could add instead an argument to "info mtree" that prints
only the AddressSpace with a given name?

Paolo

On 09/02/2019 04:26, Paolo Bonzini wrote:
> On 07/02/19 12:49, Dr. David Alan Gilbert wrote:
>>  //#define DEBUG_UNASSIGNED
>> @@ -2924,6 +2926,8 @@ struct FlatViewInfo {
>>      int counter;
>>      bool dispatch_tree;
>>      bool owner;
>> +    AccelClass *ac;
>> +    const char *ac_name;
>>  };
>>  
>>  static void mtree_print_flatview(gpointer key, gpointer value,
>> @@ -2939,6 +2943,7 @@ static void mtree_print_flatview(gpointer key, gpointer value,
>>      int n = view->nr;
>>      int i;
>>      AddressSpace *as;
>> +    bool system_as = false;
>>  
>>      p(f, "FlatView #%d\n", fvi->counter);
>>      ++fvi->counter;
>> @@ -2950,6 +2955,9 @@ static void mtree_print_flatview(gpointer key, gpointer value,
>>              p(f, ", alias %s", memory_region_name(as->root->alias));
>>          }
>>          p(f, "\n");
>> +        if (as == &address_space_memory) {
>> +            system_as = true;
>> +        }
>>      }
>>  
>>      p(f, " Root memory region: %s\n",
>> @@ -2985,6 +2993,13 @@ static void mtree_print_flatview(gpointer key, gpointer value,
>>          if (fvi->owner) {
>>              mtree_print_mr_owner(p, f, mr);
>>          }
>> +
>> +        if (system_as && fvi->ac &&
>> +            fvi->ac->has_memory(current_machine,
>> +                                int128_get64(range->addr.start),
>> +                                MR_SIZE(range->addr.size) + 1)) {
>> +            p(f, " %s", fvi->ac_name);
> 
> I don't understand this.  This doesn't check that the memory range
> actually matches the memory registered with the accelerator, only that
> there is something in that range. 


It is checking that a flat range (i.e. what actually works) has a
corresponding KVM slot:
https://git.qemu.org/?p=qemu.git;a=blob;f=accel/kvm/kvm-all.c;h=4e1de942ce554c734ac2673030031c228a752ac9;hb=HEAD#l201


> Why isn't it enough to use "info
> mtree" and look at the KVM address space?


There is no such thing in my QEMU, did you mean "KVM-SMRAM" (which is
missing on spapr)? I am not sure I understand its purpose for the task -
it prints all same ranges on my x86 laptop, not just ones which we told
KVM about.

My task is that if let's say "0000:00:1a.0 BAR 0 mmaps[0]" is split into
several sections because MSIX happens to be in a middle of that BAR and
it is not system page size aligned, then it is going to be several
ranges with no clear indication whether or not these were registered as
KVM slots. A memory chunk can be "ram" and not a KVM slot if it is 4K on
PPC with 64K system pages, for example.


FlatView #0
 AS "memory", root: system
 AS "cpu-memory-0", root: system
 AS "cpu-memory-1", root: system
 AS "cpu-memory-2", root: system
 AS "cpu-memory-3", root: system
 AS "piix3-ide", root: bus master container
 AS "virtio-net-pci", root: bus master container
 AS "vfio-pci", root: bus master container
 Root memory region: system
  0000000000000000-00000000000bffff (prio 0, ram): pc.ram kvm
  00000000000c0000-00000000000c0fff (prio 0, rom): pc.ram
@00000000000c0000 kvm
  00000000000c1000-00000000000c3fff (prio 0, ram): pc.ram
@00000000000c1000 kvm
  00000000000c4000-00000000000e7fff (prio 0, rom): pc.ram
@00000000000c4000 kvm
  00000000000e8000-00000000000effff (prio 0, ram): pc.ram
@00000000000e8000 kvm
  00000000000f0000-00000000000fffff (prio 0, rom): pc.ram
@00000000000f0000 kvm
  0000000000100000-00000000bfffffff (prio 0, ram): pc.ram
@0000000000100000 kvm
  00000000febc0000-00000000febc0fff (prio 0, ramd): 0000:00:1a.0 BAR 0
mmaps[0] kvm
  00000000febc1000-00000000febc102f (prio 0, i/o): msix-table
  00000000febc1800-00000000febc1807 (prio 0, i/o): msix-pba
  00000000febfc000-00000000febfcfff (prio 0, i/o): virtio-pci-common
  00000000febfd000-00000000febfdfff (prio 0, i/o): virtio-pci-isr
  00000000febfe000-00000000febfefff (prio 0, i/o): virtio-pci-device
  00000000febff000-00000000febfffff (prio 0, i/o): virtio-pci-notify
  00000000fec00000-00000000fec00fff (prio 0, i/o): kvm-ioapic
  00000000fed00000-00000000fed003ff (prio 0, i/o): hpet
  00000000fee00000-00000000feefffff (prio 4096, i/o): kvm-apic-msi
  00000000fffc0000-00000000ffffffff (prio 0, rom): pc.bios kvm
  0000000100000000-000000013fffffff (prio 0, ram): pc.ram
@00000000c0000000 kvm



FlatView #3
 AS "KVM-SMRAM", root: mem-container-smram
 Root memory region: mem-container-smram
  0000000000000000-00000000000bffff (prio 0, ram): pc.ram
  00000000000c0000-00000000000c0fff (prio 0, rom): pc.ram @00000000000c0000
  00000000000c1000-00000000000c3fff (prio 0, ram): pc.ram @00000000000c1000
  00000000000c4000-00000000000e7fff (prio 0, rom): pc.ram @00000000000c4000
  00000000000e8000-00000000000effff (prio 0, ram): pc.ram @00000000000e8000
  00000000000f0000-00000000000fffff (prio 0, rom): pc.ram @00000000000f0000
  0000000000100000-00000000bfffffff (prio 0, ram): pc.ram @0000000000100000
  00000000febc0000-00000000febc0fff (prio 0, ramd): 0000:00:1a.0 BAR 0
mmaps[0]
  00000000febc1000-00000000febc102f (prio 0, i/o): msix-table
  00000000febc1800-00000000febc1807 (prio 0, i/o): msix-pba
  00000000febfc000-00000000febfcfff (prio 0, i/o): virtio-pci-common
  00000000febfd000-00000000febfdfff (prio 0, i/o): virtio-pci-isr
  00000000febfe000-00000000febfefff (prio 0, i/o): virtio-pci-device
  00000000febff000-00000000febfffff (prio 0, i/o): virtio-pci-notify
  00000000fec00000-00000000fec00fff (prio 0, i/o): kvm-ioapic
  00000000fed00000-00000000fed003ff (prio 0, i/o): hpet
  00000000fee00000-00000000feefffff (prio 4096, i/o): kvm-apic-msi
  00000000fffc0000-00000000ffffffff (prio 0, rom): pc.bios
  0000000100000000-000000013fffffff (prio 0, ram): pc.ram @00000000c0000000




> Perhaps you could add instead an argument to "info mtree" that prints
> only the AddressSpace with a given name?

Nah, this is not what it is about.

On 11/02/2019 15:56, Alexey Kardashevskiy wrote:
> 
> 
> On 09/02/2019 04:26, Paolo Bonzini wrote:
>> On 07/02/19 12:49, Dr. David Alan Gilbert wrote:
>>>  //#define DEBUG_UNASSIGNED
>>> @@ -2924,6 +2926,8 @@ struct FlatViewInfo {
>>>      int counter;
>>>      bool dispatch_tree;
>>>      bool owner;
>>> +    AccelClass *ac;
>>> +    const char *ac_name;
>>>  };
>>>  
>>>  static void mtree_print_flatview(gpointer key, gpointer value,
>>> @@ -2939,6 +2943,7 @@ static void mtree_print_flatview(gpointer key, gpointer value,
>>>      int n = view->nr;
>>>      int i;
>>>      AddressSpace *as;
>>> +    bool system_as = false;
>>>  
>>>      p(f, "FlatView #%d\n", fvi->counter);
>>>      ++fvi->counter;
>>> @@ -2950,6 +2955,9 @@ static void mtree_print_flatview(gpointer key, gpointer value,
>>>              p(f, ", alias %s", memory_region_name(as->root->alias));
>>>          }
>>>          p(f, "\n");
>>> +        if (as == &address_space_memory) {
>>> +            system_as = true;
>>> +        }
>>>      }
>>>  
>>>      p(f, " Root memory region: %s\n",
>>> @@ -2985,6 +2993,13 @@ static void mtree_print_flatview(gpointer key, gpointer value,
>>>          if (fvi->owner) {
>>>              mtree_print_mr_owner(p, f, mr);
>>>          }
>>> +
>>> +        if (system_as && fvi->ac &&
>>> +            fvi->ac->has_memory(current_machine,
>>> +                                int128_get64(range->addr.start),
>>> +                                MR_SIZE(range->addr.size) + 1)) {
>>> +            p(f, " %s", fvi->ac_name);
>>
>> I don't understand this.  This doesn't check that the memory range
>> actually matches the memory registered with the accelerator, only that
>> there is something in that range. 
> 
> 
> It is checking that a flat range (i.e. what actually works) has a
> corresponding KVM slot:
> https://git.qemu.org/?p=qemu.git;a=blob;f=accel/kvm/kvm-all.c;h=4e1de942ce554c734ac2673030031c228a752ac9;hb=HEAD#l201
> 
> 
>> Why isn't it enough to use "info
>> mtree" and look at the KVM address space?
> 
> 
> There is no such thing in my QEMU, did you mean "KVM-SMRAM" (which is
> missing on spapr)? I am not sure I understand its purpose for the task -
> it prints all same ranges on my x86 laptop, not just ones which we told
> KVM about.
> 
> My task is that if let's say "0000:00:1a.0 BAR 0 mmaps[0]" is split into
> several sections because MSIX happens to be in a middle of that BAR and
> it is not system page size aligned, then it is going to be several
> ranges with no clear indication whether or not these were registered as
> KVM slots. A memory chunk can be "ram" and not a KVM slot if it is 4K on
> PPC with 64K system pages, for example.
> 
> 
> FlatView #0
>  AS "memory", root: system
>  AS "cpu-memory-0", root: system
>  AS "cpu-memory-1", root: system
>  AS "cpu-memory-2", root: system
>  AS "cpu-memory-3", root: system
>  AS "piix3-ide", root: bus master container
>  AS "virtio-net-pci", root: bus master container
>  AS "vfio-pci", root: bus master container
>  Root memory region: system
>   0000000000000000-00000000000bffff (prio 0, ram): pc.ram kvm
>   00000000000c0000-00000000000c0fff (prio 0, rom): pc.ram
> @00000000000c0000 kvm
>   00000000000c1000-00000000000c3fff (prio 0, ram): pc.ram
> @00000000000c1000 kvm
>   00000000000c4000-00000000000e7fff (prio 0, rom): pc.ram
> @00000000000c4000 kvm
>   00000000000e8000-00000000000effff (prio 0, ram): pc.ram
> @00000000000e8000 kvm
>   00000000000f0000-00000000000fffff (prio 0, rom): pc.ram
> @00000000000f0000 kvm
>   0000000000100000-00000000bfffffff (prio 0, ram): pc.ram
> @0000000000100000 kvm
>   00000000febc0000-00000000febc0fff (prio 0, ramd): 0000:00:1a.0 BAR 0
> mmaps[0] kvm
>   00000000febc1000-00000000febc102f (prio 0, i/o): msix-table
>   00000000febc1800-00000000febc1807 (prio 0, i/o): msix-pba
>   00000000febfc000-00000000febfcfff (prio 0, i/o): virtio-pci-common
>   00000000febfd000-00000000febfdfff (prio 0, i/o): virtio-pci-isr
>   00000000febfe000-00000000febfefff (prio 0, i/o): virtio-pci-device
>   00000000febff000-00000000febfffff (prio 0, i/o): virtio-pci-notify
>   00000000fec00000-00000000fec00fff (prio 0, i/o): kvm-ioapic
>   00000000fed00000-00000000fed003ff (prio 0, i/o): hpet
>   00000000fee00000-00000000feefffff (prio 4096, i/o): kvm-apic-msi
>   00000000fffc0000-00000000ffffffff (prio 0, rom): pc.bios kvm
>   0000000100000000-000000013fffffff (prio 0, ram): pc.ram
> @00000000c0000000 kvm
> 
> 
> 
> FlatView #3
>  AS "KVM-SMRAM", root: mem-container-smram
>  Root memory region: mem-container-smram
>   0000000000000000-00000000000bffff (prio 0, ram): pc.ram
>   00000000000c0000-00000000000c0fff (prio 0, rom): pc.ram @00000000000c0000
>   00000000000c1000-00000000000c3fff (prio 0, ram): pc.ram @00000000000c1000
>   00000000000c4000-00000000000e7fff (prio 0, rom): pc.ram @00000000000c4000
>   00000000000e8000-00000000000effff (prio 0, ram): pc.ram @00000000000e8000
>   00000000000f0000-00000000000fffff (prio 0, rom): pc.ram @00000000000f0000
>   0000000000100000-00000000bfffffff (prio 0, ram): pc.ram @0000000000100000
>   00000000febc0000-00000000febc0fff (prio 0, ramd): 0000:00:1a.0 BAR 0
> mmaps[0]
>   00000000febc1000-00000000febc102f (prio 0, i/o): msix-table
>   00000000febc1800-00000000febc1807 (prio 0, i/o): msix-pba
>   00000000febfc000-00000000febfcfff (prio 0, i/o): virtio-pci-common
>   00000000febfd000-00000000febfdfff (prio 0, i/o): virtio-pci-isr
>   00000000febfe000-00000000febfefff (prio 0, i/o): virtio-pci-device
>   00000000febff000-00000000febfffff (prio 0, i/o): virtio-pci-notify
>   00000000fec00000-00000000fec00fff (prio 0, i/o): kvm-ioapic
>   00000000fed00000-00000000fed003ff (prio 0, i/o): hpet
>   00000000fee00000-00000000feefffff (prio 4096, i/o): kvm-apic-msi
>   00000000fffc0000-00000000ffffffff (prio 0, rom): pc.bios
>   0000000100000000-000000013fffffff (prio 0, ram): pc.ram @00000000c0000000
> 
> 
> 
> 
>> Perhaps you could add instead an argument to "info mtree" that prints
>> only the AddressSpace with a given name?
> 
> Nah, this is not what it is about.



Still a nack? :)

Paolo, ping?


On 19/03/2019 18:05, Alexey Kardashevskiy wrote:
> 
> 
> On 11/02/2019 15:56, Alexey Kardashevskiy wrote:
>>
>>
>> On 09/02/2019 04:26, Paolo Bonzini wrote:
>>> On 07/02/19 12:49, Dr. David Alan Gilbert wrote:
>>>>  //#define DEBUG_UNASSIGNED
>>>> @@ -2924,6 +2926,8 @@ struct FlatViewInfo {
>>>>      int counter;
>>>>      bool dispatch_tree;
>>>>      bool owner;
>>>> +    AccelClass *ac;
>>>> +    const char *ac_name;
>>>>  };
>>>>  
>>>>  static void mtree_print_flatview(gpointer key, gpointer value,
>>>> @@ -2939,6 +2943,7 @@ static void mtree_print_flatview(gpointer key, gpointer value,
>>>>      int n = view->nr;
>>>>      int i;
>>>>      AddressSpace *as;
>>>> +    bool system_as = false;
>>>>  
>>>>      p(f, "FlatView #%d\n", fvi->counter);
>>>>      ++fvi->counter;
>>>> @@ -2950,6 +2955,9 @@ static void mtree_print_flatview(gpointer key, gpointer value,
>>>>              p(f, ", alias %s", memory_region_name(as->root->alias));
>>>>          }
>>>>          p(f, "\n");
>>>> +        if (as == &address_space_memory) {
>>>> +            system_as = true;
>>>> +        }
>>>>      }
>>>>  
>>>>      p(f, " Root memory region: %s\n",
>>>> @@ -2985,6 +2993,13 @@ static void mtree_print_flatview(gpointer key, gpointer value,
>>>>          if (fvi->owner) {
>>>>              mtree_print_mr_owner(p, f, mr);
>>>>          }
>>>> +
>>>> +        if (system_as && fvi->ac &&
>>>> +            fvi->ac->has_memory(current_machine,
>>>> +                                int128_get64(range->addr.start),
>>>> +                                MR_SIZE(range->addr.size) + 1)) {
>>>> +            p(f, " %s", fvi->ac_name);
>>>
>>> I don't understand this.  This doesn't check that the memory range
>>> actually matches the memory registered with the accelerator, only that
>>> there is something in that range. 
>>
>>
>> It is checking that a flat range (i.e. what actually works) has a
>> corresponding KVM slot:
>> https://git.qemu.org/?p=qemu.git;a=blob;f=accel/kvm/kvm-all.c;h=4e1de942ce554c734ac2673030031c228a752ac9;hb=HEAD#l201
>>
>>
>>> Why isn't it enough to use "info
>>> mtree" and look at the KVM address space?
>>
>>
>> There is no such thing in my QEMU, did you mean "KVM-SMRAM" (which is
>> missing on spapr)? I am not sure I understand its purpose for the task -
>> it prints all same ranges on my x86 laptop, not just ones which we told
>> KVM about.
>>
>> My task is that if let's say "0000:00:1a.0 BAR 0 mmaps[0]" is split into
>> several sections because MSIX happens to be in a middle of that BAR and
>> it is not system page size aligned, then it is going to be several
>> ranges with no clear indication whether or not these were registered as
>> KVM slots. A memory chunk can be "ram" and not a KVM slot if it is 4K on
>> PPC with 64K system pages, for example.
>>
>>
>> FlatView #0
>>  AS "memory", root: system
>>  AS "cpu-memory-0", root: system
>>  AS "cpu-memory-1", root: system
>>  AS "cpu-memory-2", root: system
>>  AS "cpu-memory-3", root: system
>>  AS "piix3-ide", root: bus master container
>>  AS "virtio-net-pci", root: bus master container
>>  AS "vfio-pci", root: bus master container
>>  Root memory region: system
>>   0000000000000000-00000000000bffff (prio 0, ram): pc.ram kvm
>>   00000000000c0000-00000000000c0fff (prio 0, rom): pc.ram
>> @00000000000c0000 kvm
>>   00000000000c1000-00000000000c3fff (prio 0, ram): pc.ram
>> @00000000000c1000 kvm
>>   00000000000c4000-00000000000e7fff (prio 0, rom): pc.ram
>> @00000000000c4000 kvm
>>   00000000000e8000-00000000000effff (prio 0, ram): pc.ram
>> @00000000000e8000 kvm
>>   00000000000f0000-00000000000fffff (prio 0, rom): pc.ram
>> @00000000000f0000 kvm
>>   0000000000100000-00000000bfffffff (prio 0, ram): pc.ram
>> @0000000000100000 kvm
>>   00000000febc0000-00000000febc0fff (prio 0, ramd): 0000:00:1a.0 BAR 0
>> mmaps[0] kvm
>>   00000000febc1000-00000000febc102f (prio 0, i/o): msix-table
>>   00000000febc1800-00000000febc1807 (prio 0, i/o): msix-pba
>>   00000000febfc000-00000000febfcfff (prio 0, i/o): virtio-pci-common
>>   00000000febfd000-00000000febfdfff (prio 0, i/o): virtio-pci-isr
>>   00000000febfe000-00000000febfefff (prio 0, i/o): virtio-pci-device
>>   00000000febff000-00000000febfffff (prio 0, i/o): virtio-pci-notify
>>   00000000fec00000-00000000fec00fff (prio 0, i/o): kvm-ioapic
>>   00000000fed00000-00000000fed003ff (prio 0, i/o): hpet
>>   00000000fee00000-00000000feefffff (prio 4096, i/o): kvm-apic-msi
>>   00000000fffc0000-00000000ffffffff (prio 0, rom): pc.bios kvm
>>   0000000100000000-000000013fffffff (prio 0, ram): pc.ram
>> @00000000c0000000 kvm
>>
>>
>>
>> FlatView #3
>>  AS "KVM-SMRAM", root: mem-container-smram
>>  Root memory region: mem-container-smram
>>   0000000000000000-00000000000bffff (prio 0, ram): pc.ram
>>   00000000000c0000-00000000000c0fff (prio 0, rom): pc.ram @00000000000c0000
>>   00000000000c1000-00000000000c3fff (prio 0, ram): pc.ram @00000000000c1000
>>   00000000000c4000-00000000000e7fff (prio 0, rom): pc.ram @00000000000c4000
>>   00000000000e8000-00000000000effff (prio 0, ram): pc.ram @00000000000e8000
>>   00000000000f0000-00000000000fffff (prio 0, rom): pc.ram @00000000000f0000
>>   0000000000100000-00000000bfffffff (prio 0, ram): pc.ram @0000000000100000
>>   00000000febc0000-00000000febc0fff (prio 0, ramd): 0000:00:1a.0 BAR 0
>> mmaps[0]
>>   00000000febc1000-00000000febc102f (prio 0, i/o): msix-table
>>   00000000febc1800-00000000febc1807 (prio 0, i/o): msix-pba
>>   00000000febfc000-00000000febfcfff (prio 0, i/o): virtio-pci-common
>>   00000000febfd000-00000000febfdfff (prio 0, i/o): virtio-pci-isr
>>   00000000febfe000-00000000febfefff (prio 0, i/o): virtio-pci-device
>>   00000000febff000-00000000febfffff (prio 0, i/o): virtio-pci-notify
>>   00000000fec00000-00000000fec00fff (prio 0, i/o): kvm-ioapic
>>   00000000fed00000-00000000fed003ff (prio 0, i/o): hpet
>>   00000000fee00000-00000000feefffff (prio 4096, i/o): kvm-apic-msi
>>   00000000fffc0000-00000000ffffffff (prio 0, rom): pc.bios
>>   0000000100000000-000000013fffffff (prio 0, ram): pc.ram @00000000c0000000
>>
>>
>>
>>
>>> Perhaps you could add instead an argument to "info mtree" that prints
>>> only the AddressSpace with a given name?
>>
>> Nah, this is not what it is about.
> 
> 
> 
> Still a nack? :)

Ping, anyone? I still enjoy seeing "kvm" next to MRs in "info mtree -f"
in my local QEMU :)



On 24/04/2019 15:32, Alexey Kardashevskiy wrote:
> Paolo, ping?
> 
> 
> On 19/03/2019 18:05, Alexey Kardashevskiy wrote:
>>
>>
>> On 11/02/2019 15:56, Alexey Kardashevskiy wrote:
>>>
>>>
>>> On 09/02/2019 04:26, Paolo Bonzini wrote:
>>>> On 07/02/19 12:49, Dr. David Alan Gilbert wrote:
>>>>>  //#define DEBUG_UNASSIGNED
>>>>> @@ -2924,6 +2926,8 @@ struct FlatViewInfo {
>>>>>      int counter;
>>>>>      bool dispatch_tree;
>>>>>      bool owner;
>>>>> +    AccelClass *ac;
>>>>> +    const char *ac_name;
>>>>>  };
>>>>>  
>>>>>  static void mtree_print_flatview(gpointer key, gpointer value,
>>>>> @@ -2939,6 +2943,7 @@ static void mtree_print_flatview(gpointer key, gpointer value,
>>>>>      int n = view->nr;
>>>>>      int i;
>>>>>      AddressSpace *as;
>>>>> +    bool system_as = false;
>>>>>  
>>>>>      p(f, "FlatView #%d\n", fvi->counter);
>>>>>      ++fvi->counter;
>>>>> @@ -2950,6 +2955,9 @@ static void mtree_print_flatview(gpointer key, gpointer value,
>>>>>              p(f, ", alias %s", memory_region_name(as->root->alias));
>>>>>          }
>>>>>          p(f, "\n");
>>>>> +        if (as == &address_space_memory) {
>>>>> +            system_as = true;
>>>>> +        }
>>>>>      }
>>>>>  
>>>>>      p(f, " Root memory region: %s\n",
>>>>> @@ -2985,6 +2993,13 @@ static void mtree_print_flatview(gpointer key, gpointer value,
>>>>>          if (fvi->owner) {
>>>>>              mtree_print_mr_owner(p, f, mr);
>>>>>          }
>>>>> +
>>>>> +        if (system_as && fvi->ac &&
>>>>> +            fvi->ac->has_memory(current_machine,
>>>>> +                                int128_get64(range->addr.start),
>>>>> +                                MR_SIZE(range->addr.size) + 1)) {
>>>>> +            p(f, " %s", fvi->ac_name);
>>>>
>>>> I don't understand this.  This doesn't check that the memory range
>>>> actually matches the memory registered with the accelerator, only that
>>>> there is something in that range. 
>>>
>>>
>>> It is checking that a flat range (i.e. what actually works) has a
>>> corresponding KVM slot:
>>> https://git.qemu.org/?p=qemu.git;a=blob;f=accel/kvm/kvm-all.c;h=4e1de942ce554c734ac2673030031c228a752ac9;hb=HEAD#l201
>>>
>>>
>>>> Why isn't it enough to use "info
>>>> mtree" and look at the KVM address space?
>>>
>>>
>>> There is no such thing in my QEMU, did you mean "KVM-SMRAM" (which is
>>> missing on spapr)? I am not sure I understand its purpose for the task -
>>> it prints all same ranges on my x86 laptop, not just ones which we told
>>> KVM about.
>>>
>>> My task is that if let's say "0000:00:1a.0 BAR 0 mmaps[0]" is split into
>>> several sections because MSIX happens to be in a middle of that BAR and
>>> it is not system page size aligned, then it is going to be several
>>> ranges with no clear indication whether or not these were registered as
>>> KVM slots. A memory chunk can be "ram" and not a KVM slot if it is 4K on
>>> PPC with 64K system pages, for example.
>>>
>>>
>>> FlatView #0
>>>  AS "memory", root: system
>>>  AS "cpu-memory-0", root: system
>>>  AS "cpu-memory-1", root: system
>>>  AS "cpu-memory-2", root: system
>>>  AS "cpu-memory-3", root: system
>>>  AS "piix3-ide", root: bus master container
>>>  AS "virtio-net-pci", root: bus master container
>>>  AS "vfio-pci", root: bus master container
>>>  Root memory region: system
>>>   0000000000000000-00000000000bffff (prio 0, ram): pc.ram kvm
>>>   00000000000c0000-00000000000c0fff (prio 0, rom): pc.ram
>>> @00000000000c0000 kvm
>>>   00000000000c1000-00000000000c3fff (prio 0, ram): pc.ram
>>> @00000000000c1000 kvm
>>>   00000000000c4000-00000000000e7fff (prio 0, rom): pc.ram
>>> @00000000000c4000 kvm
>>>   00000000000e8000-00000000000effff (prio 0, ram): pc.ram
>>> @00000000000e8000 kvm
>>>   00000000000f0000-00000000000fffff (prio 0, rom): pc.ram
>>> @00000000000f0000 kvm
>>>   0000000000100000-00000000bfffffff (prio 0, ram): pc.ram
>>> @0000000000100000 kvm
>>>   00000000febc0000-00000000febc0fff (prio 0, ramd): 0000:00:1a.0 BAR 0
>>> mmaps[0] kvm
>>>   00000000febc1000-00000000febc102f (prio 0, i/o): msix-table
>>>   00000000febc1800-00000000febc1807 (prio 0, i/o): msix-pba
>>>   00000000febfc000-00000000febfcfff (prio 0, i/o): virtio-pci-common
>>>   00000000febfd000-00000000febfdfff (prio 0, i/o): virtio-pci-isr
>>>   00000000febfe000-00000000febfefff (prio 0, i/o): virtio-pci-device
>>>   00000000febff000-00000000febfffff (prio 0, i/o): virtio-pci-notify
>>>   00000000fec00000-00000000fec00fff (prio 0, i/o): kvm-ioapic
>>>   00000000fed00000-00000000fed003ff (prio 0, i/o): hpet
>>>   00000000fee00000-00000000feefffff (prio 4096, i/o): kvm-apic-msi
>>>   00000000fffc0000-00000000ffffffff (prio 0, rom): pc.bios kvm
>>>   0000000100000000-000000013fffffff (prio 0, ram): pc.ram
>>> @00000000c0000000 kvm
>>>
>>>
>>>
>>> FlatView #3
>>>  AS "KVM-SMRAM", root: mem-container-smram
>>>  Root memory region: mem-container-smram
>>>   0000000000000000-00000000000bffff (prio 0, ram): pc.ram
>>>   00000000000c0000-00000000000c0fff (prio 0, rom): pc.ram @00000000000c0000
>>>   00000000000c1000-00000000000c3fff (prio 0, ram): pc.ram @00000000000c1000
>>>   00000000000c4000-00000000000e7fff (prio 0, rom): pc.ram @00000000000c4000
>>>   00000000000e8000-00000000000effff (prio 0, ram): pc.ram @00000000000e8000
>>>   00000000000f0000-00000000000fffff (prio 0, rom): pc.ram @00000000000f0000
>>>   0000000000100000-00000000bfffffff (prio 0, ram): pc.ram @0000000000100000
>>>   00000000febc0000-00000000febc0fff (prio 0, ramd): 0000:00:1a.0 BAR 0
>>> mmaps[0]
>>>   00000000febc1000-00000000febc102f (prio 0, i/o): msix-table
>>>   00000000febc1800-00000000febc1807 (prio 0, i/o): msix-pba
>>>   00000000febfc000-00000000febfcfff (prio 0, i/o): virtio-pci-common
>>>   00000000febfd000-00000000febfdfff (prio 0, i/o): virtio-pci-isr
>>>   00000000febfe000-00000000febfefff (prio 0, i/o): virtio-pci-device
>>>   00000000febff000-00000000febfffff (prio 0, i/o): virtio-pci-notify
>>>   00000000fec00000-00000000fec00fff (prio 0, i/o): kvm-ioapic
>>>   00000000fed00000-00000000fed003ff (prio 0, i/o): hpet
>>>   00000000fee00000-00000000feefffff (prio 4096, i/o): kvm-apic-msi
>>>   00000000fffc0000-00000000ffffffff (prio 0, rom): pc.bios
>>>   0000000100000000-000000013fffffff (prio 0, ram): pc.ram @00000000c0000000
>>>
>>>
>>>
>>>
>>>> Perhaps you could add instead an argument to "info mtree" that prints
>>>> only the AddressSpace with a given name?
>>>
>>> Nah, this is not what it is about.
>>
>>
>>
>> Still a nack? :)
> 
> 
>

On 21/05/2019 16:37, Alexey Kardashevskiy wrote:
> Ping, anyone? I still enjoy seeing "kvm" next to MRs in "info mtree -f"
> in my local QEMU :)


"Still feeling it" :)
Ping?


> 
> 
> On 24/04/2019 15:32, Alexey Kardashevskiy wrote:
>> Paolo, ping?
>>
>>
>> On 19/03/2019 18:05, Alexey Kardashevskiy wrote:
>>>
>>>
>>> On 11/02/2019 15:56, Alexey Kardashevskiy wrote:
>>>>
>>>>
>>>> On 09/02/2019 04:26, Paolo Bonzini wrote:
>>>>> On 07/02/19 12:49, Dr. David Alan Gilbert wrote:
>>>>>>  //#define DEBUG_UNASSIGNED
>>>>>> @@ -2924,6 +2926,8 @@ struct FlatViewInfo {
>>>>>>      int counter;
>>>>>>      bool dispatch_tree;
>>>>>>      bool owner;
>>>>>> +    AccelClass *ac;
>>>>>> +    const char *ac_name;
>>>>>>  };
>>>>>>  
>>>>>>  static void mtree_print_flatview(gpointer key, gpointer value,
>>>>>> @@ -2939,6 +2943,7 @@ static void mtree_print_flatview(gpointer key, gpointer value,
>>>>>>      int n = view->nr;
>>>>>>      int i;
>>>>>>      AddressSpace *as;
>>>>>> +    bool system_as = false;
>>>>>>  
>>>>>>      p(f, "FlatView #%d\n", fvi->counter);
>>>>>>      ++fvi->counter;
>>>>>> @@ -2950,6 +2955,9 @@ static void mtree_print_flatview(gpointer key, gpointer value,
>>>>>>              p(f, ", alias %s", memory_region_name(as->root->alias));
>>>>>>          }
>>>>>>          p(f, "\n");
>>>>>> +        if (as == &address_space_memory) {
>>>>>> +            system_as = true;
>>>>>> +        }
>>>>>>      }
>>>>>>  
>>>>>>      p(f, " Root memory region: %s\n",
>>>>>> @@ -2985,6 +2993,13 @@ static void mtree_print_flatview(gpointer key, gpointer value,
>>>>>>          if (fvi->owner) {
>>>>>>              mtree_print_mr_owner(p, f, mr);
>>>>>>          }
>>>>>> +
>>>>>> +        if (system_as && fvi->ac &&
>>>>>> +            fvi->ac->has_memory(current_machine,
>>>>>> +                                int128_get64(range->addr.start),
>>>>>> +                                MR_SIZE(range->addr.size) + 1)) {
>>>>>> +            p(f, " %s", fvi->ac_name);
>>>>>
>>>>> I don't understand this.  This doesn't check that the memory range
>>>>> actually matches the memory registered with the accelerator, only that
>>>>> there is something in that range. 
>>>>
>>>>
>>>> It is checking that a flat range (i.e. what actually works) has a
>>>> corresponding KVM slot:
>>>> https://git.qemu.org/?p=qemu.git;a=blob;f=accel/kvm/kvm-all.c;h=4e1de942ce554c734ac2673030031c228a752ac9;hb=HEAD#l201
>>>>
>>>>
>>>>> Why isn't it enough to use "info
>>>>> mtree" and look at the KVM address space?
>>>>
>>>>
>>>> There is no such thing in my QEMU, did you mean "KVM-SMRAM" (which is
>>>> missing on spapr)? I am not sure I understand its purpose for the task -
>>>> it prints all same ranges on my x86 laptop, not just ones which we told
>>>> KVM about.
>>>>
>>>> My task is that if let's say "0000:00:1a.0 BAR 0 mmaps[0]" is split into
>>>> several sections because MSIX happens to be in a middle of that BAR and
>>>> it is not system page size aligned, then it is going to be several
>>>> ranges with no clear indication whether or not these were registered as
>>>> KVM slots. A memory chunk can be "ram" and not a KVM slot if it is 4K on
>>>> PPC with 64K system pages, for example.
>>>>
>>>>
>>>> FlatView #0
>>>>  AS "memory", root: system
>>>>  AS "cpu-memory-0", root: system
>>>>  AS "cpu-memory-1", root: system
>>>>  AS "cpu-memory-2", root: system
>>>>  AS "cpu-memory-3", root: system
>>>>  AS "piix3-ide", root: bus master container
>>>>  AS "virtio-net-pci", root: bus master container
>>>>  AS "vfio-pci", root: bus master container
>>>>  Root memory region: system
>>>>   0000000000000000-00000000000bffff (prio 0, ram): pc.ram kvm
>>>>   00000000000c0000-00000000000c0fff (prio 0, rom): pc.ram
>>>> @00000000000c0000 kvm
>>>>   00000000000c1000-00000000000c3fff (prio 0, ram): pc.ram
>>>> @00000000000c1000 kvm
>>>>   00000000000c4000-00000000000e7fff (prio 0, rom): pc.ram
>>>> @00000000000c4000 kvm
>>>>   00000000000e8000-00000000000effff (prio 0, ram): pc.ram
>>>> @00000000000e8000 kvm
>>>>   00000000000f0000-00000000000fffff (prio 0, rom): pc.ram
>>>> @00000000000f0000 kvm
>>>>   0000000000100000-00000000bfffffff (prio 0, ram): pc.ram
>>>> @0000000000100000 kvm
>>>>   00000000febc0000-00000000febc0fff (prio 0, ramd): 0000:00:1a.0 BAR 0
>>>> mmaps[0] kvm
>>>>   00000000febc1000-00000000febc102f (prio 0, i/o): msix-table
>>>>   00000000febc1800-00000000febc1807 (prio 0, i/o): msix-pba
>>>>   00000000febfc000-00000000febfcfff (prio 0, i/o): virtio-pci-common
>>>>   00000000febfd000-00000000febfdfff (prio 0, i/o): virtio-pci-isr
>>>>   00000000febfe000-00000000febfefff (prio 0, i/o): virtio-pci-device
>>>>   00000000febff000-00000000febfffff (prio 0, i/o): virtio-pci-notify
>>>>   00000000fec00000-00000000fec00fff (prio 0, i/o): kvm-ioapic
>>>>   00000000fed00000-00000000fed003ff (prio 0, i/o): hpet
>>>>   00000000fee00000-00000000feefffff (prio 4096, i/o): kvm-apic-msi
>>>>   00000000fffc0000-00000000ffffffff (prio 0, rom): pc.bios kvm
>>>>   0000000100000000-000000013fffffff (prio 0, ram): pc.ram
>>>> @00000000c0000000 kvm
>>>>
>>>>
>>>>
>>>> FlatView #3
>>>>  AS "KVM-SMRAM", root: mem-container-smram
>>>>  Root memory region: mem-container-smram
>>>>   0000000000000000-00000000000bffff (prio 0, ram): pc.ram
>>>>   00000000000c0000-00000000000c0fff (prio 0, rom): pc.ram @00000000000c0000
>>>>   00000000000c1000-00000000000c3fff (prio 0, ram): pc.ram @00000000000c1000
>>>>   00000000000c4000-00000000000e7fff (prio 0, rom): pc.ram @00000000000c4000
>>>>   00000000000e8000-00000000000effff (prio 0, ram): pc.ram @00000000000e8000
>>>>   00000000000f0000-00000000000fffff (prio 0, rom): pc.ram @00000000000f0000
>>>>   0000000000100000-00000000bfffffff (prio 0, ram): pc.ram @0000000000100000
>>>>   00000000febc0000-00000000febc0fff (prio 0, ramd): 0000:00:1a.0 BAR 0
>>>> mmaps[0]
>>>>   00000000febc1000-00000000febc102f (prio 0, i/o): msix-table
>>>>   00000000febc1800-00000000febc1807 (prio 0, i/o): msix-pba
>>>>   00000000febfc000-00000000febfcfff (prio 0, i/o): virtio-pci-common
>>>>   00000000febfd000-00000000febfdfff (prio 0, i/o): virtio-pci-isr
>>>>   00000000febfe000-00000000febfefff (prio 0, i/o): virtio-pci-device
>>>>   00000000febff000-00000000febfffff (prio 0, i/o): virtio-pci-notify
>>>>   00000000fec00000-00000000fec00fff (prio 0, i/o): kvm-ioapic
>>>>   00000000fed00000-00000000fed003ff (prio 0, i/o): hpet
>>>>   00000000fee00000-00000000feefffff (prio 4096, i/o): kvm-apic-msi
>>>>   00000000fffc0000-00000000ffffffff (prio 0, rom): pc.bios
>>>>   0000000100000000-000000013fffffff (prio 0, ram): pc.ram @00000000c0000000
>>>>
>>>>
>>>>
>>>>
>>>>> Perhaps you could add instead an argument to "info mtree" that prints
>>>>> only the AddressSpace with a given name?
>>>>
>>>> Nah, this is not what it is about.
>>>
>>>
>>>
>>> Still a nack? :)
>>
>>
>>
>

* Alexey Kardashevskiy (aik@ozlabs.ru) wrote:
> 
> 
> On 21/05/2019 16:37, Alexey Kardashevskiy wrote:
> > Ping, anyone? I still enjoy seeing "kvm" next to MRs in "info mtree -f"
> > in my local QEMU :)
> 
> 
> "Still feeling it" :)
> Ping?

Fundamentally you've got to convince Paolo that it's the
right approach to identifying KVM RAM.

Dave

> 
> > 
> > 
> > On 24/04/2019 15:32, Alexey Kardashevskiy wrote:
> >> Paolo, ping?
> >>
> >>
> >> On 19/03/2019 18:05, Alexey Kardashevskiy wrote:
> >>>
> >>>
> >>> On 11/02/2019 15:56, Alexey Kardashevskiy wrote:
> >>>>
> >>>>
> >>>> On 09/02/2019 04:26, Paolo Bonzini wrote:
> >>>>> On 07/02/19 12:49, Dr. David Alan Gilbert wrote:
> >>>>>>  //#define DEBUG_UNASSIGNED
> >>>>>> @@ -2924,6 +2926,8 @@ struct FlatViewInfo {
> >>>>>>      int counter;
> >>>>>>      bool dispatch_tree;
> >>>>>>      bool owner;
> >>>>>> +    AccelClass *ac;
> >>>>>> +    const char *ac_name;
> >>>>>>  };
> >>>>>>  
> >>>>>>  static void mtree_print_flatview(gpointer key, gpointer value,
> >>>>>> @@ -2939,6 +2943,7 @@ static void mtree_print_flatview(gpointer key, gpointer value,
> >>>>>>      int n = view->nr;
> >>>>>>      int i;
> >>>>>>      AddressSpace *as;
> >>>>>> +    bool system_as = false;
> >>>>>>  
> >>>>>>      p(f, "FlatView #%d\n", fvi->counter);
> >>>>>>      ++fvi->counter;
> >>>>>> @@ -2950,6 +2955,9 @@ static void mtree_print_flatview(gpointer key, gpointer value,
> >>>>>>              p(f, ", alias %s", memory_region_name(as->root->alias));
> >>>>>>          }
> >>>>>>          p(f, "\n");
> >>>>>> +        if (as == &address_space_memory) {
> >>>>>> +            system_as = true;
> >>>>>> +        }
> >>>>>>      }
> >>>>>>  
> >>>>>>      p(f, " Root memory region: %s\n",
> >>>>>> @@ -2985,6 +2993,13 @@ static void mtree_print_flatview(gpointer key, gpointer value,
> >>>>>>          if (fvi->owner) {
> >>>>>>              mtree_print_mr_owner(p, f, mr);
> >>>>>>          }
> >>>>>> +
> >>>>>> +        if (system_as && fvi->ac &&
> >>>>>> +            fvi->ac->has_memory(current_machine,
> >>>>>> +                                int128_get64(range->addr.start),
> >>>>>> +                                MR_SIZE(range->addr.size) + 1)) {
> >>>>>> +            p(f, " %s", fvi->ac_name);
> >>>>>
> >>>>> I don't understand this.  This doesn't check that the memory range
> >>>>> actually matches the memory registered with the accelerator, only that
> >>>>> there is something in that range. 
> >>>>
> >>>>
> >>>> It is checking that a flat range (i.e. what actually works) has a
> >>>> corresponding KVM slot:
> >>>> https://git.qemu.org/?p=qemu.git;a=blob;f=accel/kvm/kvm-all.c;h=4e1de942ce554c734ac2673030031c228a752ac9;hb=HEAD#l201
> >>>>
> >>>>
> >>>>> Why isn't it enough to use "info
> >>>>> mtree" and look at the KVM address space?
> >>>>
> >>>>
> >>>> There is no such thing in my QEMU, did you mean "KVM-SMRAM" (which is
> >>>> missing on spapr)? I am not sure I understand its purpose for the task -
> >>>> it prints all same ranges on my x86 laptop, not just ones which we told
> >>>> KVM about.
> >>>>
> >>>> My task is that if let's say "0000:00:1a.0 BAR 0 mmaps[0]" is split into
> >>>> several sections because MSIX happens to be in a middle of that BAR and
> >>>> it is not system page size aligned, then it is going to be several
> >>>> ranges with no clear indication whether or not these were registered as
> >>>> KVM slots. A memory chunk can be "ram" and not a KVM slot if it is 4K on
> >>>> PPC with 64K system pages, for example.
> >>>>
> >>>>
> >>>> FlatView #0
> >>>>  AS "memory", root: system
> >>>>  AS "cpu-memory-0", root: system
> >>>>  AS "cpu-memory-1", root: system
> >>>>  AS "cpu-memory-2", root: system
> >>>>  AS "cpu-memory-3", root: system
> >>>>  AS "piix3-ide", root: bus master container
> >>>>  AS "virtio-net-pci", root: bus master container
> >>>>  AS "vfio-pci", root: bus master container
> >>>>  Root memory region: system
> >>>>   0000000000000000-00000000000bffff (prio 0, ram): pc.ram kvm
> >>>>   00000000000c0000-00000000000c0fff (prio 0, rom): pc.ram
> >>>> @00000000000c0000 kvm
> >>>>   00000000000c1000-00000000000c3fff (prio 0, ram): pc.ram
> >>>> @00000000000c1000 kvm
> >>>>   00000000000c4000-00000000000e7fff (prio 0, rom): pc.ram
> >>>> @00000000000c4000 kvm
> >>>>   00000000000e8000-00000000000effff (prio 0, ram): pc.ram
> >>>> @00000000000e8000 kvm
> >>>>   00000000000f0000-00000000000fffff (prio 0, rom): pc.ram
> >>>> @00000000000f0000 kvm
> >>>>   0000000000100000-00000000bfffffff (prio 0, ram): pc.ram
> >>>> @0000000000100000 kvm
> >>>>   00000000febc0000-00000000febc0fff (prio 0, ramd): 0000:00:1a.0 BAR 0
> >>>> mmaps[0] kvm
> >>>>   00000000febc1000-00000000febc102f (prio 0, i/o): msix-table
> >>>>   00000000febc1800-00000000febc1807 (prio 0, i/o): msix-pba
> >>>>   00000000febfc000-00000000febfcfff (prio 0, i/o): virtio-pci-common
> >>>>   00000000febfd000-00000000febfdfff (prio 0, i/o): virtio-pci-isr
> >>>>   00000000febfe000-00000000febfefff (prio 0, i/o): virtio-pci-device
> >>>>   00000000febff000-00000000febfffff (prio 0, i/o): virtio-pci-notify
> >>>>   00000000fec00000-00000000fec00fff (prio 0, i/o): kvm-ioapic
> >>>>   00000000fed00000-00000000fed003ff (prio 0, i/o): hpet
> >>>>   00000000fee00000-00000000feefffff (prio 4096, i/o): kvm-apic-msi
> >>>>   00000000fffc0000-00000000ffffffff (prio 0, rom): pc.bios kvm
> >>>>   0000000100000000-000000013fffffff (prio 0, ram): pc.ram
> >>>> @00000000c0000000 kvm
> >>>>
> >>>>
> >>>>
> >>>> FlatView #3
> >>>>  AS "KVM-SMRAM", root: mem-container-smram
> >>>>  Root memory region: mem-container-smram
> >>>>   0000000000000000-00000000000bffff (prio 0, ram): pc.ram
> >>>>   00000000000c0000-00000000000c0fff (prio 0, rom): pc.ram @00000000000c0000
> >>>>   00000000000c1000-00000000000c3fff (prio 0, ram): pc.ram @00000000000c1000
> >>>>   00000000000c4000-00000000000e7fff (prio 0, rom): pc.ram @00000000000c4000
> >>>>   00000000000e8000-00000000000effff (prio 0, ram): pc.ram @00000000000e8000
> >>>>   00000000000f0000-00000000000fffff (prio 0, rom): pc.ram @00000000000f0000
> >>>>   0000000000100000-00000000bfffffff (prio 0, ram): pc.ram @0000000000100000
> >>>>   00000000febc0000-00000000febc0fff (prio 0, ramd): 0000:00:1a.0 BAR 0
> >>>> mmaps[0]
> >>>>   00000000febc1000-00000000febc102f (prio 0, i/o): msix-table
> >>>>   00000000febc1800-00000000febc1807 (prio 0, i/o): msix-pba
> >>>>   00000000febfc000-00000000febfcfff (prio 0, i/o): virtio-pci-common
> >>>>   00000000febfd000-00000000febfdfff (prio 0, i/o): virtio-pci-isr
> >>>>   00000000febfe000-00000000febfefff (prio 0, i/o): virtio-pci-device
> >>>>   00000000febff000-00000000febfffff (prio 0, i/o): virtio-pci-notify
> >>>>   00000000fec00000-00000000fec00fff (prio 0, i/o): kvm-ioapic
> >>>>   00000000fed00000-00000000fed003ff (prio 0, i/o): hpet
> >>>>   00000000fee00000-00000000feefffff (prio 4096, i/o): kvm-apic-msi
> >>>>   00000000fffc0000-00000000ffffffff (prio 0, rom): pc.bios
> >>>>   0000000100000000-000000013fffffff (prio 0, ram): pc.ram @00000000c0000000
> >>>>
> >>>>
> >>>>
> >>>>
> >>>>> Perhaps you could add instead an argument to "info mtree" that prints
> >>>>> only the AddressSpace with a given name?
> >>>>
> >>>> Nah, this is not what it is about.
> >>>
> >>>
> >>>
> >>> Still a nack? :)
> >>
> >>
> >>
> > 
> 
> -- 
> Alexey
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

On 13/06/19 07:07, Alexey Kardashevskiy wrote:
> 
> 
> On 21/05/2019 16:37, Alexey Kardashevskiy wrote:
>> Ping, anyone? I still enjoy seeing "kvm" next to MRs in "info mtree -f"
>> in my local QEMU :)
> 
> 
> "Still feeling it" :)
> Ping?

Queued for persistence. :)

Paolo

> 
>>
>>
>> On 24/04/2019 15:32, Alexey Kardashevskiy wrote:
>>> Paolo, ping?
>>>
>>>
>>> On 19/03/2019 18:05, Alexey Kardashevskiy wrote:
>>>>
>>>>
>>>> On 11/02/2019 15:56, Alexey Kardashevskiy wrote:
>>>>>
>>>>>
>>>>> On 09/02/2019 04:26, Paolo Bonzini wrote:
>>>>>> On 07/02/19 12:49, Dr. David Alan Gilbert wrote:
>>>>>>>  //#define DEBUG_UNASSIGNED
>>>>>>> @@ -2924,6 +2926,8 @@ struct FlatViewInfo {
>>>>>>>      int counter;
>>>>>>>      bool dispatch_tree;
>>>>>>>      bool owner;
>>>>>>> +    AccelClass *ac;
>>>>>>> +    const char *ac_name;
>>>>>>>  };
>>>>>>>  
>>>>>>>  static void mtree_print_flatview(gpointer key, gpointer value,
>>>>>>> @@ -2939,6 +2943,7 @@ static void mtree_print_flatview(gpointer key, gpointer value,
>>>>>>>      int n = view->nr;
>>>>>>>      int i;
>>>>>>>      AddressSpace *as;
>>>>>>> +    bool system_as = false;
>>>>>>>  
>>>>>>>      p(f, "FlatView #%d\n", fvi->counter);
>>>>>>>      ++fvi->counter;
>>>>>>> @@ -2950,6 +2955,9 @@ static void mtree_print_flatview(gpointer key, gpointer value,
>>>>>>>              p(f, ", alias %s", memory_region_name(as->root->alias));
>>>>>>>          }
>>>>>>>          p(f, "\n");
>>>>>>> +        if (as == &address_space_memory) {
>>>>>>> +            system_as = true;
>>>>>>> +        }
>>>>>>>      }
>>>>>>>  
>>>>>>>      p(f, " Root memory region: %s\n",
>>>>>>> @@ -2985,6 +2993,13 @@ static void mtree_print_flatview(gpointer key, gpointer value,
>>>>>>>          if (fvi->owner) {
>>>>>>>              mtree_print_mr_owner(p, f, mr);
>>>>>>>          }
>>>>>>> +
>>>>>>> +        if (system_as && fvi->ac &&
>>>>>>> +            fvi->ac->has_memory(current_machine,
>>>>>>> +                                int128_get64(range->addr.start),
>>>>>>> +                                MR_SIZE(range->addr.size) + 1)) {
>>>>>>> +            p(f, " %s", fvi->ac_name);
>>>>>>
>>>>>> I don't understand this.  This doesn't check that the memory range
>>>>>> actually matches the memory registered with the accelerator, only that
>>>>>> there is something in that range. 
>>>>>
>>>>>
>>>>> It is checking that a flat range (i.e. what actually works) has a
>>>>> corresponding KVM slot:
>>>>> https://git.qemu.org/?p=qemu.git;a=blob;f=accel/kvm/kvm-all.c;h=4e1de942ce554c734ac2673030031c228a752ac9;hb=HEAD#l201
>>>>>
>>>>>
>>>>>> Why isn't it enough to use "info
>>>>>> mtree" and look at the KVM address space?
>>>>>
>>>>>
>>>>> There is no such thing in my QEMU, did you mean "KVM-SMRAM" (which is
>>>>> missing on spapr)? I am not sure I understand its purpose for the task -
>>>>> it prints all same ranges on my x86 laptop, not just ones which we told
>>>>> KVM about.
>>>>>
>>>>> My task is that if let's say "0000:00:1a.0 BAR 0 mmaps[0]" is split into
>>>>> several sections because MSIX happens to be in a middle of that BAR and
>>>>> it is not system page size aligned, then it is going to be several
>>>>> ranges with no clear indication whether or not these were registered as
>>>>> KVM slots. A memory chunk can be "ram" and not a KVM slot if it is 4K on
>>>>> PPC with 64K system pages, for example.
>>>>>
>>>>>
>>>>> FlatView #0
>>>>>  AS "memory", root: system
>>>>>  AS "cpu-memory-0", root: system
>>>>>  AS "cpu-memory-1", root: system
>>>>>  AS "cpu-memory-2", root: system
>>>>>  AS "cpu-memory-3", root: system
>>>>>  AS "piix3-ide", root: bus master container
>>>>>  AS "virtio-net-pci", root: bus master container
>>>>>  AS "vfio-pci", root: bus master container
>>>>>  Root memory region: system
>>>>>   0000000000000000-00000000000bffff (prio 0, ram): pc.ram kvm
>>>>>   00000000000c0000-00000000000c0fff (prio 0, rom): pc.ram
>>>>> @00000000000c0000 kvm
>>>>>   00000000000c1000-00000000000c3fff (prio 0, ram): pc.ram
>>>>> @00000000000c1000 kvm
>>>>>   00000000000c4000-00000000000e7fff (prio 0, rom): pc.ram
>>>>> @00000000000c4000 kvm
>>>>>   00000000000e8000-00000000000effff (prio 0, ram): pc.ram
>>>>> @00000000000e8000 kvm
>>>>>   00000000000f0000-00000000000fffff (prio 0, rom): pc.ram
>>>>> @00000000000f0000 kvm
>>>>>   0000000000100000-00000000bfffffff (prio 0, ram): pc.ram
>>>>> @0000000000100000 kvm
>>>>>   00000000febc0000-00000000febc0fff (prio 0, ramd): 0000:00:1a.0 BAR 0
>>>>> mmaps[0] kvm
>>>>>   00000000febc1000-00000000febc102f (prio 0, i/o): msix-table
>>>>>   00000000febc1800-00000000febc1807 (prio 0, i/o): msix-pba
>>>>>   00000000febfc000-00000000febfcfff (prio 0, i/o): virtio-pci-common
>>>>>   00000000febfd000-00000000febfdfff (prio 0, i/o): virtio-pci-isr
>>>>>   00000000febfe000-00000000febfefff (prio 0, i/o): virtio-pci-device
>>>>>   00000000febff000-00000000febfffff (prio 0, i/o): virtio-pci-notify
>>>>>   00000000fec00000-00000000fec00fff (prio 0, i/o): kvm-ioapic
>>>>>   00000000fed00000-00000000fed003ff (prio 0, i/o): hpet
>>>>>   00000000fee00000-00000000feefffff (prio 4096, i/o): kvm-apic-msi
>>>>>   00000000fffc0000-00000000ffffffff (prio 0, rom): pc.bios kvm
>>>>>   0000000100000000-000000013fffffff (prio 0, ram): pc.ram
>>>>> @00000000c0000000 kvm
>>>>>
>>>>>
>>>>>
>>>>> FlatView #3
>>>>>  AS "KVM-SMRAM", root: mem-container-smram
>>>>>  Root memory region: mem-container-smram
>>>>>   0000000000000000-00000000000bffff (prio 0, ram): pc.ram
>>>>>   00000000000c0000-00000000000c0fff (prio 0, rom): pc.ram @00000000000c0000
>>>>>   00000000000c1000-00000000000c3fff (prio 0, ram): pc.ram @00000000000c1000
>>>>>   00000000000c4000-00000000000e7fff (prio 0, rom): pc.ram @00000000000c4000
>>>>>   00000000000e8000-00000000000effff (prio 0, ram): pc.ram @00000000000e8000
>>>>>   00000000000f0000-00000000000fffff (prio 0, rom): pc.ram @00000000000f0000
>>>>>   0000000000100000-00000000bfffffff (prio 0, ram): pc.ram @0000000000100000
>>>>>   00000000febc0000-00000000febc0fff (prio 0, ramd): 0000:00:1a.0 BAR 0
>>>>> mmaps[0]
>>>>>   00000000febc1000-00000000febc102f (prio 0, i/o): msix-table
>>>>>   00000000febc1800-00000000febc1807 (prio 0, i/o): msix-pba
>>>>>   00000000febfc000-00000000febfcfff (prio 0, i/o): virtio-pci-common
>>>>>   00000000febfd000-00000000febfdfff (prio 0, i/o): virtio-pci-isr
>>>>>   00000000febfe000-00000000febfefff (prio 0, i/o): virtio-pci-device
>>>>>   00000000febff000-00000000febfffff (prio 0, i/o): virtio-pci-notify
>>>>>   00000000fec00000-00000000fec00fff (prio 0, i/o): kvm-ioapic
>>>>>   00000000fed00000-00000000fed003ff (prio 0, i/o): hpet
>>>>>   00000000fee00000-00000000feefffff (prio 4096, i/o): kvm-apic-msi
>>>>>   00000000fffc0000-00000000ffffffff (prio 0, rom): pc.bios
>>>>>   0000000100000000-000000013fffffff (prio 0, ram): pc.ram @00000000c0000000
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>> Perhaps you could add instead an argument to "info mtree" that prints
>>>>>> only the AddressSpace with a given name?
>>>>>
>>>>> Nah, this is not what it is about.
>>>>
>>>>
>>>>
>>>> Still a nack? :)
>>>
>>>
>>>
>>
>

On 13/06/19 07:07, Alexey Kardashevskiy wrote:
> +        if (as == &address_space_memory) {
> +            system_as = true;
> +        }
>      }

Ah no, wait... On x86, there are two KVM address spaces, so you have to
pass "as" to has_memory.  To detect KVM address spaces, you can allocate
an array with kvm_check_extension(kvm, KVM_CAP_MULTI_ADDRESS_SPACE)
elements (default 1 if the capability is not available), and fill it in
kvm_memory_listener_register.  Then you an search for the address space
in that array, it's very small and cheap.

Promise to merge quickly with that change.

Thanks,

Paolo