Re: [Qemu-devel] [PATCH qemu v2] hmp: Print if memory section is registered with an accelerator

[Alexey Kardashevskiy]

On 14/01/2019 12:43, Alexey Kardashevskiy wrote:
> 
> 
> On 04/01/2019 04:37, Dr. David Alan Gilbert wrote:
>> * Alexey Kardashevskiy (address@hidden) 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 <address@hidden>
>>>>>>> ---
>>>>>>>
>>>>>>> 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 / address@hidden / Manchester, UK
>>
> 

-- 
Alexey