[01/16] memory: do not look at current_machine->accel
diff mbox series

Message ID 1573655945-14912-2-git-send-email-pbonzini@redhat.com
State New
Headers show
Series
  • Complete the implementation of -accel
Related show

Commit Message

Paolo Bonzini Nov. 13, 2019, 2:38 p.m. UTC
"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(-)

Comments

Marc-André Lureau Nov. 14, 2019, 8:56 a.m. UTC | #1
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
>
>
>
Thomas Huth Nov. 14, 2019, 9:10 a.m. UTC | #2
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
Paolo Bonzini Nov. 14, 2019, 9:27 a.m. UTC | #3
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
Marc-André Lureau Nov. 14, 2019, 9:31 a.m. UTC | #4
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
Paolo Bonzini Nov. 14, 2019, 9:35 a.m. UTC | #5
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
Thomas Huth Nov. 14, 2019, 9:46 a.m. UTC | #6
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>
Peter Maydell Nov. 14, 2019, 10:21 a.m. UTC | #7
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
Paolo Bonzini Nov. 14, 2019, 10:32 a.m. UTC | #8
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
Peter Maydell Nov. 14, 2019, 10:40 a.m. UTC | #9
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
Paolo Bonzini Nov. 14, 2019, 10:47 a.m. UTC | #10
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

Patch
diff mbox series

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 */