| Message ID | 1573655945-14912-2-git-send-email-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Complete the implementation of -accel | expand |
Hi On Wed, Nov 13, 2019 at 6:39 PM Paolo Bonzini <pbonzini@redhat.com> wrote: > > "info mtree" prints the wrong accelerator name if used with for example > "-machine accel=kvm:tcg". The right thing to do is to fetch the name > from the AccelClass, which will also work nicely once > current_machine->accel stops existing. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > memory.c | 5 +---- > 1 file changed, 1 insertion(+), 4 deletions(-) > > diff --git a/memory.c b/memory.c > index c952eab..1764af8 100644 > --- a/memory.c > +++ b/memory.c > @@ -2986,7 +2986,6 @@ struct FlatViewInfo { > bool dispatch_tree; > bool owner; > AccelClass *ac; > - const char *ac_name; > }; > > static void mtree_print_flatview(gpointer key, gpointer value, > @@ -3056,7 +3055,7 @@ static void mtree_print_flatview(gpointer key, gpointer value, > if (fvi->ac->has_memory(current_machine, as, > int128_get64(range->addr.start), > MR_SIZE(range->addr.size) + 1)) { > - qemu_printf(" %s", fvi->ac_name); > + qemu_printf(" %s", fvi->ac->name); There was a discussion on the original patch that this will have -accel appended. I don't think that's a big issue, someone can submit another patch later if it's important. Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> > } > } > } > @@ -3104,8 +3103,6 @@ void mtree_info(bool flatview, bool dispatch_tree, bool owner) > > 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 */ > -- > 1.8.3.1 > > >
On 13/11/2019 15.38, Paolo Bonzini wrote: > "info mtree" prints the wrong accelerator name if used with for example > "-machine accel=kvm:tcg". I had a quick look at the output of "info mtree" with and without "accel=kvm:tcg", but I could not spot any obvious places where it's wrong. Could you give an example? Thomas
On 14/11/19 09:56, Marc-André Lureau wrote: >> + qemu_printf(" %s", fvi->ac->name); > > There was a discussion on the original patch that this will have > -accel appended. This is not the class name, it's the name member of AccelClass. Paolo
On Thu, Nov 14, 2019 at 1:27 PM Paolo Bonzini <pbonzini@redhat.com> wrote: > > On 14/11/19 09:56, Marc-André Lureau wrote: > >> + qemu_printf(" %s", fvi->ac->name); > > > > There was a discussion on the original patch that this will have > > -accel appended. > > This is not the class name, it's the name member of AccelClass. > right, thanks
On 14/11/19 10:10, Thomas Huth wrote: >> "info mtree" prints the wrong accelerator name if used with for example >> "-machine accel=kvm:tcg". > I had a quick look at the output of "info mtree" with and without > "accel=kvm:tcg", but I could not spot any obvious places where it's > wrong. Could you give an example? Indeed the commit message should say "info mtree -f". $ x86_64-softmmu/qemu-system-x86_64 -M microvm -enable-kvm -machine accel=kvm:tcg -monitor stdio QEMU 4.1.50 monitor - type 'help' for more information (qemu) info mtree -f ... FlatView #1 AS "memory", root: system AS "cpu-memory-0", root: system Root memory region: system 0000000000000000-00000000000effff (prio 0, ram): microvm.ram kvm:tcg 00000000000f0000-00000000000fffff (prio 0, ram): pc.bios kvm:tcg 0000000000100000-0000000007ffffff (prio 0, ram): microvm.ram @0000000000100000 kvm:tcg Paolo
On 14/11/2019 10.35, Paolo Bonzini wrote: > On 14/11/19 10:10, Thomas Huth wrote: >>> "info mtree" prints the wrong accelerator name if used with for example >>> "-machine accel=kvm:tcg". >> I had a quick look at the output of "info mtree" with and without >> "accel=kvm:tcg", but I could not spot any obvious places where it's >> wrong. Could you give an example? > > Indeed the commit message should say "info mtree -f". Right, with "-f" it is obvious. When you added that to the commit message, feel free to add my: Tested-by: Thomas Huth <thuth@redhat.com>
On Thu, 14 Nov 2019 at 09:36, Paolo Bonzini <pbonzini@redhat.com> wrote: > > On 14/11/19 10:10, Thomas Huth wrote: > >> "info mtree" prints the wrong accelerator name if used with for example > >> "-machine accel=kvm:tcg". > > I had a quick look at the output of "info mtree" with and without > > "accel=kvm:tcg", but I could not spot any obvious places where it's > > wrong. Could you give an example? > > Indeed the commit message should say "info mtree -f". > > $ x86_64-softmmu/qemu-system-x86_64 -M microvm -enable-kvm -machine accel=kvm:tcg -monitor stdio > QEMU 4.1.50 monitor - type 'help' for more information > (qemu) info mtree -f > ... > FlatView #1 > AS "memory", root: system > AS "cpu-memory-0", root: system > Root memory region: system > 0000000000000000-00000000000effff (prio 0, ram): microvm.ram kvm:tcg > 00000000000f0000-00000000000fffff (prio 0, ram): pc.bios kvm:tcg > 0000000000100000-0000000007ffffff (prio 0, ram): microvm.ram @0000000000100000 kvm:tcg Why do we need to print the accelerator name for every memory region? Isn't it just a global property, or at least a property of the CPU ? thanks -- PMM
On 14/11/19 11:21, Peter Maydell wrote: >> FlatView #1 >> AS "memory", root: system >> AS "cpu-memory-0", root: system >> Root memory region: system >> 0000000000000000-00000000000effff (prio 0, ram): microvm.ram kvm:tcg >> 00000000000f0000-00000000000fffff (prio 0, ram): pc.bios kvm:tcg >> 0000000000100000-0000000007ffffff (prio 0, ram): microvm.ram @0000000000100000 kvm:tcg > Why do we need to print the accelerator name for every > memory region? Isn't it just a global property, or at > least a property of the CPU ? Not all regions are registered with the accelerator, in particular not for devices. So we print it next to regions that are registered. Paolo
On Thu, 14 Nov 2019 at 10:32, Paolo Bonzini <pbonzini@redhat.com> wrote: > > On 14/11/19 11:21, Peter Maydell wrote: > >> FlatView #1 > >> AS "memory", root: system > >> AS "cpu-memory-0", root: system > >> Root memory region: system > >> 0000000000000000-00000000000effff (prio 0, ram): microvm.ram kvm:tcg > >> 00000000000f0000-00000000000fffff (prio 0, ram): pc.bios kvm:tcg > >> 0000000000100000-0000000007ffffff (prio 0, ram): microvm.ram @0000000000100000 kvm:tcg > > Why do we need to print the accelerator name for every > > memory region? Isn't it just a global property, or at > > least a property of the CPU ? > > Not all regions are registered with the accelerator, in particular not > for devices. So we print it next to regions that are registered. Ah, I see -- so it's really saying "this is a kvm memslot"? thanks -- PMM
On 14/11/19 11:40, Peter Maydell wrote: > On Thu, 14 Nov 2019 at 10:32, Paolo Bonzini <pbonzini@redhat.com> wrote: >> >> On 14/11/19 11:21, Peter Maydell wrote: >>>> FlatView #1 >>>> AS "memory", root: system >>>> AS "cpu-memory-0", root: system >>>> Root memory region: system >>>> 0000000000000000-00000000000effff (prio 0, ram): microvm.ram kvm:tcg >>>> 00000000000f0000-00000000000fffff (prio 0, ram): pc.bios kvm:tcg >>>> 0000000000100000-0000000007ffffff (prio 0, ram): microvm.ram @0000000000100000 kvm:tcg >>> Why do we need to print the accelerator name for every >>> memory region? Isn't it just a global property, or at >>> least a property of the CPU ? >> >> Not all regions are registered with the accelerator, in particular not >> for devices. So we print it next to regions that are registered. > > Ah, I see -- so it's really saying "this is a kvm memslot"? Yes, exactly. Paolo
diff --git a/memory.c b/memory.c index c952eab..1764af8 100644 --- a/memory.c +++ b/memory.c @@ -2986,7 +2986,6 @@ struct FlatViewInfo { bool dispatch_tree; bool owner; AccelClass *ac; - const char *ac_name; }; static void mtree_print_flatview(gpointer key, gpointer value, @@ -3056,7 +3055,7 @@ static void mtree_print_flatview(gpointer key, gpointer value, if (fvi->ac->has_memory(current_machine, as, int128_get64(range->addr.start), MR_SIZE(range->addr.size) + 1)) { - qemu_printf(" %s", fvi->ac_name); + qemu_printf(" %s", fvi->ac->name); } } } @@ -3104,8 +3103,6 @@ void mtree_info(bool flatview, bool dispatch_tree, bool owner) 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 */
"info mtree" prints the wrong accelerator name if used with for example "-machine accel=kvm:tcg". The right thing to do is to fetch the name from the AccelClass, which will also work nicely once current_machine->accel stops existing. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- memory.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)