[PATCH for-5.0 00/16] Complete the implementation of -accel

[PATCH for-5.0 00/16] Complete the implementation of -accel

[Paolo Bonzini]

This series completes the implementation of -accel:

- Alternative accelerators can be specified with multiple -accel options,
  and each -accel option will configure the respective accelerator.
  This is implemented in patches 1 to 6, with a small addendum in patch 7.

- Accelerators can now support arbitrary options, which are implemented
  in terms of QOM properties.  This is implemented in patches 8 to 11,
  together with the conversion of the sole existing "-accel" option to QOM.

- Accelerator-related machine options, and legacy options such as -tb-size,
  forward to accelerator objects for backwards compatibility, but they
  do not exist anymore as QOM properties.  This is implemented in patch
  6 for "-machine accel" and patches 12 to 16 for everything else.

Paolo

Paolo Bonzini (16):
  memory: do not look at current_machine->accel
  vl: extract accelerator option processing to a separate function
  vl: merge -accel processing into configure_accelerators
  vl: move icount configuration earlier
  vl: introduce object_parse_property_opt
  vl: configure accelerators from -accel options
  vl: warn for unavailable accelerators, clarify messages
  qom: introduce object_register_sugar_prop
  qom: add object_new_with_class
  accel: pass object to accel_init_machine
  tcg: convert "-accel threads" to a QOM property
  tcg: add "-accel tcg,tb-size" and deprecate "-tb-size"
  xen: convert "-machine igd-passthru" to an accelerator property
  kvm: convert "-machine kvm_shadow_mem" to an accelerator property
  kvm: introduce kvm_kernel_irqchip_* functions
  kvm: convert "-machine kernel_irqchip" to an accelerator property

 accel/accel.c             |  67 +-------------
 accel/kvm/kvm-all.c       | 117 ++++++++++++++++++++++-
 accel/tcg/tcg-all.c       | 149 +++++++++++++++++++++++++++++-
 cpus.c                    |  72 ---------------
 hw/core/machine.c         | 141 ----------------------------
 hw/ppc/e500.c             |   4 +-
 hw/ppc/spapr_irq.c        |  10 +-
 hw/xen/xen-common.c       |  16 ++++
 include/hw/boards.h       |   7 --
 include/qom/object.h      |  13 +++
 include/sysemu/accel.h    |   4 +-
 include/sysemu/cpus.h     |   2 -
 include/sysemu/kvm.h      |   7 +-
 memory.c                  |   5 +-
 qemu-deprecated.texi      |   6 ++
 qemu-options.hx           |  30 +++---
 qom/object.c              |  28 +++++-
 target/arm/kvm.c          |   8 +-
 target/i386/cpu.c         |   8 +-
 target/i386/kvm.c         |   6 +-
 target/mips/kvm.c         |   2 +-
 target/ppc/kvm.c          |   2 +-
 target/s390x/cpu_models.c |   4 +-
 target/s390x/kvm.c        |   2 +-
 vl.c                      | 231 ++++++++++++++++++++++++++++++++--------------
 25 files changed, 534 insertions(+), 407 deletions(-)

-- 
1.8.3.1

[PATCH 01/16] memory: do not look at current_machine->accel

[Paolo Bonzini]

"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);
                 }
             }
         }
@@ -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

Re: [PATCH 01/16] memory: do not look at current_machine->accel

[Marc-André Lureau]

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


-- 
Marc-André Lureau

Re: [PATCH 01/16] memory: do not look at current_machine->accel

[Paolo Bonzini]

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

Re: [PATCH 01/16] memory: do not look at current_machine->accel

[Marc-André Lureau]

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

-- 
Marc-André Lureau

Re: [PATCH 01/16] memory: do not look at current_machine->accel

[Thomas Huth]

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

Re: [PATCH 01/16] memory: do not look at current_machine->accel

[Paolo Bonzini]

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

Re: [PATCH 01/16] memory: do not look at current_machine->accel

[Thomas Huth]

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>

Re: [PATCH 01/16] memory: do not look at current_machine->accel

[Peter Maydell]

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

Re: [PATCH 01/16] memory: do not look at current_machine->accel

[Paolo Bonzini]

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

Re: [PATCH 01/16] memory: do not look at current_machine->accel

[Peter Maydell]

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

Re: [PATCH 01/16] memory: do not look at current_machine->accel

[Paolo Bonzini]

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 02/16] vl: extract accelerator option processing to a separate function

[Paolo Bonzini]

As a first step towards supporting multiple "-accel" options, push -icount
and -accel semantics into a new function, and use qemu_opts_foreach to
retrieve the key/value lists instead of stashing them into globals.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 vl.c | 40 ++++++++++++++++++++++++++++------------
 1 file changed, 28 insertions(+), 12 deletions(-)

diff --git a/vl.c b/vl.c
index 841fdae..5367f23 100644
--- a/vl.c
+++ b/vl.c
@@ -2827,6 +2827,33 @@ static void user_register_global_props(void)
                       global_init_func, NULL, NULL);
 }
 
+static int do_configure_icount(void *opaque, QemuOpts *opts, Error **errp)
+{
+    if (tcg_enabled()) {
+        configure_icount(opts, errp);
+    } else {
+        error_setg(errp, "-icount is not allowed with hardware virtualization");
+    }
+    return 0;
+}
+
+static int do_configure_accelerator(void *opaque, QemuOpts *opts, Error **errp)
+{
+    if (tcg_enabled()) {
+        qemu_tcg_configure(opts, &error_fatal);
+    }
+    return 0;
+}
+
+static void configure_accelerators(void)
+{
+    qemu_opts_foreach(qemu_find_opts("icount"),
+                      do_configure_icount, NULL, &error_fatal);
+
+    qemu_opts_foreach(qemu_find_opts("accel"),
+                      do_configure_accelerator, NULL, &error_fatal);
+}
+
 int main(int argc, char **argv, char **envp)
 {
     int i;
@@ -4241,18 +4268,7 @@ int main(int argc, char **argv, char **envp)
     qemu_spice_init();
 
     cpu_ticks_init();
-    if (icount_opts) {
-        if (!tcg_enabled()) {
-            error_report("-icount is not allowed with hardware virtualization");
-            exit(1);
-        }
-        configure_icount(icount_opts, &error_abort);
-        qemu_opts_del(icount_opts);
-    }
-
-    if (tcg_enabled()) {
-        qemu_tcg_configure(accel_opts, &error_fatal);
-    }
+    configure_accelerators();
 
     if (default_net) {
         QemuOptsList *net = qemu_find_opts("net");
-- 
1.8.3.1

Re: [PATCH 02/16] vl: extract accelerator option processing to a separate function

[Marc-André Lureau]

Hi

On Wed, Nov 13, 2019 at 6:39 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> As a first step towards supporting multiple "-accel" options, push -icount
> and -accel semantics into a new function, and use qemu_opts_foreach to
> retrieve the key/value lists instead of stashing them into globals.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  vl.c | 40 ++++++++++++++++++++++++++++------------
>  1 file changed, 28 insertions(+), 12 deletions(-)
>
> diff --git a/vl.c b/vl.c
> index 841fdae..5367f23 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2827,6 +2827,33 @@ static void user_register_global_props(void)
>                        global_init_func, NULL, NULL);
>  }
>
> +static int do_configure_icount(void *opaque, QemuOpts *opts, Error **errp)
> +{
> +    if (tcg_enabled()) {
> +        configure_icount(opts, errp);
> +    } else {
> +        error_setg(errp, "-icount is not allowed with hardware virtualization");
> +    }
> +    return 0;
> +}
> +
> +static int do_configure_accelerator(void *opaque, QemuOpts *opts, Error **errp)
> +{
> +    if (tcg_enabled()) {
> +        qemu_tcg_configure(opts, &error_fatal);
> +    }
> +    return 0;
> +}
> +
> +static void configure_accelerators(void)
> +{
> +    qemu_opts_foreach(qemu_find_opts("icount"),
> +                      do_configure_icount, NULL, &error_fatal);
> +
> +    qemu_opts_foreach(qemu_find_opts("accel"),
> +                      do_configure_accelerator, NULL, &error_fatal);

It used to call qemu_tcg_configure() when no -accel option given. In
this case, it still sets mttcg_enabled = default_mttcg_enabled(), but
now it misses that. Perhaps it could be set earlier.

> +}
> +
>  int main(int argc, char **argv, char **envp)
>  {
>      int i;
> @@ -4241,18 +4268,7 @@ int main(int argc, char **argv, char **envp)
>      qemu_spice_init();
>
>      cpu_ticks_init();
> -    if (icount_opts) {
> -        if (!tcg_enabled()) {
> -            error_report("-icount is not allowed with hardware virtualization");
> -            exit(1);
> -        }
> -        configure_icount(icount_opts, &error_abort);
> -        qemu_opts_del(icount_opts);
> -    }
> -
> -    if (tcg_enabled()) {
> -        qemu_tcg_configure(accel_opts, &error_fatal);
> -    }
> +    configure_accelerators();
>
>      if (default_net) {
>          QemuOptsList *net = qemu_find_opts("net");
> --
> 1.8.3.1
>
>
>


-- 
Marc-André Lureau

Re: [PATCH 02/16] vl: extract accelerator option processing to a separate function

[Paolo Bonzini]

On 14/11/19 08:55, Marc-André Lureau wrote:
>> +
>> +    qemu_opts_foreach(qemu_find_opts("accel"),
>> +                      do_configure_accelerator, NULL, &error_fatal);
> It used to call qemu_tcg_configure() when no -accel option given. In
> this case, it still sets mttcg_enabled = default_mttcg_enabled(), but
> now it misses that. Perhaps it could be set earlier.
> 

Oh, right.  This is fixed later in the series, but I think in v2 I will
move this to the TCG init function.

Paolo

Re: [PATCH 02/16] vl: extract accelerator option processing to a separate function

[Thomas Huth]

On 13/11/2019 15.38, Paolo Bonzini wrote:
> As a first step towards supporting multiple "-accel" options, push -icount
> and -accel semantics into a new function, and use qemu_opts_foreach to
> retrieve the key/value lists instead of stashing them into globals.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  vl.c | 40 ++++++++++++++++++++++++++++------------
>  1 file changed, 28 insertions(+), 12 deletions(-)
> 
> diff --git a/vl.c b/vl.c
> index 841fdae..5367f23 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2827,6 +2827,33 @@ static void user_register_global_props(void)
>                        global_init_func, NULL, NULL);
>  }
>  
> +static int do_configure_icount(void *opaque, QemuOpts *opts, Error **errp)
> +{
> +    if (tcg_enabled()) {
> +        configure_icount(opts, errp);
> +    } else {
> +        error_setg(errp, "-icount is not allowed with hardware virtualization");
> +    }
> +    return 0;
> +}
> +
> +static int do_configure_accelerator(void *opaque, QemuOpts *opts, Error **errp)
> +{
> +    if (tcg_enabled()) {
> +        qemu_tcg_configure(opts, &error_fatal);
> +    }
> +    return 0;
> +}
> +
> +static void configure_accelerators(void)
> +{
> +    qemu_opts_foreach(qemu_find_opts("icount"),
> +                      do_configure_icount, NULL, &error_fatal);
> +
> +    qemu_opts_foreach(qemu_find_opts("accel"),
> +                      do_configure_accelerator, NULL, &error_fatal);
> +}

vl.c is already quite overcrowded ... maybe you could add the new code
to accel/accel.c instead? Just my 0.02 €.

 Thomas

Re: [PATCH 02/16] vl: extract accelerator option processing to a separate function

[Paolo Bonzini]

On 18/11/19 11:58, Thomas Huth wrote:
>> +static void configure_accelerators(void)
>> +{
>> +    qemu_opts_foreach(qemu_find_opts("icount"),
>> +                      do_configure_icount, NULL, &error_fatal);
>> +
>> +    qemu_opts_foreach(qemu_find_opts("accel"),
>> +                      do_configure_accelerator, NULL, &error_fatal);
>> +}
>
> vl.c is already quite overcrowded ... maybe you could add the new code
> to accel/accel.c instead? Just my 0.02 €.

I liked the idea of keeping all command line parsing in vl.c, especially
because all the ugliness for backwards compatibility can then be
confined there.

Paolo

[PATCH 03/16] vl: merge -accel processing into configure_accelerators

[Paolo Bonzini]

The next step is to move the parsing of "-machine accel=..." into vl.c,
unifying it with the configure_accelerators() function that has just
been introduced.  This way, we will be able to desugar it into multiple
"-accel" options, without polluting accel/accel.c.

The CONFIG_TCG and CONFIG_KVM symbols are not available in vl.c, but
we can use accel_find instead to find their value at runtime.  Once we
know that the binary has one of TCG or KVM, the default accelerator
can be expressed simply as "tcg:kvm", because TCG never fails to initialize.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 accel/accel.c          | 63 ++------------------------------------------------
 include/sysemu/accel.h |  4 +++-
 vl.c                   | 62 +++++++++++++++++++++++++++++++++++++++++++++----
 3 files changed, 62 insertions(+), 67 deletions(-)

diff --git a/accel/accel.c b/accel/accel.c
index 5fa3171..74eda68 100644
--- a/accel/accel.c
+++ b/accel/accel.c
@@ -44,7 +44,7 @@ static const TypeInfo accel_type = {
 };
 
 /* Lookup AccelClass from opt_name. Returns NULL if not found */
-static AccelClass *accel_find(const char *opt_name)
+AccelClass *accel_find(const char *opt_name)
 {
     char *class_name = g_strdup_printf(ACCEL_CLASS_NAME("%s"), opt_name);
     AccelClass *ac = ACCEL_CLASS(object_class_by_name(class_name));
@@ -52,7 +52,7 @@ static AccelClass *accel_find(const char *opt_name)
     return ac;
 }
 
-static int accel_init_machine(AccelClass *acc, MachineState *ms)
+int accel_init_machine(AccelClass *acc, MachineState *ms)
 {
     ObjectClass *oc = OBJECT_CLASS(acc);
     const char *cname = object_class_get_name(oc);
@@ -71,65 +71,6 @@ static int accel_init_machine(AccelClass *acc, MachineState *ms)
     return ret;
 }
 
-void configure_accelerator(MachineState *ms, const char *progname)
-{
-    const char *accel;
-    char **accel_list, **tmp;
-    int ret;
-    bool accel_initialised = false;
-    bool init_failed = false;
-    AccelClass *acc = NULL;
-
-    accel = qemu_opt_get(qemu_get_machine_opts(), "accel");
-    if (accel == NULL) {
-        /* Select the default accelerator */
-        int pnlen = strlen(progname);
-        if (pnlen >= 3 && g_str_equal(&progname[pnlen - 3], "kvm")) {
-            /* If the program name ends with "kvm", we prefer KVM */
-            accel = "kvm:tcg";
-        } else {
-#if defined(CONFIG_TCG)
-            accel = "tcg";
-#elif defined(CONFIG_KVM)
-            accel = "kvm";
-#else
-            error_report("No accelerator selected and"
-                         " no default accelerator available");
-            exit(1);
-#endif
-        }
-    }
-
-    accel_list = g_strsplit(accel, ":", 0);
-
-    for (tmp = accel_list; !accel_initialised && tmp && *tmp; tmp++) {
-        acc = accel_find(*tmp);
-        if (!acc) {
-            continue;
-        }
-        ret = accel_init_machine(acc, ms);
-        if (ret < 0) {
-            init_failed = true;
-            error_report("failed to initialize %s: %s",
-                         acc->name, strerror(-ret));
-        } else {
-            accel_initialised = true;
-        }
-    }
-    g_strfreev(accel_list);
-
-    if (!accel_initialised) {
-        if (!init_failed) {
-            error_report("-machine accel=%s: No accelerator found", accel);
-        }
-        exit(1);
-    }
-
-    if (init_failed) {
-        error_report("Back to %s accelerator", acc->name);
-    }
-}
-
 void accel_setup_post(MachineState *ms)
 {
     AccelState *accel = ms->accelerator;
diff --git a/include/sysemu/accel.h b/include/sysemu/accel.h
index 8eb60b8..90b6213 100644
--- a/include/sysemu/accel.h
+++ b/include/sysemu/accel.h
@@ -66,7 +66,9 @@ typedef struct AccelClass {
 
 extern unsigned long tcg_tb_size;
 
-void configure_accelerator(MachineState *ms, const char *progname);
+AccelClass *accel_find(const char *opt_name);
+int accel_init_machine(AccelClass *acc, MachineState *ms);
+
 /* Called just before os_setup_post (ie just before drop OS privs) */
 void accel_setup_post(MachineState *ms);
 
diff --git a/vl.c b/vl.c
index 5367f23..fc9e70f 100644
--- a/vl.c
+++ b/vl.c
@@ -2845,8 +2845,62 @@ static int do_configure_accelerator(void *opaque, QemuOpts *opts, Error **errp)
     return 0;
 }
 
-static void configure_accelerators(void)
+static void configure_accelerators(const char *progname)
 {
+    const char *accel;
+    char **accel_list, **tmp;
+    int ret;
+    bool accel_initialised = false;
+    bool init_failed = false;
+    AccelClass *acc = NULL;
+
+    accel = qemu_opt_get(qemu_get_machine_opts(), "accel");
+    if (accel == NULL) {
+        /* Select the default accelerator */
+        if (!accel_find("tcg") && !accel_find("kvm")) {
+            error_report("No accelerator selected and"
+                         " no default accelerator available");
+            exit(1);
+        } else {
+            int pnlen = strlen(progname);
+            if (pnlen >= 3 && g_str_equal(&progname[pnlen - 3], "kvm")) {
+                /* If the program name ends with "kvm", we prefer KVM */
+                accel = "kvm:tcg";
+            } else {
+                accel = "tcg:kvm";
+            }
+        }
+    }
+
+    accel_list = g_strsplit(accel, ":", 0);
+
+    for (tmp = accel_list; !accel_initialised && tmp && *tmp; tmp++) {
+        acc = accel_find(*tmp);
+        if (!acc) {
+            continue;
+        }
+        ret = accel_init_machine(acc, current_machine);
+        if (ret < 0) {
+            init_failed = true;
+            error_report("failed to initialize %s: %s",
+                         acc->name, strerror(-ret));
+        } else {
+            accel_initialised = true;
+        }
+    }
+    g_strfreev(accel_list);
+
+    if (!accel_initialised) {
+        if (!init_failed) {
+            error_report("-machine accel=%s: No accelerator found", accel);
+        }
+        exit(1);
+    }
+
+    if (init_failed) {
+        error_report("Back to %s accelerator", acc->name);
+    }
+
     qemu_opts_foreach(qemu_find_opts("icount"),
                       do_configure_icount, NULL, &error_fatal);
 
@@ -4183,7 +4237,8 @@ int main(int argc, char **argv, char **envp)
      * Note: uses machine properties such as kernel-irqchip, must run
      * after machine_set_property().
      */
-    configure_accelerator(current_machine, argv[0]);
+    cpu_ticks_init();
+    configure_accelerators(argv[0]);
 
     /*
      * Beware, QOM objects created before this point miss global and
@@ -4267,9 +4322,6 @@ int main(int argc, char **argv, char **envp)
     /* spice needs the timers to be initialized by this point */
     qemu_spice_init();
 
-    cpu_ticks_init();
-    configure_accelerators();
-
     if (default_net) {
         QemuOptsList *net = qemu_find_opts("net");
         qemu_opts_set(net, NULL, "type", "nic", &error_abort);
-- 
1.8.3.1

Re: [PATCH 03/16] vl: merge -accel processing into configure_accelerators

[Marc-André Lureau]

Hi

On Wed, Nov 13, 2019 at 6:42 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> The next step is to move the parsing of "-machine accel=..." into vl.c,
> unifying it with the configure_accelerators() function that has just
> been introduced.  This way, we will be able to desugar it into multiple
> "-accel" options, without polluting accel/accel.c.
>
> The CONFIG_TCG and CONFIG_KVM symbols are not available in vl.c, but
> we can use accel_find instead to find their value at runtime.  Once we
> know that the binary has one of TCG or KVM, the default accelerator
> can be expressed simply as "tcg:kvm", because TCG never fails to initialize.
>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  accel/accel.c          | 63 ++------------------------------------------------
>  include/sysemu/accel.h |  4 +++-
>  vl.c                   | 62 +++++++++++++++++++++++++++++++++++++++++++++----
>  3 files changed, 62 insertions(+), 67 deletions(-)
>
> diff --git a/accel/accel.c b/accel/accel.c
> index 5fa3171..74eda68 100644
> --- a/accel/accel.c
> +++ b/accel/accel.c
> @@ -44,7 +44,7 @@ static const TypeInfo accel_type = {
>  };
>
>  /* Lookup AccelClass from opt_name. Returns NULL if not found */
> -static AccelClass *accel_find(const char *opt_name)
> +AccelClass *accel_find(const char *opt_name)
>  {
>      char *class_name = g_strdup_printf(ACCEL_CLASS_NAME("%s"), opt_name);
>      AccelClass *ac = ACCEL_CLASS(object_class_by_name(class_name));
> @@ -52,7 +52,7 @@ static AccelClass *accel_find(const char *opt_name)
>      return ac;
>  }
>
> -static int accel_init_machine(AccelClass *acc, MachineState *ms)
> +int accel_init_machine(AccelClass *acc, MachineState *ms)
>  {
>      ObjectClass *oc = OBJECT_CLASS(acc);
>      const char *cname = object_class_get_name(oc);
> @@ -71,65 +71,6 @@ static int accel_init_machine(AccelClass *acc, MachineState *ms)
>      return ret;
>  }
>
> -void configure_accelerator(MachineState *ms, const char *progname)
> -{
> -    const char *accel;
> -    char **accel_list, **tmp;
> -    int ret;
> -    bool accel_initialised = false;
> -    bool init_failed = false;
> -    AccelClass *acc = NULL;
> -
> -    accel = qemu_opt_get(qemu_get_machine_opts(), "accel");
> -    if (accel == NULL) {
> -        /* Select the default accelerator */
> -        int pnlen = strlen(progname);
> -        if (pnlen >= 3 && g_str_equal(&progname[pnlen - 3], "kvm")) {
> -            /* If the program name ends with "kvm", we prefer KVM */
> -            accel = "kvm:tcg";
> -        } else {
> -#if defined(CONFIG_TCG)
> -            accel = "tcg";
> -#elif defined(CONFIG_KVM)
> -            accel = "kvm";
> -#else
> -            error_report("No accelerator selected and"
> -                         " no default accelerator available");
> -            exit(1);
> -#endif
> -        }
> -    }
> -
> -    accel_list = g_strsplit(accel, ":", 0);
> -
> -    for (tmp = accel_list; !accel_initialised && tmp && *tmp; tmp++) {
> -        acc = accel_find(*tmp);
> -        if (!acc) {
> -            continue;
> -        }
> -        ret = accel_init_machine(acc, ms);
> -        if (ret < 0) {
> -            init_failed = true;
> -            error_report("failed to initialize %s: %s",
> -                         acc->name, strerror(-ret));
> -        } else {
> -            accel_initialised = true;
> -        }
> -    }
> -    g_strfreev(accel_list);
> -
> -    if (!accel_initialised) {
> -        if (!init_failed) {
> -            error_report("-machine accel=%s: No accelerator found", accel);
> -        }
> -        exit(1);
> -    }
> -
> -    if (init_failed) {
> -        error_report("Back to %s accelerator", acc->name);
> -    }
> -}
> -
>  void accel_setup_post(MachineState *ms)
>  {
>      AccelState *accel = ms->accelerator;
> diff --git a/include/sysemu/accel.h b/include/sysemu/accel.h
> index 8eb60b8..90b6213 100644
> --- a/include/sysemu/accel.h
> +++ b/include/sysemu/accel.h
> @@ -66,7 +66,9 @@ typedef struct AccelClass {
>
>  extern unsigned long tcg_tb_size;
>
> -void configure_accelerator(MachineState *ms, const char *progname);
> +AccelClass *accel_find(const char *opt_name);
> +int accel_init_machine(AccelClass *acc, MachineState *ms);
> +
>  /* Called just before os_setup_post (ie just before drop OS privs) */
>  void accel_setup_post(MachineState *ms);
>
> diff --git a/vl.c b/vl.c
> index 5367f23..fc9e70f 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2845,8 +2845,62 @@ static int do_configure_accelerator(void *opaque, QemuOpts *opts, Error **errp)
>      return 0;
>  }
>
> -static void configure_accelerators(void)
> +static void configure_accelerators(const char *progname)
>  {
> +    const char *accel;
> +    char **accel_list, **tmp;
> +    int ret;
> +    bool accel_initialised = false;
> +    bool init_failed = false;
> +    AccelClass *acc = NULL;
> +
> +    accel = qemu_opt_get(qemu_get_machine_opts(), "accel");
> +    if (accel == NULL) {
> +        /* Select the default accelerator */
> +        if (!accel_find("tcg") && !accel_find("kvm")) {
> +            error_report("No accelerator selected and"
> +                         " no default accelerator available");
> +            exit(1);
> +        } else {
> +            int pnlen = strlen(progname);
> +            if (pnlen >= 3 && g_str_equal(&progname[pnlen - 3], "kvm")) {
> +                /* If the program name ends with "kvm", we prefer KVM */
> +                accel = "kvm:tcg";
> +            } else {
> +                accel = "tcg:kvm";
> +            }
> +        }
> +    }
> +
> +    accel_list = g_strsplit(accel, ":", 0);
> +
> +    for (tmp = accel_list; !accel_initialised && tmp && *tmp; tmp++) {
> +        acc = accel_find(*tmp);
> +        if (!acc) {
> +            continue;
> +        }
> +        ret = accel_init_machine(acc, current_machine);
> +        if (ret < 0) {
> +            init_failed = true;
> +            error_report("failed to initialize %s: %s",
> +                         acc->name, strerror(-ret));
> +        } else {
> +            accel_initialised = true;
> +        }
> +    }
> +    g_strfreev(accel_list);
> +
> +    if (!accel_initialised) {
> +        if (!init_failed) {
> +            error_report("-machine accel=%s: No accelerator found", accel);
> +        }
> +        exit(1);
> +    }
> +
> +    if (init_failed) {
> +        error_report("Back to %s accelerator", acc->name);
> +    }
> +
>      qemu_opts_foreach(qemu_find_opts("icount"),
>                        do_configure_icount, NULL, &error_fatal);
>
> @@ -4183,7 +4237,8 @@ int main(int argc, char **argv, char **envp)
>       * Note: uses machine properties such as kernel-irqchip, must run
>       * after machine_set_property().
>       */
> -    configure_accelerator(current_machine, argv[0]);
> +    cpu_ticks_init();
> +    configure_accelerators(argv[0]);
>
>      /*
>       * Beware, QOM objects created before this point miss global and
> @@ -4267,9 +4322,6 @@ int main(int argc, char **argv, char **envp)
>      /* spice needs the timers to be initialized by this point */
>      qemu_spice_init();
>
> -    cpu_ticks_init();
> -    configure_accelerators();
> -
>      if (default_net) {
>          QemuOptsList *net = qemu_find_opts("net");
>          qemu_opts_set(net, NULL, "type", "nic", &error_abort);
> --
> 1.8.3.1
>
>
>


-- 
Marc-André Lureau

Re: [PATCH 03/16] vl: merge -accel processing into configure_accelerators

[Thomas Huth]

On 13/11/2019 15.38, Paolo Bonzini wrote:
> The next step is to move the parsing of "-machine accel=..." into vl.c,
> unifying it with the configure_accelerators() function that has just
> been introduced.  This way, we will be able to desugar it into multiple
> "-accel" options, without polluting accel/accel.c.
> 
> The CONFIG_TCG and CONFIG_KVM symbols are not available in vl.c, but
> we can use accel_find instead to find their value at runtime.  Once we
> know that the binary has one of TCG or KVM, the default accelerator
> can be expressed simply as "tcg:kvm", because TCG never fails to initialize.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  accel/accel.c          | 63 ++------------------------------------------------
>  include/sysemu/accel.h |  4 +++-
>  vl.c                   | 62 +++++++++++++++++++++++++++++++++++++++++++++----
>  3 files changed, 62 insertions(+), 67 deletions(-)
> 
> diff --git a/accel/accel.c b/accel/accel.c
> index 5fa3171..74eda68 100644
> --- a/accel/accel.c
> +++ b/accel/accel.c
> @@ -44,7 +44,7 @@ static const TypeInfo accel_type = {
>  };
>  
>  /* Lookup AccelClass from opt_name. Returns NULL if not found */
> -static AccelClass *accel_find(const char *opt_name)
> +AccelClass *accel_find(const char *opt_name)
>  {
>      char *class_name = g_strdup_printf(ACCEL_CLASS_NAME("%s"), opt_name);
>      AccelClass *ac = ACCEL_CLASS(object_class_by_name(class_name));
> @@ -52,7 +52,7 @@ static AccelClass *accel_find(const char *opt_name)
>      return ac;
>  }
>  
> -static int accel_init_machine(AccelClass *acc, MachineState *ms)
> +int accel_init_machine(AccelClass *acc, MachineState *ms)
>  {
>      ObjectClass *oc = OBJECT_CLASS(acc);
>      const char *cname = object_class_get_name(oc);
> @@ -71,65 +71,6 @@ static int accel_init_machine(AccelClass *acc, MachineState *ms)
>      return ret;
>  }
>  
> -void configure_accelerator(MachineState *ms, const char *progname)
> -{
> -    const char *accel;
> -    char **accel_list, **tmp;
> -    int ret;
> -    bool accel_initialised = false;
> -    bool init_failed = false;
> -    AccelClass *acc = NULL;
> -
> -    accel = qemu_opt_get(qemu_get_machine_opts(), "accel");
> -    if (accel == NULL) {
> -        /* Select the default accelerator */
> -        int pnlen = strlen(progname);
> -        if (pnlen >= 3 && g_str_equal(&progname[pnlen - 3], "kvm")) {
> -            /* If the program name ends with "kvm", we prefer KVM */
> -            accel = "kvm:tcg";
> -        } else {
> -#if defined(CONFIG_TCG)
> -            accel = "tcg";
> -#elif defined(CONFIG_KVM)
> -            accel = "kvm";
> -#else
> -            error_report("No accelerator selected and"
> -                         " no default accelerator available");
> -            exit(1);
> -#endif
> -        }
> -    }
> -
> -    accel_list = g_strsplit(accel, ":", 0);
> -
> -    for (tmp = accel_list; !accel_initialised && tmp && *tmp; tmp++) {
> -        acc = accel_find(*tmp);
> -        if (!acc) {
> -            continue;
> -        }
> -        ret = accel_init_machine(acc, ms);
> -        if (ret < 0) {
> -            init_failed = true;
> -            error_report("failed to initialize %s: %s",
> -                         acc->name, strerror(-ret));
> -        } else {
> -            accel_initialised = true;
> -        }
> -    }
> -    g_strfreev(accel_list);
> -
> -    if (!accel_initialised) {
> -        if (!init_failed) {
> -            error_report("-machine accel=%s: No accelerator found", accel);
> -        }
> -        exit(1);
> -    }
> -
> -    if (init_failed) {
> -        error_report("Back to %s accelerator", acc->name);
> -    }
> -}
> -
>  void accel_setup_post(MachineState *ms)
>  {
>      AccelState *accel = ms->accelerator;
> diff --git a/include/sysemu/accel.h b/include/sysemu/accel.h
> index 8eb60b8..90b6213 100644
> --- a/include/sysemu/accel.h
> +++ b/include/sysemu/accel.h
> @@ -66,7 +66,9 @@ typedef struct AccelClass {
>  
>  extern unsigned long tcg_tb_size;
>  
> -void configure_accelerator(MachineState *ms, const char *progname);
> +AccelClass *accel_find(const char *opt_name);
> +int accel_init_machine(AccelClass *acc, MachineState *ms);
> +
>  /* Called just before os_setup_post (ie just before drop OS privs) */
>  void accel_setup_post(MachineState *ms);
>  
> diff --git a/vl.c b/vl.c
> index 5367f23..fc9e70f 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2845,8 +2845,62 @@ static int do_configure_accelerator(void *opaque, QemuOpts *opts, Error **errp)
>      return 0;
>  }
>  
> -static void configure_accelerators(void)
> +static void configure_accelerators(const char *progname)
>  {
> +    const char *accel;
> +    char **accel_list, **tmp;
> +    int ret;
> +    bool accel_initialised = false;
> +    bool init_failed = false;
> +    AccelClass *acc = NULL;
> +
> +    accel = qemu_opt_get(qemu_get_machine_opts(), "accel");
> +    if (accel == NULL) {
> +        /* Select the default accelerator */
> +        if (!accel_find("tcg") && !accel_find("kvm")) {
> +            error_report("No accelerator selected and"
> +                         " no default accelerator available");
> +            exit(1);
> +        } else {
> +            int pnlen = strlen(progname);
> +            if (pnlen >= 3 && g_str_equal(&progname[pnlen - 3], "kvm")) {
> +                /* If the program name ends with "kvm", we prefer KVM */
> +                accel = "kvm:tcg";
> +            } else {
> +                accel = "tcg:kvm";
> +            }
> +        }
> +    }
> +
> +    accel_list = g_strsplit(accel, ":", 0);
> +
> +    for (tmp = accel_list; !accel_initialised && tmp && *tmp; tmp++) {
> +        acc = accel_find(*tmp);
> +        if (!acc) {
> +            continue;
> +        }
> +        ret = accel_init_machine(acc, current_machine);
> +        if (ret < 0) {
> +            init_failed = true;
> +            error_report("failed to initialize %s: %s",
> +                         acc->name, strerror(-ret));
> +        } else {
> +            accel_initialised = true;
> +        }
> +    }
> +    g_strfreev(accel_list);
> +
> +    if (!accel_initialised) {
> +        if (!init_failed) {
> +            error_report("-machine accel=%s: No accelerator found", accel);
> +        }
> +        exit(1);
> +    }
> +
> +    if (init_failed) {
> +        error_report("Back to %s accelerator", acc->name);
> +    }
> +
>      qemu_opts_foreach(qemu_find_opts("icount"),
>                        do_configure_icount, NULL, &error_fatal);
>  
> @@ -4183,7 +4237,8 @@ int main(int argc, char **argv, char **envp)
>       * Note: uses machine properties such as kernel-irqchip, must run
>       * after machine_set_property().
>       */
> -    configure_accelerator(current_machine, argv[0]);
> +    cpu_ticks_init();
> +    configure_accelerators(argv[0]);

The comment about kernel-irqchip obviously rather belongs to
configure_accelerators() instead of cpu_ticks_init(), so maybe you
should move the cpu_ticks_init() before the comment now? (does it need
to be moved here at all? ... it looks pretty independent at a first glance)

 Thomas

Re: [PATCH 03/16] vl: merge -accel processing into configure_accelerators

[Paolo Bonzini]

On 18/11/19 12:27, Thomas Huth wrote:
>>       * Note: uses machine properties such as kernel-irqchip, must run
>>       * after machine_set_property().
>>       */
>> -    configure_accelerator(current_machine, argv[0]);
>> +    cpu_ticks_init();
>> +    configure_accelerators(argv[0]);
> The comment about kernel-irqchip obviously rather belongs to
> configure_accelerators() instead of cpu_ticks_init(), so maybe you
> should move the cpu_ticks_init() before the comment now?

Ok.

> (does it need
> to be moved here at all? ... it looks pretty independent at a first glance)

No, it doesn't.  Whether to move it or not is a matter of personal
preference, so I can certainly move it back.

Paolo

Re: [PATCH 03/16] vl: merge -accel processing into configure_accelerators

[Wainer dos Santos Moschetta]

On 11/13/19 12:38 PM, Paolo Bonzini wrote:
> The next step is to move the parsing of "-machine accel=..." into vl.c,
> unifying it with the configure_accelerators() function that has just
> been introduced.  This way, we will be able to desugar it into multiple
> "-accel" options, without polluting accel/accel.c.
>
> The CONFIG_TCG and CONFIG_KVM symbols are not available in vl.c, but
> we can use accel_find instead to find their value at runtime.  Once we
> know that the binary has one of TCG or KVM, the default accelerator
> can be expressed simply as "tcg:kvm", because TCG never fails to initialize.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   accel/accel.c          | 63 ++------------------------------------------------
>   include/sysemu/accel.h |  4 +++-
>   vl.c                   | 62 +++++++++++++++++++++++++++++++++++++++++++++----
>   3 files changed, 62 insertions(+), 67 deletions(-)


Reviewed-by: Wainer dos Santos Moschetta <wainersm@redhat.com>


>
> diff --git a/accel/accel.c b/accel/accel.c
> index 5fa3171..74eda68 100644
> --- a/accel/accel.c
> +++ b/accel/accel.c
> @@ -44,7 +44,7 @@ static const TypeInfo accel_type = {
>   };
>   
>   /* Lookup AccelClass from opt_name. Returns NULL if not found */
> -static AccelClass *accel_find(const char *opt_name)
> +AccelClass *accel_find(const char *opt_name)
>   {
>       char *class_name = g_strdup_printf(ACCEL_CLASS_NAME("%s"), opt_name);
>       AccelClass *ac = ACCEL_CLASS(object_class_by_name(class_name));
> @@ -52,7 +52,7 @@ static AccelClass *accel_find(const char *opt_name)
>       return ac;
>   }
>   
> -static int accel_init_machine(AccelClass *acc, MachineState *ms)
> +int accel_init_machine(AccelClass *acc, MachineState *ms)
>   {
>       ObjectClass *oc = OBJECT_CLASS(acc);
>       const char *cname = object_class_get_name(oc);
> @@ -71,65 +71,6 @@ static int accel_init_machine(AccelClass *acc, MachineState *ms)
>       return ret;
>   }
>   
> -void configure_accelerator(MachineState *ms, const char *progname)
> -{
> -    const char *accel;
> -    char **accel_list, **tmp;
> -    int ret;
> -    bool accel_initialised = false;
> -    bool init_failed = false;
> -    AccelClass *acc = NULL;
> -
> -    accel = qemu_opt_get(qemu_get_machine_opts(), "accel");
> -    if (accel == NULL) {
> -        /* Select the default accelerator */
> -        int pnlen = strlen(progname);
> -        if (pnlen >= 3 && g_str_equal(&progname[pnlen - 3], "kvm")) {
> -            /* If the program name ends with "kvm", we prefer KVM */
> -            accel = "kvm:tcg";
> -        } else {
> -#if defined(CONFIG_TCG)
> -            accel = "tcg";
> -#elif defined(CONFIG_KVM)
> -            accel = "kvm";
> -#else
> -            error_report("No accelerator selected and"
> -                         " no default accelerator available");
> -            exit(1);
> -#endif
> -        }
> -    }
> -
> -    accel_list = g_strsplit(accel, ":", 0);
> -
> -    for (tmp = accel_list; !accel_initialised && tmp && *tmp; tmp++) {
> -        acc = accel_find(*tmp);
> -        if (!acc) {
> -            continue;
> -        }
> -        ret = accel_init_machine(acc, ms);
> -        if (ret < 0) {
> -            init_failed = true;
> -            error_report("failed to initialize %s: %s",
> -                         acc->name, strerror(-ret));
> -        } else {
> -            accel_initialised = true;
> -        }
> -    }
> -    g_strfreev(accel_list);
> -
> -    if (!accel_initialised) {
> -        if (!init_failed) {
> -            error_report("-machine accel=%s: No accelerator found", accel);
> -        }
> -        exit(1);
> -    }
> -
> -    if (init_failed) {
> -        error_report("Back to %s accelerator", acc->name);
> -    }
> -}
> -
>   void accel_setup_post(MachineState *ms)
>   {
>       AccelState *accel = ms->accelerator;
> diff --git a/include/sysemu/accel.h b/include/sysemu/accel.h
> index 8eb60b8..90b6213 100644
> --- a/include/sysemu/accel.h
> +++ b/include/sysemu/accel.h
> @@ -66,7 +66,9 @@ typedef struct AccelClass {
>   
>   extern unsigned long tcg_tb_size;
>   
> -void configure_accelerator(MachineState *ms, const char *progname);
> +AccelClass *accel_find(const char *opt_name);
> +int accel_init_machine(AccelClass *acc, MachineState *ms);
> +
>   /* Called just before os_setup_post (ie just before drop OS privs) */
>   void accel_setup_post(MachineState *ms);
>   
> diff --git a/vl.c b/vl.c
> index 5367f23..fc9e70f 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2845,8 +2845,62 @@ static int do_configure_accelerator(void *opaque, QemuOpts *opts, Error **errp)
>       return 0;
>   }
>   
> -static void configure_accelerators(void)
> +static void configure_accelerators(const char *progname)
>   {
> +    const char *accel;
> +    char **accel_list, **tmp;
> +    int ret;
> +    bool accel_initialised = false;
> +    bool init_failed = false;
> +    AccelClass *acc = NULL;
> +
> +    accel = qemu_opt_get(qemu_get_machine_opts(), "accel");
> +    if (accel == NULL) {
> +        /* Select the default accelerator */
> +        if (!accel_find("tcg") && !accel_find("kvm")) {
> +            error_report("No accelerator selected and"
> +                         " no default accelerator available");
> +            exit(1);
> +        } else {
> +            int pnlen = strlen(progname);
> +            if (pnlen >= 3 && g_str_equal(&progname[pnlen - 3], "kvm")) {
> +                /* If the program name ends with "kvm", we prefer KVM */
> +                accel = "kvm:tcg";
> +            } else {
> +                accel = "tcg:kvm";
> +            }
> +        }
> +    }
> +
> +    accel_list = g_strsplit(accel, ":", 0);
> +
> +    for (tmp = accel_list; !accel_initialised && tmp && *tmp; tmp++) {
> +        acc = accel_find(*tmp);
> +        if (!acc) {
> +            continue;
> +        }
> +        ret = accel_init_machine(acc, current_machine);
> +        if (ret < 0) {
> +            init_failed = true;
> +            error_report("failed to initialize %s: %s",
> +                         acc->name, strerror(-ret));
> +        } else {
> +            accel_initialised = true;
> +        }
> +    }
> +    g_strfreev(accel_list);
> +
> +    if (!accel_initialised) {
> +        if (!init_failed) {
> +            error_report("-machine accel=%s: No accelerator found", accel);
> +        }
> +        exit(1);
> +    }
> +
> +    if (init_failed) {
> +        error_report("Back to %s accelerator", acc->name);
> +    }
> +
>       qemu_opts_foreach(qemu_find_opts("icount"),
>                         do_configure_icount, NULL, &error_fatal);
>   
> @@ -4183,7 +4237,8 @@ int main(int argc, char **argv, char **envp)
>        * Note: uses machine properties such as kernel-irqchip, must run
>        * after machine_set_property().
>        */
> -    configure_accelerator(current_machine, argv[0]);
> +    cpu_ticks_init();
> +    configure_accelerators(argv[0]);
>   
>       /*
>        * Beware, QOM objects created before this point miss global and
> @@ -4267,9 +4322,6 @@ int main(int argc, char **argv, char **envp)
>       /* spice needs the timers to be initialized by this point */
>       qemu_spice_init();
>   
> -    cpu_ticks_init();
> -    configure_accelerators();
> -
>       if (default_net) {
>           QemuOptsList *net = qemu_find_opts("net");
>           qemu_opts_set(net, NULL, "type", "nic", &error_abort);

[PATCH 04/16] vl: move icount configuration earlier

[Paolo Bonzini]

Once qemu_tcg_configure is turned into a QOM property setter, it will not
be able to set a default value for mttcg_enabled.  Setting the default will
move to the TCG init_machine method, which currently runs after "-icount"
is processed.

However, it is harmless to do configure_icount for all accelerators; we will
just fail later if a non-TCG accelerator being selected.  So do that.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 vl.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/vl.c b/vl.c
index fc9e70f..dbc99d7 100644
--- a/vl.c
+++ b/vl.c
@@ -2829,11 +2829,7 @@ static void user_register_global_props(void)
 
 static int do_configure_icount(void *opaque, QemuOpts *opts, Error **errp)
 {
-    if (tcg_enabled()) {
-        configure_icount(opts, errp);
-    } else {
-        error_setg(errp, "-icount is not allowed with hardware virtualization");
-    }
+    configure_icount(opts, errp);
     return 0;
 }
 
@@ -2854,6 +2850,9 @@ static void configure_accelerators(const char *progname)
     bool init_failed = false;
     AccelClass *acc = NULL;
 
+    qemu_opts_foreach(qemu_find_opts("icount"),
+                      do_configure_icount, NULL, &error_fatal);
+
     accel = qemu_opt_get(qemu_get_machine_opts(), "accel");
     if (accel == NULL) {
         /* Select the default accelerator */
@@ -2901,11 +2900,13 @@ static void configure_accelerators(const char *progname)
         error_report("Back to %s accelerator", acc->name);
     }
 
-    qemu_opts_foreach(qemu_find_opts("icount"),
-                      do_configure_icount, NULL, &error_fatal);
-
     qemu_opts_foreach(qemu_find_opts("accel"),
                       do_configure_accelerator, NULL, &error_fatal);
+
+    if (!tcg_enabled() && use_icount) {
+        error_report("-icount is not allowed with hardware virtualization");
+        exit(1);
+    }
 }
 
 int main(int argc, char **argv, char **envp)
-- 
1.8.3.1

Re: [PATCH 04/16] vl: move icount configuration earlier

[Marc-André Lureau]

On Wed, Nov 13, 2019 at 6:43 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> Once qemu_tcg_configure is turned into a QOM property setter, it will not
> be able to set a default value for mttcg_enabled.  Setting the default will
> move to the TCG init_machine method, which currently runs after "-icount"
> is processed.
>
> However, it is harmless to do configure_icount for all accelerators; we will
> just fail later if a non-TCG accelerator being selected.  So do that.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>


> ---
>  vl.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/vl.c b/vl.c
> index fc9e70f..dbc99d7 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2829,11 +2829,7 @@ static void user_register_global_props(void)
>
>  static int do_configure_icount(void *opaque, QemuOpts *opts, Error **errp)
>  {
> -    if (tcg_enabled()) {
> -        configure_icount(opts, errp);
> -    } else {
> -        error_setg(errp, "-icount is not allowed with hardware virtualization");
> -    }
> +    configure_icount(opts, errp);
>      return 0;
>  }
>
> @@ -2854,6 +2850,9 @@ static void configure_accelerators(const char *progname)
>      bool init_failed = false;
>      AccelClass *acc = NULL;
>
> +    qemu_opts_foreach(qemu_find_opts("icount"),
> +                      do_configure_icount, NULL, &error_fatal);
> +
>      accel = qemu_opt_get(qemu_get_machine_opts(), "accel");
>      if (accel == NULL) {
>          /* Select the default accelerator */
> @@ -2901,11 +2900,13 @@ static void configure_accelerators(const char *progname)
>          error_report("Back to %s accelerator", acc->name);
>      }
>
> -    qemu_opts_foreach(qemu_find_opts("icount"),
> -                      do_configure_icount, NULL, &error_fatal);
> -
>      qemu_opts_foreach(qemu_find_opts("accel"),
>                        do_configure_accelerator, NULL, &error_fatal);
> +
> +    if (!tcg_enabled() && use_icount) {
> +        error_report("-icount is not allowed with hardware virtualization");
> +        exit(1);
> +    }
>  }
>
>  int main(int argc, char **argv, char **envp)
> --
> 1.8.3.1
>
>
>


--
Marc-André Lureau

Re: [PATCH 04/16] vl: move icount configuration earlier

[Thomas Huth]

On 13/11/2019 15.38, Paolo Bonzini wrote:
> Once qemu_tcg_configure is turned into a QOM property setter, it will not
> be able to set a default value for mttcg_enabled.  Setting the default will
> move to the TCG init_machine method, which currently runs after "-icount"
> is processed.
> 
> However, it is harmless to do configure_icount for all accelerators; we will
> just fail later if a non-TCG accelerator being selected.  So do that.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  vl.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)

Reviewed-by: Thomas Huth <thuth@redhat.com>

[PATCH 05/16] vl: introduce object_parse_property_opt

[Paolo Bonzini]

We will reuse the parsing loop of machine_set_property soon for "-accel",
but we do not want the "_" -> "-" conversion since "-accel" can just
standardize on dashes.  We will also add a bunch of legacy option handling
to keep the QOM machine object clean.  Extract the loop into a separate
function, and keep the legacy handling in machine_set_property.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 vl.c | 38 +++++++++++++++++++++++---------------
 1 file changed, 23 insertions(+), 15 deletions(-)

diff --git a/vl.c b/vl.c
index dbc99d7..b93217d 100644
--- a/vl.c
+++ b/vl.c
@@ -2617,27 +2617,17 @@ static MachineClass *select_machine(void)
     return machine_class;
 }
 
-static int machine_set_property(void *opaque,
-                                const char *name, const char *value,
-                                Error **errp)
+static int object_parse_property_opt(Object *obj,
+                                     const char *name, const char *value,
+                                     const char *skip, Error **errp)
 {
-    Object *obj = OBJECT(opaque);
     Error *local_err = NULL;
-    char *p, *qom_name;
 
-    if (strcmp(name, "type") == 0) {
+    if (g_str_equal(name, skip)) {
         return 0;
     }
 
-    qom_name = g_strdup(name);
-    for (p = qom_name; *p; p++) {
-        if (*p == '_') {
-            *p = '-';
-        }
-    }
-
-    object_property_parse(obj, value, qom_name, &local_err);
-    g_free(qom_name);
+    object_property_parse(obj, value, name, &local_err);
 
     if (local_err) {
         error_propagate(errp, local_err);
@@ -2647,6 +2637,24 @@ static int machine_set_property(void *opaque,
     return 0;
 }
 
+static int machine_set_property(void *opaque,
+                                const char *name, const char *value,
+                                Error **errp)
+{
+    char *qom_name = g_strdup(name);
+    char *p;
+    int r;
+
+    for (p = qom_name; *p; p++) {
+        if (*p == '_') {
+            *p = '-';
+        }
+    }
+
+    r = object_parse_property_opt(opaque, name, value, "type", errp);
+    g_free(qom_name);
+    return r;
+}
 
 /*
  * Initial object creation happens before all other
-- 
1.8.3.1

Re: [PATCH 05/16] vl: introduce object_parse_property_opt

[Marc-André Lureau]

On Wed, Nov 13, 2019 at 6:42 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> We will reuse the parsing loop of machine_set_property soon for "-accel",
> but we do not want the "_" -> "-" conversion since "-accel" can just
> standardize on dashes.  We will also add a bunch of legacy option handling
> to keep the QOM machine object clean.  Extract the loop into a separate
> function, and keep the legacy handling in machine_set_property.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  vl.c | 38 +++++++++++++++++++++++---------------
>  1 file changed, 23 insertions(+), 15 deletions(-)
>
> diff --git a/vl.c b/vl.c
> index dbc99d7..b93217d 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2617,27 +2617,17 @@ static MachineClass *select_machine(void)
>      return machine_class;
>  }
>
> -static int machine_set_property(void *opaque,
> -                                const char *name, const char *value,
> -                                Error **errp)
> +static int object_parse_property_opt(Object *obj,
> +                                     const char *name, const char *value,
> +                                     const char *skip, Error **errp)
>  {
> -    Object *obj = OBJECT(opaque);
>      Error *local_err = NULL;
> -    char *p, *qom_name;
>
> -    if (strcmp(name, "type") == 0) {
> +    if (g_str_equal(name, skip)) {
>          return 0;
>      }
>
> -    qom_name = g_strdup(name);
> -    for (p = qom_name; *p; p++) {
> -        if (*p == '_') {
> -            *p = '-';
> -        }
> -    }
> -
> -    object_property_parse(obj, value, qom_name, &local_err);
> -    g_free(qom_name);
> +    object_property_parse(obj, value, name, &local_err);
>
>      if (local_err) {
>          error_propagate(errp, local_err);
> @@ -2647,6 +2637,24 @@ static int machine_set_property(void *opaque,
>      return 0;
>  }
>
> +static int machine_set_property(void *opaque,
> +                                const char *name, const char *value,
> +                                Error **errp)
> +{
> +    char *qom_name = g_strdup(name);

Could use g_autofree, and thus return directly without r.



> +    char *p;
> +    int r;
> +
> +    for (p = qom_name; *p; p++) {
> +        if (*p == '_') {
> +            *p = '-';
> +        }
> +    }
> +
> +    r = object_parse_property_opt(opaque, name, value, "type", errp);

You want to pass qom_name, I guess

> +    g_free(qom_name);
> +    return r;
> +}
>
>  /*
>   * Initial object creation happens before all other
> --
> 1.8.3.1
>
>
>


--
Marc-André Lureau

Re: [PATCH 05/16] vl: introduce object_parse_property_opt

[Paolo Bonzini]

On 14/11/19 09:23, Marc-André Lureau wrote:
>> +{
>> +    char *qom_name = g_strdup(name);
> Could use g_autofree, and thus return directly without r.

And avoid a bunch of memory leaks in the rest of the series.

Paolo

[PATCH 06/16] vl: configure accelerators from -accel options

[Paolo Bonzini]

Drop the "accel" property from MachineState, and instead desugar
"-machine accel=" to a list of "-accel" options.

This has a semantic change due to removing merge_lists from -accel.
For example:

- "-accel kvm -accel tcg" all but ignored "-accel kvm".  This is a bugfix.

- "-accel kvm -accel thread=single" ignored "thread=single", since it
  applied the option to KVM.  Now it fails due to not specifying the
  accelerator on "-accel thread=single".

- "-accel tcg -accel thread=single" chose single-threaded TCG, while now
  it will fail due to not specifying the accelerator on "-accel
  thread=single".

Also, "-machine accel" and "-accel" become incompatible.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/core/machine.c   | 21 ------------
 include/hw/boards.h |  1 -
 vl.c                | 93 +++++++++++++++++++++++++++++++----------------------
 3 files changed, 54 insertions(+), 61 deletions(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 1689ad3..45ddfb6 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -173,21 +173,6 @@ GlobalProperty hw_compat_2_1[] = {
 };
 const size_t hw_compat_2_1_len = G_N_ELEMENTS(hw_compat_2_1);
 
-static char *machine_get_accel(Object *obj, Error **errp)
-{
-    MachineState *ms = MACHINE(obj);
-
-    return g_strdup(ms->accel);
-}
-
-static void machine_set_accel(Object *obj, const char *value, Error **errp)
-{
-    MachineState *ms = MACHINE(obj);
-
-    g_free(ms->accel);
-    ms->accel = g_strdup(value);
-}
-
 static void machine_set_kernel_irqchip(Object *obj, Visitor *v,
                                        const char *name, void *opaque,
                                        Error **errp)
@@ -808,11 +793,6 @@ static void machine_class_init(ObjectClass *oc, void *data)
     mc->numa_mem_align_shift = 23;
     mc->numa_auto_assign_ram = numa_default_auto_assign_ram;
 
-    object_class_property_add_str(oc, "accel",
-        machine_get_accel, machine_set_accel, &error_abort);
-    object_class_property_set_description(oc, "accel",
-        "Accelerator list", &error_abort);
-
     object_class_property_add(oc, "kernel-irqchip", "on|off|split",
         NULL, machine_set_kernel_irqchip,
         NULL, NULL, &error_abort);
@@ -971,7 +951,6 @@ static void machine_finalize(Object *obj)
 {
     MachineState *ms = MACHINE(obj);
 
-    g_free(ms->accel);
     g_free(ms->kernel_filename);
     g_free(ms->initrd_filename);
     g_free(ms->kernel_cmdline);
diff --git a/include/hw/boards.h b/include/hw/boards.h
index de45087..36fcbda 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -275,7 +275,6 @@ struct MachineState {
 
     /*< public >*/
 
-    char *accel;
     bool kernel_irqchip_allowed;
     bool kernel_irqchip_required;
     bool kernel_irqchip_split;
diff --git a/vl.c b/vl.c
index b93217d..dd895db 100644
--- a/vl.c
+++ b/vl.c
@@ -293,7 +293,6 @@ static QemuOptsList qemu_accel_opts = {
     .name = "accel",
     .implied_opt_name = "accel",
     .head = QTAILQ_HEAD_INITIALIZER(qemu_accel_opts.head),
-    .merge_lists = true,
     .desc = {
         {
             .name = "accel",
@@ -2651,6 +2650,11 @@ static int machine_set_property(void *opaque,
         }
     }
 
+    /* Legacy options do not correspond to MachineState properties.  */
+    if (g_str_equal(qom_name, "accel")) {
+        return 0;
+    }
+
     r = object_parse_property_opt(opaque, name, value, "type", errp);
     g_free(qom_name);
     return r;
@@ -2843,74 +2847,88 @@ static int do_configure_icount(void *opaque, QemuOpts *opts, Error **errp)
 
 static int do_configure_accelerator(void *opaque, QemuOpts *opts, Error **errp)
 {
+    bool *p_init_failed = opaque;
+    const char *acc = qemu_opt_get(opts, "accel");
+    AccelClass *ac = accel_find(acc);
+    int ret;
+
+    if (!ac) {
+        return 0;
+    }
+    ret = accel_init_machine(ac, current_machine);
+    if (ret < 0) {
+        *p_init_failed = true;
+        error_report("failed to initialize %s: %s",
+                     acc, strerror(-ret));
+        return 0;
+    }
+
     if (tcg_enabled()) {
         qemu_tcg_configure(opts, &error_fatal);
     }
-    return 0;
+    return 1;
 }
 
 static void configure_accelerators(const char *progname)
 {
     const char *accel;
     char **accel_list, **tmp;
-    int ret;
     bool accel_initialised = false;
     bool init_failed = false;
-    AccelClass *acc = NULL;
 
     qemu_opts_foreach(qemu_find_opts("icount"),
                       do_configure_icount, NULL, &error_fatal);
 
     accel = qemu_opt_get(qemu_get_machine_opts(), "accel");
-    if (accel == NULL) {
-        /* Select the default accelerator */
-        if (!accel_find("tcg") && !accel_find("kvm")) {
-            error_report("No accelerator selected and"
-                         " no default accelerator available");
-            exit(1);
-        } else {
-            int pnlen = strlen(progname);
-            if (pnlen >= 3 && g_str_equal(&progname[pnlen - 3], "kvm")) {
-                /* If the program name ends with "kvm", we prefer KVM */
-                accel = "kvm:tcg";
+    if (QTAILQ_EMPTY(&qemu_accel_opts.head)) {
+        if (accel == NULL) {
+            /* Select the default accelerator */
+            if (!accel_find("tcg") && !accel_find("kvm")) {
+                error_report("No accelerator selected and"
+                             " no default accelerator available");
+                exit(1);
             } else {
-                accel = "tcg:kvm";
+                int pnlen = strlen(progname);
+                if (pnlen >= 3 && g_str_equal(&progname[pnlen - 3], "kvm")) {
+                    /* If the program name ends with "kvm", we prefer KVM */
+                    accel = "kvm:tcg";
+                } else {
+                    accel = "tcg:kvm";
+                }
             }
         }
-    }
 
-    accel_list = g_strsplit(accel, ":", 0);
+        accel_list = g_strsplit(accel, ":", 0);
 
-    for (tmp = accel_list; !accel_initialised && tmp && *tmp; tmp++) {
-        acc = accel_find(*tmp);
-        if (!acc) {
-            continue;
+        for (tmp = accel_list; !accel_initialised && tmp && *tmp; tmp++) {
+            /*
+             * Filter invalid accelerators here, to prevent obscenities
+             * such as "-machine accel=tcg,,thread=single".
+             */
+            if (accel_find(*tmp)) {
+                qemu_opts_parse_noisily(qemu_find_opts("accel"), *tmp, true);
+            }
         }
-        ret = accel_init_machine(acc, current_machine);
-        if (ret < 0) {
-            init_failed = true;
-            error_report("failed to initialize %s: %s",
-                         acc->name, strerror(-ret));
-        } else {
-            accel_initialised = true;
+    } else {
+        if (accel != NULL) {
+            error_report("The -accel and \"-machine accel=\" options are incompatible");
+            exit(1);
         }
     }
-    g_strfreev(accel_list);
 
-    if (!accel_initialised) {
+    if (!qemu_opts_foreach(qemu_find_opts("accel"),
+                           do_configure_accelerator, &init_failed, &error_fatal)) {
         if (!init_failed) {
-            error_report("-machine accel=%s: No accelerator found", accel);
+            error_report("no accelerator found");
         }
         exit(1);
     }
 
     if (init_failed) {
-        error_report("Back to %s accelerator", acc->name);
+        AccelClass *ac = ACCEL_GET_CLASS(current_machine->accelerator);
+        error_report("Back to %s accelerator", ac->name);
     }
 
-    qemu_opts_foreach(qemu_find_opts("accel"),
-                      do_configure_accelerator, NULL, &error_fatal);
-
     if (!tcg_enabled() && use_icount) {
         error_report("-icount is not allowed with hardware virtualization");
         exit(1);
@@ -3618,9 +3636,6 @@ int main(int argc, char **argv, char **envp)
                                  "use -M accel=... for now instead");
                     exit(1);
                 }
-                opts = qemu_opts_create(qemu_find_opts("machine"), NULL,
-                                        false, &error_abort);
-                qemu_opt_set(opts, "accel", optarg, &error_abort);
                 break;
             case QEMU_OPTION_usb:
                 olist = qemu_find_opts("machine");
-- 
1.8.3.1

Re: [PATCH 06/16] vl: configure accelerators from -accel options

[Wainer dos Santos Moschetta]

On 11/13/19 12:38 PM, Paolo Bonzini wrote:
> Drop the "accel" property from MachineState, and instead desugar
> "-machine accel=" to a list of "-accel" options.
>
> This has a semantic change due to removing merge_lists from -accel.
> For example:
>
> - "-accel kvm -accel tcg" all but ignored "-accel kvm".  This is a bugfix.
>
> - "-accel kvm -accel thread=single" ignored "thread=single", since it
>    applied the option to KVM.  Now it fails due to not specifying the
>    accelerator on "-accel thread=single".
>
> - "-accel tcg -accel thread=single" chose single-threaded TCG, while now
>    it will fail due to not specifying the accelerator on "-accel
>    thread=single".
>
> Also, "-machine accel" and "-accel" become incompatible.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   hw/core/machine.c   | 21 ------------
>   include/hw/boards.h |  1 -
>   vl.c                | 93 +++++++++++++++++++++++++++++++----------------------
>   3 files changed, 54 insertions(+), 61 deletions(-)
>
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 1689ad3..45ddfb6 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -173,21 +173,6 @@ GlobalProperty hw_compat_2_1[] = {
>   };
>   const size_t hw_compat_2_1_len = G_N_ELEMENTS(hw_compat_2_1);
>   
> -static char *machine_get_accel(Object *obj, Error **errp)
> -{
> -    MachineState *ms = MACHINE(obj);
> -
> -    return g_strdup(ms->accel);
> -}
> -
> -static void machine_set_accel(Object *obj, const char *value, Error **errp)
> -{
> -    MachineState *ms = MACHINE(obj);
> -
> -    g_free(ms->accel);
> -    ms->accel = g_strdup(value);
> -}
> -
>   static void machine_set_kernel_irqchip(Object *obj, Visitor *v,
>                                          const char *name, void *opaque,
>                                          Error **errp)
> @@ -808,11 +793,6 @@ static void machine_class_init(ObjectClass *oc, void *data)
>       mc->numa_mem_align_shift = 23;
>       mc->numa_auto_assign_ram = numa_default_auto_assign_ram;
>   
> -    object_class_property_add_str(oc, "accel",
> -        machine_get_accel, machine_set_accel, &error_abort);
> -    object_class_property_set_description(oc, "accel",
> -        "Accelerator list", &error_abort);
> -
>       object_class_property_add(oc, "kernel-irqchip", "on|off|split",
>           NULL, machine_set_kernel_irqchip,
>           NULL, NULL, &error_abort);
> @@ -971,7 +951,6 @@ static void machine_finalize(Object *obj)
>   {
>       MachineState *ms = MACHINE(obj);
>   
> -    g_free(ms->accel);
>       g_free(ms->kernel_filename);
>       g_free(ms->initrd_filename);
>       g_free(ms->kernel_cmdline);
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index de45087..36fcbda 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -275,7 +275,6 @@ struct MachineState {
>   
>       /*< public >*/
>   
> -    char *accel;
>       bool kernel_irqchip_allowed;
>       bool kernel_irqchip_required;
>       bool kernel_irqchip_split;
> diff --git a/vl.c b/vl.c
> index b93217d..dd895db 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -293,7 +293,6 @@ static QemuOptsList qemu_accel_opts = {
>       .name = "accel",
>       .implied_opt_name = "accel",
>       .head = QTAILQ_HEAD_INITIALIZER(qemu_accel_opts.head),
> -    .merge_lists = true,
>       .desc = {
>           {
>               .name = "accel",
> @@ -2651,6 +2650,11 @@ static int machine_set_property(void *opaque,
>           }
>       }
>   
> +    /* Legacy options do not correspond to MachineState properties.  */
> +    if (g_str_equal(qom_name, "accel")) {
> +        return 0;
> +    }
> +
>       r = object_parse_property_opt(opaque, name, value, "type", errp);
>       g_free(qom_name);
>       return r;
> @@ -2843,74 +2847,88 @@ static int do_configure_icount(void *opaque, QemuOpts *opts, Error **errp)
>   
>   static int do_configure_accelerator(void *opaque, QemuOpts *opts, Error **errp)
>   {
> +    bool *p_init_failed = opaque;
> +    const char *acc = qemu_opt_get(opts, "accel");
> +    AccelClass *ac = accel_find(acc);
> +    int ret;
> +
> +    if (!ac) {
> +        return 0;
> +    }
> +    ret = accel_init_machine(ac, current_machine);
> +    if (ret < 0) {
> +        *p_init_failed = true;
> +        error_report("failed to initialize %s: %s",
> +                     acc, strerror(-ret));
> +        return 0;
> +    }
> +
>       if (tcg_enabled()) {
>           qemu_tcg_configure(opts, &error_fatal);
>       }
> -    return 0;
> +    return 1;
>   }
>   
>   static void configure_accelerators(const char *progname)
>   {
>       const char *accel;
>       char **accel_list, **tmp;
> -    int ret;
>       bool accel_initialised = false;
>       bool init_failed = false;
> -    AccelClass *acc = NULL;
>   
>       qemu_opts_foreach(qemu_find_opts("icount"),
>                         do_configure_icount, NULL, &error_fatal);
>   
>       accel = qemu_opt_get(qemu_get_machine_opts(), "accel");
> -    if (accel == NULL) {
> -        /* Select the default accelerator */
> -        if (!accel_find("tcg") && !accel_find("kvm")) {
> -            error_report("No accelerator selected and"
> -                         " no default accelerator available");

Perhaps on patch 07 you can also clarify this message, mentioning what's 
the default accelerator.

> -            exit(1);
> -        } else {
> -            int pnlen = strlen(progname);
> -            if (pnlen >= 3 && g_str_equal(&progname[pnlen - 3], "kvm")) {
> -                /* If the program name ends with "kvm", we prefer KVM */
> -                accel = "kvm:tcg";
> +    if (QTAILQ_EMPTY(&qemu_accel_opts.head)) {
> +        if (accel == NULL) {
> +            /* Select the default accelerator */
> +            if (!accel_find("tcg") && !accel_find("kvm")) {
> +                error_report("No accelerator selected and"
> +                             " no default accelerator available");
> +                exit(1);
>               } else {
> -                accel = "tcg:kvm";
> +                int pnlen = strlen(progname);
> +                if (pnlen >= 3 && g_str_equal(&progname[pnlen - 3], "kvm")) {
> +                    /* If the program name ends with "kvm", we prefer KVM */
> +                    accel = "kvm:tcg";
> +                } else {
> +                    accel = "tcg:kvm";
> +                }
>               }
>           }
> -    }
>   
> -    accel_list = g_strsplit(accel, ":", 0);
> +        accel_list = g_strsplit(accel, ":", 0);
>   
> -    for (tmp = accel_list; !accel_initialised && tmp && *tmp; tmp++) {
> -        acc = accel_find(*tmp);
> -        if (!acc) {
> -            continue;
> +        for (tmp = accel_list; !accel_initialised && tmp && *tmp; tmp++) {
> +            /*
> +             * Filter invalid accelerators here, to prevent obscenities
> +             * such as "-machine accel=tcg,,thread=single".
> +             */
> +            if (accel_find(*tmp)) {
> +                qemu_opts_parse_noisily(qemu_find_opts("accel"), *tmp, true);
> +            }
>           }
> -        ret = accel_init_machine(acc, current_machine);
> -        if (ret < 0) {
> -            init_failed = true;
> -            error_report("failed to initialize %s: %s",
> -                         acc->name, strerror(-ret));
> -        } else {
> -            accel_initialised = true;
> +    } else {
> +        if (accel != NULL) {
> +            error_report("The -accel and \"-machine accel=\" options are incompatible");
> +            exit(1);
>           }
>       }
> -    g_strfreev(accel_list);
>   
> -    if (!accel_initialised) {
> +    if (!qemu_opts_foreach(qemu_find_opts("accel"),
> +                           do_configure_accelerator, &init_failed, &error_fatal)) {
>           if (!init_failed) {
> -            error_report("-machine accel=%s: No accelerator found", accel);
> +            error_report("no accelerator found");

Likewise.

Thanks,

Wainer

>           }
>           exit(1);
>       }
>   
>       if (init_failed) {
> -        error_report("Back to %s accelerator", acc->name);
> +        AccelClass *ac = ACCEL_GET_CLASS(current_machine->accelerator);
> +        error_report("Back to %s accelerator", ac->name);
>       }
>   
> -    qemu_opts_foreach(qemu_find_opts("accel"),
> -                      do_configure_accelerator, NULL, &error_fatal);
> -
>       if (!tcg_enabled() && use_icount) {
>           error_report("-icount is not allowed with hardware virtualization");
>           exit(1);
> @@ -3618,9 +3636,6 @@ int main(int argc, char **argv, char **envp)
>                                    "use -M accel=... for now instead");
>                       exit(1);
>                   }
> -                opts = qemu_opts_create(qemu_find_opts("machine"), NULL,
> -                                        false, &error_abort);
> -                qemu_opt_set(opts, "accel", optarg, &error_abort);
>                   break;
>               case QEMU_OPTION_usb:
>                   olist = qemu_find_opts("machine");

[PATCH 07/16] vl: warn for unavailable accelerators, clarify messages

[Paolo Bonzini]

So far, specifying an accelerator that was not compiled in did not result
in an error; fix that.

While at it, clarify the mysterious "Back to TCG" message.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 vl.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/vl.c b/vl.c
index dd895db..843b263 100644
--- a/vl.c
+++ b/vl.c
@@ -2853,6 +2853,8 @@ static int do_configure_accelerator(void *opaque, QemuOpts *opts, Error **errp)
     int ret;
 
     if (!ac) {
+        *p_init_failed = true;
+        error_report("invalid accelerator %s", acc);
         return 0;
     }
     ret = accel_init_machine(ac, current_machine);
@@ -2907,6 +2909,9 @@ static void configure_accelerators(const char *progname)
              */
             if (accel_find(*tmp)) {
                 qemu_opts_parse_noisily(qemu_find_opts("accel"), *tmp, true);
+            } else {
+                init_failed = true;
+                error_report("invalid accelerator %s", *tmp);
             }
         }
     } else {
@@ -2926,7 +2931,7 @@ static void configure_accelerators(const char *progname)
 
     if (init_failed) {
         AccelClass *ac = ACCEL_GET_CLASS(current_machine->accelerator);
-        error_report("Back to %s accelerator", ac->name);
+        error_report("falling back to %s", ac->name);
     }
 
     if (!tcg_enabled() && use_icount) {
-- 
1.8.3.1

Re: [PATCH 07/16] vl: warn for unavailable accelerators, clarify messages

[Marc-André Lureau]

Hi

On Wed, Nov 13, 2019 at 6:39 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> So far, specifying an accelerator that was not compiled in did not result
> in an error; fix that.
>
> While at it, clarify the mysterious "Back to TCG" message.

I was wondering why this was not addressed in previous patch :)

>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> ---
>  vl.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/vl.c b/vl.c
> index dd895db..843b263 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2853,6 +2853,8 @@ static int do_configure_accelerator(void *opaque, QemuOpts *opts, Error **errp)
>      int ret;
>
>      if (!ac) {
> +        *p_init_failed = true;
> +        error_report("invalid accelerator %s", acc);
>          return 0;
>      }
>      ret = accel_init_machine(ac, current_machine);
> @@ -2907,6 +2909,9 @@ static void configure_accelerators(const char *progname)
>               */
>              if (accel_find(*tmp)) {
>                  qemu_opts_parse_noisily(qemu_find_opts("accel"), *tmp, true);
> +            } else {
> +                init_failed = true;
> +                error_report("invalid accelerator %s", *tmp);
>              }
>          }
>      } else {
> @@ -2926,7 +2931,7 @@ static void configure_accelerators(const char *progname)
>
>      if (init_failed) {
>          AccelClass *ac = ACCEL_GET_CLASS(current_machine->accelerator);
> -        error_report("Back to %s accelerator", ac->name);
> +        error_report("falling back to %s", ac->name);
>      }
>
>      if (!tcg_enabled() && use_icount) {
> --
> 1.8.3.1
>
>
>


-- 
Marc-André Lureau

[PATCH 08/16] qom: introduce object_register_sugar_prop

[Paolo Bonzini]

Similar to the existing "-rtc driftfix" option, we will convert some
legacy "-machine" command line options to global properties on accelerators.
Because accelerators are not devices, we cannot use qdev_prop_register_global.
Instead, provide a slot in the generic object_compat_props arrays for
command line syntactic sugar.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/qom/object.h |  1 +
 qom/object.c         | 23 +++++++++++++++++++++--
 vl.c                 | 10 +++-------
 3 files changed, 25 insertions(+), 9 deletions(-)

diff --git a/include/qom/object.h b/include/qom/object.h
index 128d00c..230b18f 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -679,6 +679,7 @@ void object_apply_global_props(Object *obj, const GPtrArray *props,
                                Error **errp);
 void object_set_machine_compat_props(GPtrArray *compat_props);
 void object_set_accelerator_compat_props(GPtrArray *compat_props);
+void object_register_sugar_prop(const char *driver, const char *prop, const char *value);
 void object_apply_compat_props(Object *obj);
 
 /**
diff --git a/qom/object.c b/qom/object.c
index 6fa9c61..c7825dd 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -414,10 +414,29 @@ void object_apply_global_props(Object *obj, const GPtrArray *props, Error **errp
  * Global property defaults
  * Slot 0: accelerator's global property defaults
  * Slot 1: machine's global property defaults
+ * Slot 2: global properties from legacy command line option
  * Each is a GPtrArray of of GlobalProperty.
  * Applied in order, later entries override earlier ones.
  */
-static GPtrArray *object_compat_props[2];
+static GPtrArray *object_compat_props[3];
+
+/*
+ * Retrieve @GPtrArray for global property defined with options
+ * other than "-global".  These are generally used for syntactic
+ * sugar and legacy command line options.
+ */
+void object_register_sugar_prop(const char *driver, const char *prop, const char *value)
+{
+    GlobalProperty *g;
+    if (!object_compat_props[2]) {
+        object_compat_props[2] = g_ptr_array_new();
+    }
+    g = g_new(GlobalProperty, 1);
+    g->driver = g_strdup(driver);
+    g->property = g_strdup(prop);
+    g->value = g_strdup(value);
+    g_ptr_array_add(object_compat_props[2], g);
+}
 
 /*
  * Set machine's global property defaults to @compat_props.
@@ -445,7 +464,7 @@ void object_apply_compat_props(Object *obj)
 
     for (i = 0; i < ARRAY_SIZE(object_compat_props); i++) {
         object_apply_global_props(obj, object_compat_props[i],
-                                  &error_abort);
+                                  i == 2 ? &error_fatal : &error_abort);
     }
 }
 
diff --git a/vl.c b/vl.c
index 843b263..cb993dd 100644
--- a/vl.c
+++ b/vl.c
@@ -896,13 +896,9 @@ static void configure_rtc(QemuOpts *opts)
     value = qemu_opt_get(opts, "driftfix");
     if (value) {
         if (!strcmp(value, "slew")) {
-            static GlobalProperty slew_lost_ticks = {
-                .driver   = "mc146818rtc",
-                .property = "lost_tick_policy",
-                .value    = "slew",
-            };
-
-            qdev_prop_register_global(&slew_lost_ticks);
+            object_register_sugar_prop("mc146818rtc",
+                                       "lost_tick_policy",
+                                       "slew");
         } else if (!strcmp(value, "none")) {
             /* discard is default */
         } else {
-- 
1.8.3.1

Re: [PATCH 08/16] qom: introduce object_register_sugar_prop

[Marc-André Lureau]

Hi

On Wed, Nov 13, 2019 at 6:46 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> Similar to the existing "-rtc driftfix" option, we will convert some
> legacy "-machine" command line options to global properties on accelerators.
> Because accelerators are not devices, we cannot use qdev_prop_register_global.
> Instead, provide a slot in the generic object_compat_props arrays for
> command line syntactic sugar.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

sounds reasonable

> ---
>  include/qom/object.h |  1 +
>  qom/object.c         | 23 +++++++++++++++++++++--
>  vl.c                 | 10 +++-------
>  3 files changed, 25 insertions(+), 9 deletions(-)
>
> diff --git a/include/qom/object.h b/include/qom/object.h
> index 128d00c..230b18f 100644
> --- a/include/qom/object.h
> +++ b/include/qom/object.h
> @@ -679,6 +679,7 @@ void object_apply_global_props(Object *obj, const GPtrArray *props,
>                                 Error **errp);
>  void object_set_machine_compat_props(GPtrArray *compat_props);
>  void object_set_accelerator_compat_props(GPtrArray *compat_props);
> +void object_register_sugar_prop(const char *driver, const char *prop, const char *value);

Or simply

void object_add_global_prop(const char *typename, ...) ?


>  void object_apply_compat_props(Object *obj);
>
>  /**
> diff --git a/qom/object.c b/qom/object.c
> index 6fa9c61..c7825dd 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -414,10 +414,29 @@ void object_apply_global_props(Object *obj, const GPtrArray *props, Error **errp
>   * Global property defaults
>   * Slot 0: accelerator's global property defaults
>   * Slot 1: machine's global property defaults
> + * Slot 2: global properties from legacy command line option
>   * Each is a GPtrArray of of GlobalProperty.
>   * Applied in order, later entries override earlier ones.
>   */
> -static GPtrArray *object_compat_props[2];
> +static GPtrArray *object_compat_props[3];
> +
> +/*
> + * Retrieve @GPtrArray for global property defined with options
> + * other than "-global".  These are generally used for syntactic
> + * sugar and legacy command line options.
> + */
> +void object_register_sugar_prop(const char *driver, const char *prop, const char *value)
> +{
> +    GlobalProperty *g;
> +    if (!object_compat_props[2]) {
> +        object_compat_props[2] = g_ptr_array_new();
> +    }
> +    g = g_new(GlobalProperty, 1);
> +    g->driver = g_strdup(driver);
> +    g->property = g_strdup(prop);
> +    g->value = g_strdup(value);
> +    g_ptr_array_add(object_compat_props[2], g);
> +}
>
>  /*
>   * Set machine's global property defaults to @compat_props.
> @@ -445,7 +464,7 @@ void object_apply_compat_props(Object *obj)
>
>      for (i = 0; i < ARRAY_SIZE(object_compat_props); i++) {
>          object_apply_global_props(obj, object_compat_props[i],
> -                                  &error_abort);
> +                                  i == 2 ? &error_fatal : &error_abort);

Isn't error_abort() appropriate in all cases?


>      }
>  }
>
> diff --git a/vl.c b/vl.c
> index 843b263..cb993dd 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -896,13 +896,9 @@ static void configure_rtc(QemuOpts *opts)
>      value = qemu_opt_get(opts, "driftfix");
>      if (value) {
>          if (!strcmp(value, "slew")) {
> -            static GlobalProperty slew_lost_ticks = {
> -                .driver   = "mc146818rtc",
> -                .property = "lost_tick_policy",
> -                .value    = "slew",
> -            };
> -
> -            qdev_prop_register_global(&slew_lost_ticks);
> +            object_register_sugar_prop("mc146818rtc",
> +                                       "lost_tick_policy",
> +                                       "slew");

Why do you convert this since it's a device?

>          } else if (!strcmp(value, "none")) {
>              /* discard is default */
>          } else {
> --
> 1.8.3.1
>
>
>


-- 
Marc-André Lureau

Re: [PATCH 08/16] qom: introduce object_register_sugar_prop

[Paolo Bonzini]

On 14/11/19 10:53, Marc-André Lureau wrote:
>>  include/qom/object.h |  1 +
>>  qom/object.c         | 23 +++++++++++++++++++++--
>>  vl.c                 | 10 +++-------
>>  3 files changed, 25 insertions(+), 9 deletions(-)
>>
>> diff --git a/include/qom/object.h b/include/qom/object.h
>> index 128d00c..230b18f 100644
>> --- a/include/qom/object.h
>> +++ b/include/qom/object.h
>> @@ -679,6 +679,7 @@ void object_apply_global_props(Object *obj, const GPtrArray *props,
>>                                 Error **errp);
>>  void object_set_machine_compat_props(GPtrArray *compat_props);
>>  void object_set_accelerator_compat_props(GPtrArray *compat_props);
>> +void object_register_sugar_prop(const char *driver, const char *prop, const char *value);
> 
> Or simply
> 
> void object_add_global_prop(const char *typename, ...) ?

This is actually how I called it first, but I didn't like it because
it's prioritized _below_ -global, and it's easy to confuse it with
object_add_global_prop.

>>  /*
>>   * Set machine's global property defaults to @compat_props.
>> @@ -445,7 +464,7 @@ void object_apply_compat_props(Object *obj)
>>
>>      for (i = 0; i < ARRAY_SIZE(object_compat_props); i++) {
>>          object_apply_global_props(obj, object_compat_props[i],
>> -                                  &error_abort);
>> +                                  i == 2 ? &error_fatal : &error_abort);
> 
> Isn't error_abort() appropriate in all cases?

Unfortunately not, because otherwise "-accel tcg,tb-size=foo" would crash.

>>      }
>>  }
>>
>> diff --git a/vl.c b/vl.c
>> index 843b263..cb993dd 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -896,13 +896,9 @@ static void configure_rtc(QemuOpts *opts)
>>      value = qemu_opt_get(opts, "driftfix");
>>      if (value) {
>>          if (!strcmp(value, "slew")) {
>> -            static GlobalProperty slew_lost_ticks = {
>> -                .driver   = "mc146818rtc",
>> -                .property = "lost_tick_policy",
>> -                .value    = "slew",
>> -            };
>> -
>> -            qdev_prop_register_global(&slew_lost_ticks);
>> +            object_register_sugar_prop("mc146818rtc",
>> +                                       "lost_tick_policy",
>> +                                       "slew");
> 
> Why do you convert this since it's a device?

Not strictly necessary, but it's more compact and it more or less
matches the usecase for this function.

Paolo

[PATCH 09/16] qom: add object_new_with_class

[Paolo Bonzini]

Similar to CPU and machine classes, "-accel" class names are mangled,
so we have to first get a class via accel_find and then instantiate it.
Provide a new function to instantiate a class without going through
object_class_get_name, and use it for CPUs and machines already.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/qom/object.h      | 12 ++++++++++++
 qom/object.c              |  5 +++++
 target/i386/cpu.c         |  8 ++++----
 target/s390x/cpu_models.c |  4 ++--
 vl.c                      |  3 +--
 5 files changed, 24 insertions(+), 8 deletions(-)

diff --git a/include/qom/object.h b/include/qom/object.h
index 230b18f..f9ad692 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -593,6 +593,18 @@ struct InterfaceClass
                                              __FILE__, __LINE__, __func__))
 
 /**
+ * object_new_with_class:
+ * @klass: The class to instantiate.
+ *
+ * This function will initialize a new object using heap allocated memory.
+ * The returned object has a reference count of 1, and will be freed when
+ * the last reference is dropped.
+ *
+ * Returns: The newly allocated and instantiated object.
+ */
+Object *object_new_with_class(ObjectClass *klass);
+
+/**
  * object_new:
  * @typename: The name of the type of the object to instantiate.
  *
diff --git a/qom/object.c b/qom/object.c
index c7825dd..ee7708e 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -658,6 +658,11 @@ static Object *object_new_with_type(Type type)
     return obj;
 }
 
+Object *object_new_with_class(ObjectClass *klass)
+{
+    return object_new_with_type(klass->type);
+}
+
 Object *object_new(const char *typename)
 {
     TypeImpl *ti = type_get_by_name(typename);
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index a624163..4742a0e 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -3881,7 +3881,7 @@ static void x86_cpu_class_check_missing_features(X86CPUClass *xcc,
         return;
     }
 
-    xc = X86_CPU(object_new(object_class_get_name(OBJECT_CLASS(xcc))));
+    xc = X86_CPU(object_new_with_class(OBJECT_CLASS(xcc)));
 
     x86_cpu_expand_features(xc, &err);
     if (err) {
@@ -3952,7 +3952,7 @@ static GSList *get_sorted_cpu_model_list(void)
 
 static char *x86_cpu_class_get_model_id(X86CPUClass *xc)
 {
-    Object *obj = object_new(object_class_get_name(OBJECT_CLASS(xc)));
+    Object *obj = object_new_with_class(OBJECT_CLASS(xc));
     char *r = object_property_get_str(obj, "model-id", &error_abort);
     object_unref(obj);
     return r;
@@ -4333,7 +4333,7 @@ static X86CPU *x86_cpu_from_model(const char *model, QDict *props, Error **errp)
         goto out;
     }
 
-    xc = X86_CPU(object_new(object_class_get_name(OBJECT_CLASS(xcc))));
+    xc = X86_CPU(object_new_with_class(OBJECT_CLASS(xcc)));
     if (props) {
         object_apply_props(OBJECT(xc), props, &err);
         if (err) {
@@ -5177,7 +5177,7 @@ static void x86_cpu_apic_create(X86CPU *cpu, Error **errp)
     APICCommonState *apic;
     ObjectClass *apic_class = OBJECT_CLASS(apic_get_class());
 
-    cpu->apic_state = DEVICE(object_new(object_class_get_name(apic_class)));
+    cpu->apic_state = DEVICE(object_new_with_class(apic_class));
 
     object_property_add_child(OBJECT(cpu), "lapic",
                               OBJECT(cpu->apic_state), &error_abort);
diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
index 7e92fb2..72cf48b 100644
--- a/target/s390x/cpu_models.c
+++ b/target/s390x/cpu_models.c
@@ -440,7 +440,7 @@ static void create_cpu_model_list(ObjectClass *klass, void *opaque)
     if (cpu_list_data->model) {
         Object *obj;
         S390CPU *sc;
-        obj = object_new(object_class_get_name(klass));
+        obj = object_new_with_class(klass);
         sc = S390_CPU(obj);
         if (sc->model) {
             info->has_unavailable_features = true;
@@ -501,7 +501,7 @@ static void cpu_model_from_info(S390CPUModel *model, const CpuModelInfo *info,
         error_setg(errp, "The CPU definition '%s' requires KVM", info->name);
         return;
     }
-    obj = object_new(object_class_get_name(oc));
+    obj = object_new_with_class(oc);
     cpu = S390_CPU(obj);
 
     if (!cpu->model) {
diff --git a/vl.c b/vl.c
index cb993dd..6e406d4 100644
--- a/vl.c
+++ b/vl.c
@@ -3988,8 +3988,7 @@ int main(int argc, char **argv, char **envp)
                       cleanup_add_fd, NULL, &error_fatal);
 #endif
 
-    current_machine = MACHINE(object_new(object_class_get_name(
-                          OBJECT_CLASS(machine_class))));
+    current_machine = MACHINE(object_new_with_class(OBJECT_CLASS(machine_class)));
     if (machine_help_func(qemu_get_machine_opts(), current_machine)) {
         exit(0);
     }
-- 
1.8.3.1

Re: [PATCH 09/16] qom: add object_new_with_class

[Marc-André Lureau]

On Wed, Nov 13, 2019 at 6:48 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> Similar to CPU and machine classes, "-accel" class names are mangled,
> so we have to first get a class via accel_find and then instantiate it.
> Provide a new function to instantiate a class without going through
> object_class_get_name, and use it for CPUs and machines already.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> ---
>  include/qom/object.h      | 12 ++++++++++++
>  qom/object.c              |  5 +++++
>  target/i386/cpu.c         |  8 ++++----
>  target/s390x/cpu_models.c |  4 ++--
>  vl.c                      |  3 +--
>  5 files changed, 24 insertions(+), 8 deletions(-)
>
> diff --git a/include/qom/object.h b/include/qom/object.h
> index 230b18f..f9ad692 100644
> --- a/include/qom/object.h
> +++ b/include/qom/object.h
> @@ -593,6 +593,18 @@ struct InterfaceClass
>                                               __FILE__, __LINE__, __func__))
>
>  /**
> + * object_new_with_class:
> + * @klass: The class to instantiate.
> + *
> + * This function will initialize a new object using heap allocated memory.
> + * The returned object has a reference count of 1, and will be freed when
> + * the last reference is dropped.
> + *
> + * Returns: The newly allocated and instantiated object.
> + */
> +Object *object_new_with_class(ObjectClass *klass);
> +
> +/**
>   * object_new:
>   * @typename: The name of the type of the object to instantiate.
>   *
> diff --git a/qom/object.c b/qom/object.c
> index c7825dd..ee7708e 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -658,6 +658,11 @@ static Object *object_new_with_type(Type type)
>      return obj;
>  }
>
> +Object *object_new_with_class(ObjectClass *klass)
> +{
> +    return object_new_with_type(klass->type);
> +}
> +
>  Object *object_new(const char *typename)
>  {
>      TypeImpl *ti = type_get_by_name(typename);
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index a624163..4742a0e 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -3881,7 +3881,7 @@ static void x86_cpu_class_check_missing_features(X86CPUClass *xcc,
>          return;
>      }
>
> -    xc = X86_CPU(object_new(object_class_get_name(OBJECT_CLASS(xcc))));
> +    xc = X86_CPU(object_new_with_class(OBJECT_CLASS(xcc)));
>
>      x86_cpu_expand_features(xc, &err);
>      if (err) {
> @@ -3952,7 +3952,7 @@ static GSList *get_sorted_cpu_model_list(void)
>
>  static char *x86_cpu_class_get_model_id(X86CPUClass *xc)
>  {
> -    Object *obj = object_new(object_class_get_name(OBJECT_CLASS(xc)));
> +    Object *obj = object_new_with_class(OBJECT_CLASS(xc));
>      char *r = object_property_get_str(obj, "model-id", &error_abort);
>      object_unref(obj);
>      return r;
> @@ -4333,7 +4333,7 @@ static X86CPU *x86_cpu_from_model(const char *model, QDict *props, Error **errp)
>          goto out;
>      }
>
> -    xc = X86_CPU(object_new(object_class_get_name(OBJECT_CLASS(xcc))));
> +    xc = X86_CPU(object_new_with_class(OBJECT_CLASS(xcc)));
>      if (props) {
>          object_apply_props(OBJECT(xc), props, &err);
>          if (err) {
> @@ -5177,7 +5177,7 @@ static void x86_cpu_apic_create(X86CPU *cpu, Error **errp)
>      APICCommonState *apic;
>      ObjectClass *apic_class = OBJECT_CLASS(apic_get_class());
>
> -    cpu->apic_state = DEVICE(object_new(object_class_get_name(apic_class)));
> +    cpu->apic_state = DEVICE(object_new_with_class(apic_class));
>
>      object_property_add_child(OBJECT(cpu), "lapic",
>                                OBJECT(cpu->apic_state), &error_abort);
> diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
> index 7e92fb2..72cf48b 100644
> --- a/target/s390x/cpu_models.c
> +++ b/target/s390x/cpu_models.c
> @@ -440,7 +440,7 @@ static void create_cpu_model_list(ObjectClass *klass, void *opaque)
>      if (cpu_list_data->model) {
>          Object *obj;
>          S390CPU *sc;
> -        obj = object_new(object_class_get_name(klass));
> +        obj = object_new_with_class(klass);
>          sc = S390_CPU(obj);
>          if (sc->model) {
>              info->has_unavailable_features = true;
> @@ -501,7 +501,7 @@ static void cpu_model_from_info(S390CPUModel *model, const CpuModelInfo *info,
>          error_setg(errp, "The CPU definition '%s' requires KVM", info->name);
>          return;
>      }
> -    obj = object_new(object_class_get_name(oc));
> +    obj = object_new_with_class(oc);
>      cpu = S390_CPU(obj);
>
>      if (!cpu->model) {
> diff --git a/vl.c b/vl.c
> index cb993dd..6e406d4 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -3988,8 +3988,7 @@ int main(int argc, char **argv, char **envp)
>                        cleanup_add_fd, NULL, &error_fatal);
>  #endif
>
> -    current_machine = MACHINE(object_new(object_class_get_name(
> -                          OBJECT_CLASS(machine_class))));
> +    current_machine = MACHINE(object_new_with_class(OBJECT_CLASS(machine_class)));
>      if (machine_help_func(qemu_get_machine_opts(), current_machine)) {
>          exit(0);
>      }
> --
> 1.8.3.1
>
>
>


-- 
Marc-André Lureau

[PATCH 10/16] accel: pass object to accel_init_machine

[Paolo Bonzini]

We will have to set QOM properties before accel_init_machine, based on the
options provided to -accel.  Construct the object outside it so that it
will be possible to insert the iteration between object_new_with_class
and accel_init_machine.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 accel/accel.c          | 6 ++----
 include/sysemu/accel.h | 2 +-
 vl.c                   | 4 +++-
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/accel/accel.c b/accel/accel.c
index 74eda68..822e945 100644
--- a/accel/accel.c
+++ b/accel/accel.c
@@ -52,11 +52,9 @@ AccelClass *accel_find(const char *opt_name)
     return ac;
 }
 
-int accel_init_machine(AccelClass *acc, MachineState *ms)
+int accel_init_machine(AccelState *accel, MachineState *ms)
 {
-    ObjectClass *oc = OBJECT_CLASS(acc);
-    const char *cname = object_class_get_name(oc);
-    AccelState *accel = ACCEL(object_new(cname));
+    AccelClass *acc = ACCEL_GET_CLASS(accel);
     int ret;
     ms->accelerator = accel;
     *(acc->allowed) = true;
diff --git a/include/sysemu/accel.h b/include/sysemu/accel.h
index 90b6213..22cac0f 100644
--- a/include/sysemu/accel.h
+++ b/include/sysemu/accel.h
@@ -67,7 +67,7 @@ typedef struct AccelClass {
 extern unsigned long tcg_tb_size;
 
 AccelClass *accel_find(const char *opt_name);
-int accel_init_machine(AccelClass *acc, MachineState *ms);
+int accel_init_machine(AccelState *accel, MachineState *ms);
 
 /* Called just before os_setup_post (ie just before drop OS privs) */
 void accel_setup_post(MachineState *ms);
diff --git a/vl.c b/vl.c
index 6e406d4..c8ec906 100644
--- a/vl.c
+++ b/vl.c
@@ -2846,6 +2846,7 @@ static int do_configure_accelerator(void *opaque, QemuOpts *opts, Error **errp)
     bool *p_init_failed = opaque;
     const char *acc = qemu_opt_get(opts, "accel");
     AccelClass *ac = accel_find(acc);
+    AccelState *accel;
     int ret;
 
     if (!ac) {
@@ -2853,7 +2854,8 @@ static int do_configure_accelerator(void *opaque, QemuOpts *opts, Error **errp)
         error_report("invalid accelerator %s", acc);
         return 0;
     }
-    ret = accel_init_machine(ac, current_machine);
+    accel = ACCEL(object_new_with_class(OBJECT_CLASS(ac)));
+    ret = accel_init_machine(accel, current_machine);
     if (ret < 0) {
         *p_init_failed = true;
         error_report("failed to initialize %s: %s",
-- 
1.8.3.1

Re: [PATCH 10/16] accel: pass object to accel_init_machine

[Marc-André Lureau]

On Wed, Nov 13, 2019 at 6:45 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> We will have to set QOM properties before accel_init_machine, based on the
> options provided to -accel.  Construct the object outside it so that it
> will be possible to insert the iteration between object_new_with_class
> and accel_init_machine.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>


Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> ---
>  accel/accel.c          | 6 ++----
>  include/sysemu/accel.h | 2 +-
>  vl.c                   | 4 +++-
>  3 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/accel/accel.c b/accel/accel.c
> index 74eda68..822e945 100644
> --- a/accel/accel.c
> +++ b/accel/accel.c
> @@ -52,11 +52,9 @@ AccelClass *accel_find(const char *opt_name)
>      return ac;
>  }
>
> -int accel_init_machine(AccelClass *acc, MachineState *ms)
> +int accel_init_machine(AccelState *accel, MachineState *ms)
>  {
> -    ObjectClass *oc = OBJECT_CLASS(acc);
> -    const char *cname = object_class_get_name(oc);
> -    AccelState *accel = ACCEL(object_new(cname));
> +    AccelClass *acc = ACCEL_GET_CLASS(accel);
>      int ret;
>      ms->accelerator = accel;
>      *(acc->allowed) = true;
> diff --git a/include/sysemu/accel.h b/include/sysemu/accel.h
> index 90b6213..22cac0f 100644
> --- a/include/sysemu/accel.h
> +++ b/include/sysemu/accel.h
> @@ -67,7 +67,7 @@ typedef struct AccelClass {
>  extern unsigned long tcg_tb_size;
>
>  AccelClass *accel_find(const char *opt_name);
> -int accel_init_machine(AccelClass *acc, MachineState *ms);
> +int accel_init_machine(AccelState *accel, MachineState *ms);
>
>  /* Called just before os_setup_post (ie just before drop OS privs) */
>  void accel_setup_post(MachineState *ms);
> diff --git a/vl.c b/vl.c
> index 6e406d4..c8ec906 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2846,6 +2846,7 @@ static int do_configure_accelerator(void *opaque, QemuOpts *opts, Error **errp)
>      bool *p_init_failed = opaque;
>      const char *acc = qemu_opt_get(opts, "accel");
>      AccelClass *ac = accel_find(acc);
> +    AccelState *accel;
>      int ret;
>
>      if (!ac) {
> @@ -2853,7 +2854,8 @@ static int do_configure_accelerator(void *opaque, QemuOpts *opts, Error **errp)
>          error_report("invalid accelerator %s", acc);
>          return 0;
>      }
> -    ret = accel_init_machine(ac, current_machine);
> +    accel = ACCEL(object_new_with_class(OBJECT_CLASS(ac)));
> +    ret = accel_init_machine(accel, current_machine);
>      if (ret < 0) {
>          *p_init_failed = true;
>          error_report("failed to initialize %s: %s",
> --
> 1.8.3.1
>
>
>


-- 
Marc-André Lureau

[PATCH 11/16] tcg: convert "-accel threads" to a QOM property

[Paolo Bonzini]

Replace the ad-hoc qemu_tcg_configure with generic code invoking
QOM property getters and setters.  This will be extended in the
next patches, which will turn accelerator-related "-machine"
options into QOM properties as well.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 accel/tcg/tcg-all.c   | 111 +++++++++++++++++++++++++++++++++++++++++++++++++-
 cpus.c                |  72 --------------------------------
 include/sysemu/cpus.h |   2 -
 vl.c                  |  32 ++++++++-------
 4 files changed, 126 insertions(+), 91 deletions(-)

diff --git a/accel/tcg/tcg-all.c b/accel/tcg/tcg-all.c
index c59d5b0..7829f02 100644
--- a/accel/tcg/tcg-all.c
+++ b/accel/tcg/tcg-all.c
@@ -30,6 +30,21 @@
 #include "cpu.h"
 #include "sysemu/cpus.h"
 #include "qemu/main-loop.h"
+#include "tcg/tcg.h"
+#include "include/qapi/error.h"
+#include "include/qemu/error-report.h"
+#include "include/hw/boards.h"
+
+typedef struct TCGState {
+    AccelState parent_obj;
+
+    bool mttcg_enabled;
+} TCGState;
+
+#define TYPE_TCG_ACCEL ACCEL_CLASS_NAME("tcg")
+
+#define TCG_STATE(obj) \
+        OBJECT_CHECK(TCGState, (obj), TYPE_TCG_ACCEL)
 
 unsigned long tcg_tb_size;
 
@@ -58,27 +73,119 @@ static void tcg_handle_interrupt(CPUState *cpu, int mask)
     }
 }
 
+/*
+ * We default to false if we know other options have been enabled
+ * which are currently incompatible with MTTCG. Otherwise when each
+ * guest (target) has been updated to support:
+ *   - atomic instructions
+ *   - memory ordering primitives (barriers)
+ * they can set the appropriate CONFIG flags in ${target}-softmmu.mak
+ *
+ * Once a guest architecture has been converted to the new primitives
+ * there are two remaining limitations to check.
+ *
+ * - The guest can't be oversized (e.g. 64 bit guest on 32 bit host)
+ * - The host must have a stronger memory order than the guest
+ *
+ * It may be possible in future to support strong guests on weak hosts
+ * but that will require tagging all load/stores in a guest with their
+ * implicit memory order requirements which would likely slow things
+ * down a lot.
+ */
+
+static bool check_tcg_memory_orders_compatible(void)
+{
+#if defined(TCG_GUEST_DEFAULT_MO) && defined(TCG_TARGET_DEFAULT_MO)
+    return (TCG_GUEST_DEFAULT_MO & ~TCG_TARGET_DEFAULT_MO) == 0;
+#else
+    return false;
+#endif
+}
+
+static bool default_mttcg_enabled(void)
+{
+    if (use_icount || TCG_OVERSIZED_GUEST) {
+        return false;
+    } else {
+#ifdef TARGET_SUPPORTS_MTTCG
+        return check_tcg_memory_orders_compatible();
+#else
+        return false;
+#endif
+    }
+}
+
+static void tcg_accel_instance_init(Object *obj)
+{
+    TCGState *s = TCG_STATE(obj);
+
+    s->mttcg_enabled = default_mttcg_enabled();
+}
+
 static int tcg_init(MachineState *ms)
 {
+    TCGState *s = TCG_STATE(current_machine->accelerator);
+
     tcg_exec_init(tcg_tb_size * 1024 * 1024);
     cpu_interrupt_handler = tcg_handle_interrupt;
+    mttcg_enabled = s->mttcg_enabled;
     return 0;
 }
 
+static char *tcg_get_thread(Object *obj, Error **errp)
+{
+    TCGState *s = TCG_STATE(obj);
+
+    return g_strdup(s->mttcg_enabled ? "multi" : "single");
+}
+
+static void tcg_set_thread(Object *obj, const char *value, Error **errp)
+{
+    TCGState *s = TCG_STATE(obj);
+
+    if (strcmp(value, "multi") == 0) {
+        if (TCG_OVERSIZED_GUEST) {
+            error_setg(errp, "No MTTCG when guest word size > hosts");
+        } else if (use_icount) {
+            error_setg(errp, "No MTTCG when icount is enabled");
+        } else {
+#ifndef TARGET_SUPPORTS_MTTCG
+            warn_report("Guest not yet converted to MTTCG - "
+                        "you may get unexpected results");
+#endif
+            if (!check_tcg_memory_orders_compatible()) {
+                warn_report("Guest expects a stronger memory ordering "
+                            "than the host provides");
+                error_printf("This may cause strange/hard to debug errors\n");
+            }
+            s->mttcg_enabled = true;
+        }
+    } else if (strcmp(value, "single") == 0) {
+        s->mttcg_enabled = false;
+    } else {
+        error_setg(errp, "Invalid 'thread' setting %s", value);
+    }
+}
+
 static void tcg_accel_class_init(ObjectClass *oc, void *data)
 {
     AccelClass *ac = ACCEL_CLASS(oc);
     ac->name = "tcg";
     ac->init_machine = tcg_init;
     ac->allowed = &tcg_allowed;
-}
 
-#define TYPE_TCG_ACCEL ACCEL_CLASS_NAME("tcg")
+    object_class_property_add_str(oc, "thread",
+                                  tcg_get_thread,
+                                  tcg_set_thread,
+                                  NULL);
+}
 
 static const TypeInfo tcg_accel_type = {
     .name = TYPE_TCG_ACCEL,
     .parent = TYPE_ACCEL,
+    .instance_init = tcg_accel_instance_init,
     .class_init = tcg_accel_class_init,
+    .instance_size = sizeof(TCGState),
 };
 
 static void register_accel_types(void)
diff --git a/cpus.c b/cpus.c
index fabbeca..69d4f6a 100644
--- a/cpus.c
+++ b/cpus.c
@@ -165,78 +165,6 @@ typedef struct TimersState {
 static TimersState timers_state;
 bool mttcg_enabled;
 
-/*
- * We default to false if we know other options have been enabled
- * which are currently incompatible with MTTCG. Otherwise when each
- * guest (target) has been updated to support:
- *   - atomic instructions
- *   - memory ordering primitives (barriers)
- * they can set the appropriate CONFIG flags in ${target}-softmmu.mak
- *
- * Once a guest architecture has been converted to the new primitives
- * there are two remaining limitations to check.
- *
- * - The guest can't be oversized (e.g. 64 bit guest on 32 bit host)
- * - The host must have a stronger memory order than the guest
- *
- * It may be possible in future to support strong guests on weak hosts
- * but that will require tagging all load/stores in a guest with their
- * implicit memory order requirements which would likely slow things
- * down a lot.
- */
-
-static bool check_tcg_memory_orders_compatible(void)
-{
-#if defined(TCG_GUEST_DEFAULT_MO) && defined(TCG_TARGET_DEFAULT_MO)
-    return (TCG_GUEST_DEFAULT_MO & ~TCG_TARGET_DEFAULT_MO) == 0;
-#else
-    return false;
-#endif
-}
-
-static bool default_mttcg_enabled(void)
-{
-    if (use_icount || TCG_OVERSIZED_GUEST) {
-        return false;
-    } else {
-#ifdef TARGET_SUPPORTS_MTTCG
-        return check_tcg_memory_orders_compatible();
-#else
-        return false;
-#endif
-    }
-}
-
-void qemu_tcg_configure(QemuOpts *opts, Error **errp)
-{
-    const char *t = qemu_opt_get(opts, "thread");
-    if (t) {
-        if (strcmp(t, "multi") == 0) {
-            if (TCG_OVERSIZED_GUEST) {
-                error_setg(errp, "No MTTCG when guest word size > hosts");
-            } else if (use_icount) {
-                error_setg(errp, "No MTTCG when icount is enabled");
-            } else {
-#ifndef TARGET_SUPPORTS_MTTCG
-                warn_report("Guest not yet converted to MTTCG - "
-                            "you may get unexpected results");
-#endif
-                if (!check_tcg_memory_orders_compatible()) {
-                    warn_report("Guest expects a stronger memory ordering "
-                                "than the host provides");
-                    error_printf("This may cause strange/hard to debug errors\n");
-                }
-                mttcg_enabled = true;
-            }
-        } else if (strcmp(t, "single") == 0) {
-            mttcg_enabled = false;
-        } else {
-            error_setg(errp, "Invalid 'thread' setting %s", t);
-        }
-    } else {
-        mttcg_enabled = default_mttcg_enabled();
-    }
-}
 
 /* The current number of executed instructions is based on what we
  * originally budgeted minus the current state of the decrementing
diff --git a/include/sysemu/cpus.h b/include/sysemu/cpus.h
index 32c05f2..3c1da6a 100644
--- a/include/sysemu/cpus.h
+++ b/include/sysemu/cpus.h
@@ -40,6 +40,4 @@ extern int smp_threads;
 
 void list_cpus(const char *optarg);
 
-void qemu_tcg_configure(QemuOpts *opts, Error **errp);
-
 #endif
diff --git a/vl.c b/vl.c
index c8ec906..2ea94c7 100644
--- a/vl.c
+++ b/vl.c
@@ -294,17 +294,12 @@ static QemuOptsList qemu_accel_opts = {
     .implied_opt_name = "accel",
     .head = QTAILQ_HEAD_INITIALIZER(qemu_accel_opts.head),
     .desc = {
-        {
-            .name = "accel",
-            .type = QEMU_OPT_STRING,
-            .help = "Select the type of accelerator",
-        },
-        {
-            .name = "thread",
-            .type = QEMU_OPT_STRING,
-            .help = "Enable/disable multi-threaded TCG",
-        },
-        { /* end of list */ }
+        /*
+         * no elements => accept any
+         * sanity checking will happen later
+         * when setting accelerator properties
+         */
+        { }
     },
 };
 
@@ -2841,6 +2836,13 @@ static int do_configure_icount(void *opaque, QemuOpts *opts, Error **errp)
     return 0;
 }
 
+static int accelerator_set_property(void *opaque,
+                                const char *name, const char *value,
+                                Error **errp)
+{
+    return object_parse_property_opt(opaque, name, value, "accel", errp);
+}
+
 static int do_configure_accelerator(void *opaque, QemuOpts *opts, Error **errp)
 {
     bool *p_init_failed = opaque;
@@ -2855,6 +2857,10 @@ static int do_configure_accelerator(void *opaque, QemuOpts *opts, Error **errp)
         return 0;
     }
     accel = ACCEL(object_new_with_class(OBJECT_CLASS(ac)));
+    qemu_opt_foreach(opts, accelerator_set_property,
+                     accel,
+                     &error_fatal);
+
     ret = accel_init_machine(accel, current_machine);
     if (ret < 0) {
         *p_init_failed = true;
@@ -2862,10 +2868,6 @@ static int do_configure_accelerator(void *opaque, QemuOpts *opts, Error **errp)
                      acc, strerror(-ret));
         return 0;
     }
-
-    if (tcg_enabled()) {
-        qemu_tcg_configure(opts, &error_fatal);
-    }
     return 1;
 }
 
-- 
1.8.3.1

Re: [PATCH 11/16] tcg: convert "-accel threads" to a QOM property

[Marc-André Lureau]

On Wed, Nov 13, 2019 at 6:49 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> Replace the ad-hoc qemu_tcg_configure with generic code invoking
> QOM property getters and setters.  This will be extended in the
> next patches, which will turn accelerator-related "-machine"
> options into QOM properties as well.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> ---
>  accel/tcg/tcg-all.c   | 111 +++++++++++++++++++++++++++++++++++++++++++++++++-
>  cpus.c                |  72 --------------------------------
>  include/sysemu/cpus.h |   2 -
>  vl.c                  |  32 ++++++++-------
>  4 files changed, 126 insertions(+), 91 deletions(-)
>
> diff --git a/accel/tcg/tcg-all.c b/accel/tcg/tcg-all.c
> index c59d5b0..7829f02 100644
> --- a/accel/tcg/tcg-all.c
> +++ b/accel/tcg/tcg-all.c
> @@ -30,6 +30,21 @@
>  #include "cpu.h"
>  #include "sysemu/cpus.h"
>  #include "qemu/main-loop.h"
> +#include "tcg/tcg.h"
> +#include "include/qapi/error.h"
> +#include "include/qemu/error-report.h"
> +#include "include/hw/boards.h"
> +
> +typedef struct TCGState {
> +    AccelState parent_obj;
> +
> +    bool mttcg_enabled;
> +} TCGState;
> +
> +#define TYPE_TCG_ACCEL ACCEL_CLASS_NAME("tcg")
> +
> +#define TCG_STATE(obj) \
> +        OBJECT_CHECK(TCGState, (obj), TYPE_TCG_ACCEL)
>
>  unsigned long tcg_tb_size;
>
> @@ -58,27 +73,119 @@ static void tcg_handle_interrupt(CPUState *cpu, int mask)
>      }
>  }
>
> +/*
> + * We default to false if we know other options have been enabled
> + * which are currently incompatible with MTTCG. Otherwise when each
> + * guest (target) has been updated to support:
> + *   - atomic instructions
> + *   - memory ordering primitives (barriers)
> + * they can set the appropriate CONFIG flags in ${target}-softmmu.mak
> + *
> + * Once a guest architecture has been converted to the new primitives
> + * there are two remaining limitations to check.
> + *
> + * - The guest can't be oversized (e.g. 64 bit guest on 32 bit host)
> + * - The host must have a stronger memory order than the guest
> + *
> + * It may be possible in future to support strong guests on weak hosts
> + * but that will require tagging all load/stores in a guest with their
> + * implicit memory order requirements which would likely slow things
> + * down a lot.
> + */
> +
> +static bool check_tcg_memory_orders_compatible(void)
> +{
> +#if defined(TCG_GUEST_DEFAULT_MO) && defined(TCG_TARGET_DEFAULT_MO)
> +    return (TCG_GUEST_DEFAULT_MO & ~TCG_TARGET_DEFAULT_MO) == 0;
> +#else
> +    return false;
> +#endif
> +}
> +
> +static bool default_mttcg_enabled(void)
> +{
> +    if (use_icount || TCG_OVERSIZED_GUEST) {
> +        return false;
> +    } else {
> +#ifdef TARGET_SUPPORTS_MTTCG
> +        return check_tcg_memory_orders_compatible();
> +#else
> +        return false;
> +#endif
> +    }
> +}
> +
> +static void tcg_accel_instance_init(Object *obj)
> +{
> +    TCGState *s = TCG_STATE(obj);
> +
> +    s->mttcg_enabled = default_mttcg_enabled();
> +}
> +
>  static int tcg_init(MachineState *ms)
>  {
> +    TCGState *s = TCG_STATE(current_machine->accelerator);
> +
>      tcg_exec_init(tcg_tb_size * 1024 * 1024);
>      cpu_interrupt_handler = tcg_handle_interrupt;
> +    mttcg_enabled = s->mttcg_enabled;
>      return 0;
>  }
>
> +static char *tcg_get_thread(Object *obj, Error **errp)
> +{
> +    TCGState *s = TCG_STATE(obj);
> +
> +    return g_strdup(s->mttcg_enabled ? "multi" : "single");
> +}
> +
> +static void tcg_set_thread(Object *obj, const char *value, Error **errp)
> +{
> +    TCGState *s = TCG_STATE(obj);
> +
> +    if (strcmp(value, "multi") == 0) {
> +        if (TCG_OVERSIZED_GUEST) {
> +            error_setg(errp, "No MTTCG when guest word size > hosts");
> +        } else if (use_icount) {
> +            error_setg(errp, "No MTTCG when icount is enabled");
> +        } else {
> +#ifndef TARGET_SUPPORTS_MTTCG
> +            warn_report("Guest not yet converted to MTTCG - "
> +                        "you may get unexpected results");
> +#endif
> +            if (!check_tcg_memory_orders_compatible()) {
> +                warn_report("Guest expects a stronger memory ordering "
> +                            "than the host provides");
> +                error_printf("This may cause strange/hard to debug errors\n");
> +            }
> +            s->mttcg_enabled = true;
> +        }
> +    } else if (strcmp(value, "single") == 0) {
> +        s->mttcg_enabled = false;
> +    } else {
> +        error_setg(errp, "Invalid 'thread' setting %s", value);
> +    }
> +}
> +
>  static void tcg_accel_class_init(ObjectClass *oc, void *data)
>  {
>      AccelClass *ac = ACCEL_CLASS(oc);
>      ac->name = "tcg";
>      ac->init_machine = tcg_init;
>      ac->allowed = &tcg_allowed;
> -}
>
> -#define TYPE_TCG_ACCEL ACCEL_CLASS_NAME("tcg")
> +    object_class_property_add_str(oc, "thread",
> +                                  tcg_get_thread,
> +                                  tcg_set_thread,
> +                                  NULL);
> +}
>
>  static const TypeInfo tcg_accel_type = {
>      .name = TYPE_TCG_ACCEL,
>      .parent = TYPE_ACCEL,
> +    .instance_init = tcg_accel_instance_init,
>      .class_init = tcg_accel_class_init,
> +    .instance_size = sizeof(TCGState),
>  };
>
>  static void register_accel_types(void)
> diff --git a/cpus.c b/cpus.c
> index fabbeca..69d4f6a 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -165,78 +165,6 @@ typedef struct TimersState {
>  static TimersState timers_state;
>  bool mttcg_enabled;
>
> -/*
> - * We default to false if we know other options have been enabled
> - * which are currently incompatible with MTTCG. Otherwise when each
> - * guest (target) has been updated to support:
> - *   - atomic instructions
> - *   - memory ordering primitives (barriers)
> - * they can set the appropriate CONFIG flags in ${target}-softmmu.mak
> - *
> - * Once a guest architecture has been converted to the new primitives
> - * there are two remaining limitations to check.
> - *
> - * - The guest can't be oversized (e.g. 64 bit guest on 32 bit host)
> - * - The host must have a stronger memory order than the guest
> - *
> - * It may be possible in future to support strong guests on weak hosts
> - * but that will require tagging all load/stores in a guest with their
> - * implicit memory order requirements which would likely slow things
> - * down a lot.
> - */
> -
> -static bool check_tcg_memory_orders_compatible(void)
> -{
> -#if defined(TCG_GUEST_DEFAULT_MO) && defined(TCG_TARGET_DEFAULT_MO)
> -    return (TCG_GUEST_DEFAULT_MO & ~TCG_TARGET_DEFAULT_MO) == 0;
> -#else
> -    return false;
> -#endif
> -}
> -
> -static bool default_mttcg_enabled(void)
> -{
> -    if (use_icount || TCG_OVERSIZED_GUEST) {
> -        return false;
> -    } else {
> -#ifdef TARGET_SUPPORTS_MTTCG
> -        return check_tcg_memory_orders_compatible();
> -#else
> -        return false;
> -#endif
> -    }
> -}
> -
> -void qemu_tcg_configure(QemuOpts *opts, Error **errp)
> -{
> -    const char *t = qemu_opt_get(opts, "thread");
> -    if (t) {
> -        if (strcmp(t, "multi") == 0) {
> -            if (TCG_OVERSIZED_GUEST) {
> -                error_setg(errp, "No MTTCG when guest word size > hosts");
> -            } else if (use_icount) {
> -                error_setg(errp, "No MTTCG when icount is enabled");
> -            } else {
> -#ifndef TARGET_SUPPORTS_MTTCG
> -                warn_report("Guest not yet converted to MTTCG - "
> -                            "you may get unexpected results");
> -#endif
> -                if (!check_tcg_memory_orders_compatible()) {
> -                    warn_report("Guest expects a stronger memory ordering "
> -                                "than the host provides");
> -                    error_printf("This may cause strange/hard to debug errors\n");
> -                }
> -                mttcg_enabled = true;
> -            }
> -        } else if (strcmp(t, "single") == 0) {
> -            mttcg_enabled = false;
> -        } else {
> -            error_setg(errp, "Invalid 'thread' setting %s", t);
> -        }
> -    } else {
> -        mttcg_enabled = default_mttcg_enabled();
> -    }
> -}
>
>  /* The current number of executed instructions is based on what we
>   * originally budgeted minus the current state of the decrementing
> diff --git a/include/sysemu/cpus.h b/include/sysemu/cpus.h
> index 32c05f2..3c1da6a 100644
> --- a/include/sysemu/cpus.h
> +++ b/include/sysemu/cpus.h
> @@ -40,6 +40,4 @@ extern int smp_threads;
>
>  void list_cpus(const char *optarg);
>
> -void qemu_tcg_configure(QemuOpts *opts, Error **errp);
> -
>  #endif
> diff --git a/vl.c b/vl.c
> index c8ec906..2ea94c7 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -294,17 +294,12 @@ static QemuOptsList qemu_accel_opts = {
>      .implied_opt_name = "accel",
>      .head = QTAILQ_HEAD_INITIALIZER(qemu_accel_opts.head),
>      .desc = {
> -        {
> -            .name = "accel",
> -            .type = QEMU_OPT_STRING,
> -            .help = "Select the type of accelerator",
> -        },
> -        {
> -            .name = "thread",
> -            .type = QEMU_OPT_STRING,
> -            .help = "Enable/disable multi-threaded TCG",
> -        },
> -        { /* end of list */ }
> +        /*
> +         * no elements => accept any
> +         * sanity checking will happen later
> +         * when setting accelerator properties
> +         */
> +        { }
>      },
>  };
>
> @@ -2841,6 +2836,13 @@ static int do_configure_icount(void *opaque, QemuOpts *opts, Error **errp)
>      return 0;
>  }
>
> +static int accelerator_set_property(void *opaque,
> +                                const char *name, const char *value,
> +                                Error **errp)
> +{
> +    return object_parse_property_opt(opaque, name, value, "accel", errp);
> +}
> +
>  static int do_configure_accelerator(void *opaque, QemuOpts *opts, Error **errp)
>  {
>      bool *p_init_failed = opaque;
> @@ -2855,6 +2857,10 @@ static int do_configure_accelerator(void *opaque, QemuOpts *opts, Error **errp)
>          return 0;
>      }
>      accel = ACCEL(object_new_with_class(OBJECT_CLASS(ac)));
> +    qemu_opt_foreach(opts, accelerator_set_property,
> +                     accel,
> +                     &error_fatal);
> +
>      ret = accel_init_machine(accel, current_machine);
>      if (ret < 0) {
>          *p_init_failed = true;
> @@ -2862,10 +2868,6 @@ static int do_configure_accelerator(void *opaque, QemuOpts *opts, Error **errp)
>                       acc, strerror(-ret));
>          return 0;
>      }
> -
> -    if (tcg_enabled()) {
> -        qemu_tcg_configure(opts, &error_fatal);
> -    }
>      return 1;
>  }
>
> --
> 1.8.3.1
>
>
>


-- 
Marc-André Lureau

[PATCH 12/16] tcg: add "-accel tcg,tb-size" and deprecate "-tb-size"

[Paolo Bonzini]

-tb-size fits nicely in the new framework for accelerator-specific options.  It
is a very niche option, so insta-deprecate the old version.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 accel/tcg/tcg-all.c    | 40 +++++++++++++++++++++++++++++++++++++---
 include/sysemu/accel.h |  2 --
 qemu-deprecated.texi   |  6 ++++++
 qemu-options.hx        |  6 +++++-
 vl.c                   |  8 ++++----
 5 files changed, 52 insertions(+), 10 deletions(-)

diff --git a/accel/tcg/tcg-all.c b/accel/tcg/tcg-all.c
index 7829f02..1dc384c 100644
--- a/accel/tcg/tcg-all.c
+++ b/accel/tcg/tcg-all.c
@@ -34,11 +34,13 @@
 #include "include/qapi/error.h"
 #include "include/qemu/error-report.h"
 #include "include/hw/boards.h"
+#include "qapi/qapi-builtin-visit.h"
 
 typedef struct TCGState {
     AccelState parent_obj;
 
     bool mttcg_enabled;
+    unsigned long tb_size;
 } TCGState;
 
 #define TYPE_TCG_ACCEL ACCEL_CLASS_NAME("tcg")
@@ -46,8 +48,6 @@ typedef struct TCGState {
 #define TCG_STATE(obj) \
         OBJECT_CHECK(TCGState, (obj), TYPE_TCG_ACCEL)
 
-unsigned long tcg_tb_size;
-
 /* mask must never be zero, except for A20 change call */
 static void tcg_handle_interrupt(CPUState *cpu, int mask)
 {
@@ -126,7 +126,7 @@ static int tcg_init(MachineState *ms)
 {
     TCGState *s = TCG_STATE(current_machine->accelerator);
 
-    tcg_exec_init(tcg_tb_size * 1024 * 1024);
+    tcg_exec_init(s->tb_size * 1024 * 1024);
     cpu_interrupt_handler = tcg_handle_interrupt;
     mttcg_enabled = s->mttcg_enabled;
     return 0;
@@ -167,6 +167,33 @@ static void tcg_set_thread(Object *obj, const char *value, Error **errp)
     }
 }
 
+static void tcg_get_tb_size(Object *obj, Visitor *v,
+                            const char *name, void *opaque,
+                            Error **errp)
+{
+    TCGState *s = TCG_STATE(obj);
+    uint32_t value = s->tb_size;
+
+    visit_type_uint32(v, name, &value, errp);
+}
+
+static void tcg_set_tb_size(Object *obj, Visitor *v,
+                            const char *name, void *opaque,
+                            Error **errp)
+{
+    TCGState *s = TCG_STATE(obj);
+    Error *error = NULL;
+    uint32_t value;
+
+    visit_type_uint32(v, name, &value, &error);
+    if (error) {
+        error_propagate(errp, error);
+        return;
+    }
+
+    s->tb_size = value;
+}
+
 static void tcg_accel_class_init(ObjectClass *oc, void *data)
 {
     AccelClass *ac = ACCEL_CLASS(oc);
@@ -178,6 +205,13 @@ static void tcg_accel_class_init(ObjectClass *oc, void *data)
                                   tcg_get_thread,
                                   tcg_set_thread,
                                   NULL);
+
+    object_class_property_add(oc, "tb-size", "int",
+        tcg_get_tb_size, tcg_set_tb_size,
+        NULL, NULL, &error_abort);
+    object_class_property_set_description(oc, "tb-size",
+        "TCG translation block cache size", &error_abort);
+
 }
 
 static const TypeInfo tcg_accel_type = {
diff --git a/include/sysemu/accel.h b/include/sysemu/accel.h
index 22cac0f..d4c1429 100644
--- a/include/sysemu/accel.h
+++ b/include/sysemu/accel.h
@@ -64,8 +64,6 @@ typedef struct AccelClass {
 #define ACCEL_GET_CLASS(obj) \
     OBJECT_GET_CLASS(AccelClass, (obj), TYPE_ACCEL)
 
-extern unsigned long tcg_tb_size;
-
 AccelClass *accel_find(const char *opt_name);
 int accel_init_machine(AccelState *accel, MachineState *ms);
 
diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
index 7239e09..6ca5f80 100644
--- a/qemu-deprecated.texi
+++ b/qemu-deprecated.texi
@@ -147,6 +147,12 @@ QEMU 4.1 has three options, please migrate to one of these three:
       to do is specify the kernel they want to boot with the -kernel option
  3. ``-bios <file>`` - Tells QEMU to load the specified file as the firmwrae.
 
+@subsection -tb-size option (since 5.0)
+
+QEMU 5.0 introduced an alternative syntax to specify the size of the translation
+block cache, @option{-accel tcg,tb-size=}.  The new syntax deprecates the
+previously available @option{-tb-size} option.
+
 @section QEMU Machine Protocol (QMP) commands
 
 @subsection query-block result field dirty-bitmaps[i].status (since 4.0)
diff --git a/qemu-options.hx b/qemu-options.hx
index b95bf9f..3931f90 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -120,6 +120,7 @@ ETEXI
 DEF("accel", HAS_ARG, QEMU_OPTION_accel,
     "-accel [accel=]accelerator[,thread=single|multi]\n"
     "                select accelerator (kvm, xen, hax, hvf, whpx or tcg; use 'help' for a list)\n"
+    "                tb-size=n (TCG translation block cache size)\n"
     "                thread=single|multi (enable multi-threaded TCG)\n", QEMU_ARCH_ALL)
 STEXI
 @item -accel @var{name}[,prop=@var{value}[,...]]
@@ -129,6 +130,8 @@ kvm, xen, hax, hvf, whpx or tcg can be available. By default, tcg is used. If th
 more than one accelerator specified, the next one is used if the previous one
 fails to initialize.
 @table @option
+@item tb-size=@var{n}
+Controls the size (in MiB) of the TCG translation block cache.
 @item thread=single|multi
 Controls number of TCG threads. When the TCG is multi-threaded there will be one
 thread per vCPU therefor taking advantage of additional host cores. The default
@@ -3995,7 +3998,8 @@ DEF("tb-size", HAS_ARG, QEMU_OPTION_tb_size, \
 STEXI
 @item -tb-size @var{n}
 @findex -tb-size
-Set TB size.
+Set TCG translation block cache size.  Deprecated, use @samp{-accel tcg,tb-size=@var{n}}
+instead.
 ETEXI
 
 DEF("incoming", HAS_ARG, QEMU_OPTION_incoming, \
diff --git a/vl.c b/vl.c
index 2ea94c7..06c6ad9 100644
--- a/vl.c
+++ b/vl.c
@@ -2857,6 +2857,7 @@ static int do_configure_accelerator(void *opaque, QemuOpts *opts, Error **errp)
         return 0;
     }
     accel = ACCEL(object_new_with_class(OBJECT_CLASS(ac)));
+    object_apply_compat_props(OBJECT(accel));
     qemu_opt_foreach(opts, accelerator_set_property,
                      accel,
                      &error_fatal);
@@ -2868,6 +2869,7 @@ static int do_configure_accelerator(void *opaque, QemuOpts *opts, Error **errp)
                      acc, strerror(-ret));
         return 0;
     }
+
     return 1;
 }
 
@@ -3747,10 +3749,8 @@ int main(int argc, char **argv, char **envp)
                 error_report("TCG is disabled");
                 exit(1);
 #endif
-                if (qemu_strtoul(optarg, NULL, 0, &tcg_tb_size) < 0) {
-                    error_report("Invalid argument to -tb-size");
-                    exit(1);
-                }
+                warn_report("The -tb-size option is deprecated, use -accel tcg,tb-size instead");
+                object_register_sugar_prop(ACCEL_CLASS_NAME("tcg"), "tb-size", optarg);
                 break;
             case QEMU_OPTION_icount:
                 icount_opts = qemu_opts_parse_noisily(qemu_find_opts("icount"),
-- 
1.8.3.1

Re: [PATCH 12/16] tcg: add "-accel tcg,tb-size" and deprecate "-tb-size"

[Thomas Huth]

On 13/11/2019 15.39, Paolo Bonzini wrote:
> -tb-size fits nicely in the new framework for accelerator-specific options.  It
> is a very niche option, so insta-deprecate the old version.

Good idea!

> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  accel/tcg/tcg-all.c    | 40 +++++++++++++++++++++++++++++++++++++---
>  include/sysemu/accel.h |  2 --
>  qemu-deprecated.texi   |  6 ++++++
>  qemu-options.hx        |  6 +++++-
>  vl.c                   |  8 ++++----
>  5 files changed, 52 insertions(+), 10 deletions(-)
> 
> diff --git a/accel/tcg/tcg-all.c b/accel/tcg/tcg-all.c
> index 7829f02..1dc384c 100644
> --- a/accel/tcg/tcg-all.c
> +++ b/accel/tcg/tcg-all.c
> @@ -34,11 +34,13 @@
>  #include "include/qapi/error.h"
>  #include "include/qemu/error-report.h"
>  #include "include/hw/boards.h"
> +#include "qapi/qapi-builtin-visit.h"
>  
>  typedef struct TCGState {
>      AccelState parent_obj;
>  
>      bool mttcg_enabled;
> +    unsigned long tb_size;

So this tb_size is "long" ...

>  } TCGState;
>  
>  #define TYPE_TCG_ACCEL ACCEL_CLASS_NAME("tcg")
> @@ -46,8 +48,6 @@ typedef struct TCGState {
>  #define TCG_STATE(obj) \
>          OBJECT_CHECK(TCGState, (obj), TYPE_TCG_ACCEL)
>  
> -unsigned long tcg_tb_size;
> -
>  /* mask must never be zero, except for A20 change call */
>  static void tcg_handle_interrupt(CPUState *cpu, int mask)
>  {
> @@ -126,7 +126,7 @@ static int tcg_init(MachineState *ms)
>  {
>      TCGState *s = TCG_STATE(current_machine->accelerator);
>  
> -    tcg_exec_init(tcg_tb_size * 1024 * 1024);
> +    tcg_exec_init(s->tb_size * 1024 * 1024);

... which is likely ok for this calculation here ...

>      cpu_interrupt_handler = tcg_handle_interrupt;
>      mttcg_enabled = s->mttcg_enabled;
>      return 0;
> @@ -167,6 +167,33 @@ static void tcg_set_thread(Object *obj, const char *value, Error **errp)
>      }
>  }
>  
> +static void tcg_get_tb_size(Object *obj, Visitor *v,
> +                            const char *name, void *opaque,
> +                            Error **errp)
> +{
> +    TCGState *s = TCG_STATE(obj);
> +    uint32_t value = s->tb_size;
> +
> +    visit_type_uint32(v, name, &value, errp);

... but this is only using 32-bit values.

Should be fine, I guess, but it's a little bit confusing when looking at
the code with a quick glance only.

Maybe turn tb_size into a "uint32_t", too, and then use "* MiB" in above
calculation instead of "* 1024 * 1024" ?

> +}
[...]
> diff --git a/qemu-options.hx b/qemu-options.hx
> index b95bf9f..3931f90 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -120,6 +120,7 @@ ETEXI
>  DEF("accel", HAS_ARG, QEMU_OPTION_accel,
>      "-accel [accel=]accelerator[,thread=single|multi]\n"

Maybe add the "[,prop[=value]" change from the next patch here already?

Or should we even split it up into separate lines, like we do it for
-netdev for example already? i.e.:

        -accel tcg,[,thread=single|multi][,tb-size=n]\n
           ...
        -accel xen,[igd-passthrough=...]\n

?

>      "                select accelerator (kvm, xen, hax, hvf, whpx or tcg; use 'help' for a list)\n"
> +    "                tb-size=n (TCG translation block cache size)\n"
>      "                thread=single|multi (enable multi-threaded TCG)\n", QEMU_ARCH_ALL)
>  STEXI
>  @item -accel @var{name}[,prop=@var{value}[,...]]
> @@ -129,6 +130,8 @@ kvm, xen, hax, hvf, whpx or tcg can be available. By default, tcg is used. If th
>  more than one accelerator specified, the next one is used if the previous one
>  fails to initialize.
>  @table @option
> +@item tb-size=@var{n}
> +Controls the size (in MiB) of the TCG translation block cache.
>  @item thread=single|multi
>  Controls number of TCG threads. When the TCG is multi-threaded there will be one
>  thread per vCPU therefor taking advantage of additional host cores. The default
> @@ -3995,7 +3998,8 @@ DEF("tb-size", HAS_ARG, QEMU_OPTION_tb_size, \
>  STEXI
>  @item -tb-size @var{n}
>  @findex -tb-size
> -Set TB size.
> +Set TCG translation block cache size.  Deprecated, use @samp{-accel tcg,tb-size=@var{n}}
> +instead.
>  ETEXI
>  
>  DEF("incoming", HAS_ARG, QEMU_OPTION_incoming, \
> diff --git a/vl.c b/vl.c
> index 2ea94c7..06c6ad9 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2857,6 +2857,7 @@ static int do_configure_accelerator(void *opaque, QemuOpts *opts, Error **errp)
>          return 0;
>      }
>      accel = ACCEL(object_new_with_class(OBJECT_CLASS(ac)));
> +    object_apply_compat_props(OBJECT(accel));
>      qemu_opt_foreach(opts, accelerator_set_property,
>                       accel,
>                       &error_fatal);
> @@ -2868,6 +2869,7 @@ static int do_configure_accelerator(void *opaque, QemuOpts *opts, Error **errp)
>                       acc, strerror(-ret));
>          return 0;
>      }
> +

Unnecessary white space change.

>      return 1;
>  }
>  
> @@ -3747,10 +3749,8 @@ int main(int argc, char **argv, char **envp)
>                  error_report("TCG is disabled");
>                  exit(1);
>  #endif
> -                if (qemu_strtoul(optarg, NULL, 0, &tcg_tb_size) < 0) {
> -                    error_report("Invalid argument to -tb-size");
> -                    exit(1);
> -                }
> +                warn_report("The -tb-size option is deprecated, use -accel tcg,tb-size instead");
> +                object_register_sugar_prop(ACCEL_CLASS_NAME("tcg"), "tb-size", optarg);
>                  break;
>              case QEMU_OPTION_icount:
>                  icount_opts = qemu_opts_parse_noisily(qemu_find_opts("icount"),
> 

 Thomas

[PATCH 13/16] xen: convert "-machine igd-passthru" to an accelerator property

[Paolo Bonzini]

The first machine property to fall is Xen's Intel integrated graphics
passthrough.  The "-machine igd-passthru" option does not set anymore
a property on the machine object, but desugars to a GlobalProperty on
accelerator objects.

The setter is very simple, since the value ends up in a
global variable, so this patch also provides an example before the more
complicated cases that follow it.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/core/machine.c   | 20 --------------------
 hw/xen/xen-common.c | 16 ++++++++++++++++
 include/hw/boards.h |  1 -
 qemu-options.hx     |  9 +++++----
 vl.c                | 14 ++++----------
 5 files changed, 25 insertions(+), 35 deletions(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 45ddfb6..d7a0356 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -412,20 +412,6 @@ static void machine_set_graphics(Object *obj, bool value, Error **errp)
     ms->enable_graphics = value;
 }
 
-static bool machine_get_igd_gfx_passthru(Object *obj, Error **errp)
-{
-    MachineState *ms = MACHINE(obj);
-
-    return ms->igd_gfx_passthru;
-}
-
-static void machine_set_igd_gfx_passthru(Object *obj, bool value, Error **errp)
-{
-    MachineState *ms = MACHINE(obj);
-
-    ms->igd_gfx_passthru = value;
-}
-
 static char *machine_get_firmware(Object *obj, Error **errp)
 {
     MachineState *ms = MACHINE(obj);
@@ -862,12 +848,6 @@ static void machine_class_init(ObjectClass *oc, void *data)
     object_class_property_set_description(oc, "graphics",
         "Set on/off to enable/disable graphics emulation", &error_abort);
 
-    object_class_property_add_bool(oc, "igd-passthru",
-        machine_get_igd_gfx_passthru, machine_set_igd_gfx_passthru,
-        &error_abort);
-    object_class_property_set_description(oc, "igd-passthru",
-        "Set on/off to enable/disable igd passthrou", &error_abort);
-
     object_class_property_add_str(oc, "firmware",
         machine_get_firmware, machine_set_firmware,
         &error_abort);
diff --git a/hw/xen/xen-common.c b/hw/xen/xen-common.c
index 5284b0d..6cba30c 100644
--- a/hw/xen/xen-common.c
+++ b/hw/xen/xen-common.c
@@ -124,6 +124,16 @@ static void xen_change_state_handler(void *opaque, int running,
     }
 }
 
+static bool xen_get_igd_gfx_passthru(Object *obj, Error **errp)
+{
+    return has_igd_gfx_passthru;
+}
+
+static void xen_set_igd_gfx_passthru(Object *obj, bool value, Error **errp)
+{
+    has_igd_gfx_passthru = value;
+}
+
 static void xen_setup_post(MachineState *ms, AccelState *accel)
 {
     int rc;
@@ -177,6 +187,12 @@ static void xen_accel_class_init(ObjectClass *oc, void *data)
     ac->compat_props = g_ptr_array_new();
 
     compat_props_add(ac->compat_props, compat, G_N_ELEMENTS(compat));
+
+    object_class_property_add_bool(oc, "igd-passthru",
+        xen_get_igd_gfx_passthru, xen_set_igd_gfx_passthru,
+        &error_abort);
+    object_class_property_set_description(oc, "igd-passthru",
+        "Set on/off to enable/disable igd passthrou", &error_abort);
 }
 
 #define TYPE_XEN_ACCEL ACCEL_CLASS_NAME("xen")
diff --git a/include/hw/boards.h b/include/hw/boards.h
index 36fcbda..cdcf481 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -287,7 +287,6 @@ struct MachineState {
     bool mem_merge;
     bool usb;
     bool usb_disabled;
-    bool igd_gfx_passthru;
     char *firmware;
     bool iommu;
     bool suppress_vmdesc;
diff --git a/qemu-options.hx b/qemu-options.hx
index 3931f90..5b43a83 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -37,7 +37,6 @@ DEF("machine", HAS_ARG, QEMU_OPTION_machine, \
     "                kvm_shadow_mem=size of KVM shadow MMU in bytes\n"
     "                dump-guest-core=on|off include guest memory in a core dump (default=on)\n"
     "                mem-merge=on|off controls memory merge support (default: on)\n"
-    "                igd-passthru=on|off controls IGD GFX passthrough support (default=off)\n"
     "                aes-key-wrap=on|off controls support for AES key wrapping (default=on)\n"
     "                dea-key-wrap=on|off controls support for DEA key wrapping (default=on)\n"
     "                suppress-vmdesc=on|off disables self-describing migration (default=off)\n"
@@ -71,8 +70,6 @@ more than one accelerator specified, the next one is used if the previous one
 fails to initialize.
 @item kernel_irqchip=on|off
 Controls in-kernel irqchip support for the chosen accelerator when available.
-@item gfx_passthru=on|off
-Enables IGD GFX passthrough support for the chosen machine when available.
 @item vmport=on|off|auto
 Enables emulation of VMWare IO port, for vmmouse etc. auto says to select the
 value based on accel. For accel=xen the default is off otherwise the default
@@ -118,8 +115,9 @@ Select CPU model (@code{-cpu help} for list and additional feature selection)
 ETEXI
 
 DEF("accel", HAS_ARG, QEMU_OPTION_accel,
-    "-accel [accel=]accelerator[,thread=single|multi]\n"
+    "-accel [accel=]accelerator[,prop[=value][,...]]\n"
     "                select accelerator (kvm, xen, hax, hvf, whpx or tcg; use 'help' for a list)\n"
+    "                igd-passthru=on|off (enable Xen integrated Intel graphics passthrough, default=off)\n"
     "                tb-size=n (TCG translation block cache size)\n"
     "                thread=single|multi (enable multi-threaded TCG)\n", QEMU_ARCH_ALL)
 STEXI
@@ -130,6 +128,9 @@ kvm, xen, hax, hvf, whpx or tcg can be available. By default, tcg is used. If th
 more than one accelerator specified, the next one is used if the previous one
 fails to initialize.
 @table @option
+@item igd-passthru=on|off
+When Xen is in use, this option controls whether Intel integrated graphics
+devices can be passed through to the guest (default=off)
 @item tb-size=@var{n}
 Controls the size (in MiB) of the TCG translation block cache.
 @item thread=single|multi
diff --git a/vl.c b/vl.c
index 06c6ad9..7d8fed1 100644
--- a/vl.c
+++ b/vl.c
@@ -1256,13 +1256,6 @@ static void configure_msg(QemuOpts *opts)
 }
 
 
-/* Now we still need this for compatibility with XEN. */
-bool has_igd_gfx_passthru;
-static void igd_gfx_passthru(void)
-{
-    has_igd_gfx_passthru = current_machine->igd_gfx_passthru;
-}
-
 /***********************************************************/
 /* USB devices */
 
@@ -2645,6 +2638,10 @@ static int machine_set_property(void *opaque,
     if (g_str_equal(qom_name, "accel")) {
         return 0;
     }
+    if (g_str_equal(qom_name, "igd-passthru")) {
+        object_register_sugar_prop(ACCEL_CLASS_NAME("xen"), qom_name, value);
+        return 0;
+    }
 
     r = object_parse_property_opt(opaque, name, value, "type", errp);
     g_free(qom_name);
@@ -4449,9 +4446,6 @@ int main(int argc, char **argv, char **envp)
             exit(1);
     }
 
-    /* Check if IGD GFX passthrough. */
-    igd_gfx_passthru();
-
     /* init generic devices */
     rom_set_order_override(FW_CFG_ORDER_OVERRIDE_DEVICE);
     qemu_opts_foreach(qemu_find_opts("device"),
-- 
1.8.3.1

Re: [PATCH 13/16] xen: convert "-machine igd-passthru" to an accelerator property

[Paul Durrant]

On Wed, 13 Nov 2019 at 14:53, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> The first machine property to fall is Xen's Intel integrated graphics
> passthrough.  The "-machine igd-passthru" option does not set anymore
> a property on the machine object, but desugars to a GlobalProperty on
> accelerator objects.
>
> The setter is very simple, since the value ends up in a
> global variable, so this patch also provides an example before the more
> complicated cases that follow it.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Did something go wrong with get_maintainers here? The Xen maintainers
ought to have been cc-ed. The Xen toolstack will require consequent
modification.

Cc-ing (rest of) Xen maintainers and xen-devel manually.

  Paul

> ---
>  hw/core/machine.c   | 20 --------------------
>  hw/xen/xen-common.c | 16 ++++++++++++++++
>  include/hw/boards.h |  1 -
>  qemu-options.hx     |  9 +++++----
>  vl.c                | 14 ++++----------
>  5 files changed, 25 insertions(+), 35 deletions(-)
>
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 45ddfb6..d7a0356 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -412,20 +412,6 @@ static void machine_set_graphics(Object *obj, bool value, Error **errp)
>      ms->enable_graphics = value;
>  }
>
> -static bool machine_get_igd_gfx_passthru(Object *obj, Error **errp)
> -{
> -    MachineState *ms = MACHINE(obj);
> -
> -    return ms->igd_gfx_passthru;
> -}
> -
> -static void machine_set_igd_gfx_passthru(Object *obj, bool value, Error **errp)
> -{
> -    MachineState *ms = MACHINE(obj);
> -
> -    ms->igd_gfx_passthru = value;
> -}
> -
>  static char *machine_get_firmware(Object *obj, Error **errp)
>  {
>      MachineState *ms = MACHINE(obj);
> @@ -862,12 +848,6 @@ static void machine_class_init(ObjectClass *oc, void *data)
>      object_class_property_set_description(oc, "graphics",
>          "Set on/off to enable/disable graphics emulation", &error_abort);
>
> -    object_class_property_add_bool(oc, "igd-passthru",
> -        machine_get_igd_gfx_passthru, machine_set_igd_gfx_passthru,
> -        &error_abort);
> -    object_class_property_set_description(oc, "igd-passthru",
> -        "Set on/off to enable/disable igd passthrou", &error_abort);
> -
>      object_class_property_add_str(oc, "firmware",
>          machine_get_firmware, machine_set_firmware,
>          &error_abort);
> diff --git a/hw/xen/xen-common.c b/hw/xen/xen-common.c
> index 5284b0d..6cba30c 100644
> --- a/hw/xen/xen-common.c
> +++ b/hw/xen/xen-common.c
> @@ -124,6 +124,16 @@ static void xen_change_state_handler(void *opaque, int running,
>      }
>  }
>
> +static bool xen_get_igd_gfx_passthru(Object *obj, Error **errp)
> +{
> +    return has_igd_gfx_passthru;
> +}
> +
> +static void xen_set_igd_gfx_passthru(Object *obj, bool value, Error **errp)
> +{
> +    has_igd_gfx_passthru = value;
> +}
> +
>  static void xen_setup_post(MachineState *ms, AccelState *accel)
>  {
>      int rc;
> @@ -177,6 +187,12 @@ static void xen_accel_class_init(ObjectClass *oc, void *data)
>      ac->compat_props = g_ptr_array_new();
>
>      compat_props_add(ac->compat_props, compat, G_N_ELEMENTS(compat));
> +
> +    object_class_property_add_bool(oc, "igd-passthru",
> +        xen_get_igd_gfx_passthru, xen_set_igd_gfx_passthru,
> +        &error_abort);
> +    object_class_property_set_description(oc, "igd-passthru",
> +        "Set on/off to enable/disable igd passthrou", &error_abort);
>  }
>
>  #define TYPE_XEN_ACCEL ACCEL_CLASS_NAME("xen")
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 36fcbda..cdcf481 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -287,7 +287,6 @@ struct MachineState {
>      bool mem_merge;
>      bool usb;
>      bool usb_disabled;
> -    bool igd_gfx_passthru;
>      char *firmware;
>      bool iommu;
>      bool suppress_vmdesc;
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 3931f90..5b43a83 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -37,7 +37,6 @@ DEF("machine", HAS_ARG, QEMU_OPTION_machine, \
>      "                kvm_shadow_mem=size of KVM shadow MMU in bytes\n"
>      "                dump-guest-core=on|off include guest memory in a core dump (default=on)\n"
>      "                mem-merge=on|off controls memory merge support (default: on)\n"
> -    "                igd-passthru=on|off controls IGD GFX passthrough support (default=off)\n"
>      "                aes-key-wrap=on|off controls support for AES key wrapping (default=on)\n"
>      "                dea-key-wrap=on|off controls support for DEA key wrapping (default=on)\n"
>      "                suppress-vmdesc=on|off disables self-describing migration (default=off)\n"
> @@ -71,8 +70,6 @@ more than one accelerator specified, the next one is used if the previous one
>  fails to initialize.
>  @item kernel_irqchip=on|off
>  Controls in-kernel irqchip support for the chosen accelerator when available.
> -@item gfx_passthru=on|off
> -Enables IGD GFX passthrough support for the chosen machine when available.
>  @item vmport=on|off|auto
>  Enables emulation of VMWare IO port, for vmmouse etc. auto says to select the
>  value based on accel. For accel=xen the default is off otherwise the default
> @@ -118,8 +115,9 @@ Select CPU model (@code{-cpu help} for list and additional feature selection)
>  ETEXI
>
>  DEF("accel", HAS_ARG, QEMU_OPTION_accel,
> -    "-accel [accel=]accelerator[,thread=single|multi]\n"
> +    "-accel [accel=]accelerator[,prop[=value][,...]]\n"
>      "                select accelerator (kvm, xen, hax, hvf, whpx or tcg; use 'help' for a list)\n"
> +    "                igd-passthru=on|off (enable Xen integrated Intel graphics passthrough, default=off)\n"
>      "                tb-size=n (TCG translation block cache size)\n"
>      "                thread=single|multi (enable multi-threaded TCG)\n", QEMU_ARCH_ALL)
>  STEXI
> @@ -130,6 +128,9 @@ kvm, xen, hax, hvf, whpx or tcg can be available. By default, tcg is used. If th
>  more than one accelerator specified, the next one is used if the previous one
>  fails to initialize.
>  @table @option
> +@item igd-passthru=on|off
> +When Xen is in use, this option controls whether Intel integrated graphics
> +devices can be passed through to the guest (default=off)
>  @item tb-size=@var{n}
>  Controls the size (in MiB) of the TCG translation block cache.
>  @item thread=single|multi
> diff --git a/vl.c b/vl.c
> index 06c6ad9..7d8fed1 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1256,13 +1256,6 @@ static void configure_msg(QemuOpts *opts)
>  }
>
>
> -/* Now we still need this for compatibility with XEN. */
> -bool has_igd_gfx_passthru;
> -static void igd_gfx_passthru(void)
> -{
> -    has_igd_gfx_passthru = current_machine->igd_gfx_passthru;
> -}
> -
>  /***********************************************************/
>  /* USB devices */
>
> @@ -2645,6 +2638,10 @@ static int machine_set_property(void *opaque,
>      if (g_str_equal(qom_name, "accel")) {
>          return 0;
>      }
> +    if (g_str_equal(qom_name, "igd-passthru")) {
> +        object_register_sugar_prop(ACCEL_CLASS_NAME("xen"), qom_name, value);
> +        return 0;
> +    }
>
>      r = object_parse_property_opt(opaque, name, value, "type", errp);
>      g_free(qom_name);
> @@ -4449,9 +4446,6 @@ int main(int argc, char **argv, char **envp)
>              exit(1);
>      }
>
> -    /* Check if IGD GFX passthrough. */
> -    igd_gfx_passthru();
> -
>      /* init generic devices */
>      rom_set_order_override(FW_CFG_ORDER_OVERRIDE_DEVICE);
>      qemu_opts_foreach(qemu_find_opts("device"),
> --
> 1.8.3.1
>
>
>

Re: [Xen-devel] [PATCH 13/16] xen: convert "-machine igd-passthru" to an accelerator property

[Paul Durrant]

On Wed, 13 Nov 2019 at 14:53, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> The first machine property to fall is Xen's Intel integrated graphics
> passthrough.  The "-machine igd-passthru" option does not set anymore
> a property on the machine object, but desugars to a GlobalProperty on
> accelerator objects.
>
> The setter is very simple, since the value ends up in a
> global variable, so this patch also provides an example before the more
> complicated cases that follow it.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Did something go wrong with get_maintainers here? The Xen maintainers
ought to have been cc-ed. The Xen toolstack will require consequent
modification.

Cc-ing (rest of) Xen maintainers and xen-devel manually.

  Paul

> ---
>  hw/core/machine.c   | 20 --------------------
>  hw/xen/xen-common.c | 16 ++++++++++++++++
>  include/hw/boards.h |  1 -
>  qemu-options.hx     |  9 +++++----
>  vl.c                | 14 ++++----------
>  5 files changed, 25 insertions(+), 35 deletions(-)
>
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 45ddfb6..d7a0356 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -412,20 +412,6 @@ static void machine_set_graphics(Object *obj, bool value, Error **errp)
>      ms->enable_graphics = value;
>  }
>
> -static bool machine_get_igd_gfx_passthru(Object *obj, Error **errp)
> -{
> -    MachineState *ms = MACHINE(obj);
> -
> -    return ms->igd_gfx_passthru;
> -}
> -
> -static void machine_set_igd_gfx_passthru(Object *obj, bool value, Error **errp)
> -{
> -    MachineState *ms = MACHINE(obj);
> -
> -    ms->igd_gfx_passthru = value;
> -}
> -
>  static char *machine_get_firmware(Object *obj, Error **errp)
>  {
>      MachineState *ms = MACHINE(obj);
> @@ -862,12 +848,6 @@ static void machine_class_init(ObjectClass *oc, void *data)
>      object_class_property_set_description(oc, "graphics",
>          "Set on/off to enable/disable graphics emulation", &error_abort);
>
> -    object_class_property_add_bool(oc, "igd-passthru",
> -        machine_get_igd_gfx_passthru, machine_set_igd_gfx_passthru,
> -        &error_abort);
> -    object_class_property_set_description(oc, "igd-passthru",
> -        "Set on/off to enable/disable igd passthrou", &error_abort);
> -
>      object_class_property_add_str(oc, "firmware",
>          machine_get_firmware, machine_set_firmware,
>          &error_abort);
> diff --git a/hw/xen/xen-common.c b/hw/xen/xen-common.c
> index 5284b0d..6cba30c 100644
> --- a/hw/xen/xen-common.c
> +++ b/hw/xen/xen-common.c
> @@ -124,6 +124,16 @@ static void xen_change_state_handler(void *opaque, int running,
>      }
>  }
>
> +static bool xen_get_igd_gfx_passthru(Object *obj, Error **errp)
> +{
> +    return has_igd_gfx_passthru;
> +}
> +
> +static void xen_set_igd_gfx_passthru(Object *obj, bool value, Error **errp)
> +{
> +    has_igd_gfx_passthru = value;
> +}
> +
>  static void xen_setup_post(MachineState *ms, AccelState *accel)
>  {
>      int rc;
> @@ -177,6 +187,12 @@ static void xen_accel_class_init(ObjectClass *oc, void *data)
>      ac->compat_props = g_ptr_array_new();
>
>      compat_props_add(ac->compat_props, compat, G_N_ELEMENTS(compat));
> +
> +    object_class_property_add_bool(oc, "igd-passthru",
> +        xen_get_igd_gfx_passthru, xen_set_igd_gfx_passthru,
> +        &error_abort);
> +    object_class_property_set_description(oc, "igd-passthru",
> +        "Set on/off to enable/disable igd passthrou", &error_abort);
>  }
>
>  #define TYPE_XEN_ACCEL ACCEL_CLASS_NAME("xen")
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 36fcbda..cdcf481 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -287,7 +287,6 @@ struct MachineState {
>      bool mem_merge;
>      bool usb;
>      bool usb_disabled;
> -    bool igd_gfx_passthru;
>      char *firmware;
>      bool iommu;
>      bool suppress_vmdesc;
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 3931f90..5b43a83 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -37,7 +37,6 @@ DEF("machine", HAS_ARG, QEMU_OPTION_machine, \
>      "                kvm_shadow_mem=size of KVM shadow MMU in bytes\n"
>      "                dump-guest-core=on|off include guest memory in a core dump (default=on)\n"
>      "                mem-merge=on|off controls memory merge support (default: on)\n"
> -    "                igd-passthru=on|off controls IGD GFX passthrough support (default=off)\n"
>      "                aes-key-wrap=on|off controls support for AES key wrapping (default=on)\n"
>      "                dea-key-wrap=on|off controls support for DEA key wrapping (default=on)\n"
>      "                suppress-vmdesc=on|off disables self-describing migration (default=off)\n"
> @@ -71,8 +70,6 @@ more than one accelerator specified, the next one is used if the previous one
>  fails to initialize.
>  @item kernel_irqchip=on|off
>  Controls in-kernel irqchip support for the chosen accelerator when available.
> -@item gfx_passthru=on|off
> -Enables IGD GFX passthrough support for the chosen machine when available.
>  @item vmport=on|off|auto
>  Enables emulation of VMWare IO port, for vmmouse etc. auto says to select the
>  value based on accel. For accel=xen the default is off otherwise the default
> @@ -118,8 +115,9 @@ Select CPU model (@code{-cpu help} for list and additional feature selection)
>  ETEXI
>
>  DEF("accel", HAS_ARG, QEMU_OPTION_accel,
> -    "-accel [accel=]accelerator[,thread=single|multi]\n"
> +    "-accel [accel=]accelerator[,prop[=value][,...]]\n"
>      "                select accelerator (kvm, xen, hax, hvf, whpx or tcg; use 'help' for a list)\n"
> +    "                igd-passthru=on|off (enable Xen integrated Intel graphics passthrough, default=off)\n"
>      "                tb-size=n (TCG translation block cache size)\n"
>      "                thread=single|multi (enable multi-threaded TCG)\n", QEMU_ARCH_ALL)
>  STEXI
> @@ -130,6 +128,9 @@ kvm, xen, hax, hvf, whpx or tcg can be available. By default, tcg is used. If th
>  more than one accelerator specified, the next one is used if the previous one
>  fails to initialize.
>  @table @option
> +@item igd-passthru=on|off
> +When Xen is in use, this option controls whether Intel integrated graphics
> +devices can be passed through to the guest (default=off)
>  @item tb-size=@var{n}
>  Controls the size (in MiB) of the TCG translation block cache.
>  @item thread=single|multi
> diff --git a/vl.c b/vl.c
> index 06c6ad9..7d8fed1 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1256,13 +1256,6 @@ static void configure_msg(QemuOpts *opts)
>  }
>
>
> -/* Now we still need this for compatibility with XEN. */
> -bool has_igd_gfx_passthru;
> -static void igd_gfx_passthru(void)
> -{
> -    has_igd_gfx_passthru = current_machine->igd_gfx_passthru;
> -}
> -
>  /***********************************************************/
>  /* USB devices */
>
> @@ -2645,6 +2638,10 @@ static int machine_set_property(void *opaque,
>      if (g_str_equal(qom_name, "accel")) {
>          return 0;
>      }
> +    if (g_str_equal(qom_name, "igd-passthru")) {
> +        object_register_sugar_prop(ACCEL_CLASS_NAME("xen"), qom_name, value);
> +        return 0;
> +    }
>
>      r = object_parse_property_opt(opaque, name, value, "type", errp);
>      g_free(qom_name);
> @@ -4449,9 +4446,6 @@ int main(int argc, char **argv, char **envp)
>              exit(1);
>      }
>
> -    /* Check if IGD GFX passthrough. */
> -    igd_gfx_passthru();
> -
>      /* init generic devices */
>      rom_set_order_override(FW_CFG_ORDER_OVERRIDE_DEVICE);
>      qemu_opts_foreach(qemu_find_opts("device"),
> --
> 1.8.3.1
>
>
>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 13/16] xen: convert "-machine igd-passthru" to an accelerator property

[Paolo Bonzini]

On 14/11/19 10:39, Paul Durrant wrote:
> On Wed, 13 Nov 2019 at 14:53, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>> The first machine property to fall is Xen's Intel integrated graphics
>> passthrough.  The "-machine igd-passthru" option does not set anymore
>> a property on the machine object, but desugars to a GlobalProperty on
>> accelerator objects.
>>
>> The setter is very simple, since the value ends up in a
>> global variable, so this patch also provides an example before the more
>> complicated cases that follow it.
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> Did something go wrong with get_maintainers here? The Xen maintainers
> ought to have been cc-ed. The Xen toolstack will require consequent
> modification.

No, I just didn't use getmaintainers, my bad.  But backwards-compatible
syntactic sugar is provided, so no modifications are needed to Xen.  See
here in the code:

+    if (g_str_equal(qom_name, "igd-passthru")) {
+        object_register_sugar_prop(ACCEL_CLASS_NAME("xen"), qom_name, value);
+        return 0;
+    }

Paolo

> Cc-ing (rest of) Xen maintainers and xen-devel manually.
> 
>   Paul
> 
>> ---
>>  hw/core/machine.c   | 20 --------------------
>>  hw/xen/xen-common.c | 16 ++++++++++++++++
>>  include/hw/boards.h |  1 -
>>  qemu-options.hx     |  9 +++++----
>>  vl.c                | 14 ++++----------
>>  5 files changed, 25 insertions(+), 35 deletions(-)
>>
>> diff --git a/hw/core/machine.c b/hw/core/machine.c
>> index 45ddfb6..d7a0356 100644
>> --- a/hw/core/machine.c
>> +++ b/hw/core/machine.c
>> @@ -412,20 +412,6 @@ static void machine_set_graphics(Object *obj, bool value, Error **errp)
>>      ms->enable_graphics = value;
>>  }
>>
>> -static bool machine_get_igd_gfx_passthru(Object *obj, Error **errp)
>> -{
>> -    MachineState *ms = MACHINE(obj);
>> -
>> -    return ms->igd_gfx_passthru;
>> -}
>> -
>> -static void machine_set_igd_gfx_passthru(Object *obj, bool value, Error **errp)
>> -{
>> -    MachineState *ms = MACHINE(obj);
>> -
>> -    ms->igd_gfx_passthru = value;
>> -}
>> -
>>  static char *machine_get_firmware(Object *obj, Error **errp)
>>  {
>>      MachineState *ms = MACHINE(obj);
>> @@ -862,12 +848,6 @@ static void machine_class_init(ObjectClass *oc, void *data)
>>      object_class_property_set_description(oc, "graphics",
>>          "Set on/off to enable/disable graphics emulation", &error_abort);
>>
>> -    object_class_property_add_bool(oc, "igd-passthru",
>> -        machine_get_igd_gfx_passthru, machine_set_igd_gfx_passthru,
>> -        &error_abort);
>> -    object_class_property_set_description(oc, "igd-passthru",
>> -        "Set on/off to enable/disable igd passthrou", &error_abort);
>> -
>>      object_class_property_add_str(oc, "firmware",
>>          machine_get_firmware, machine_set_firmware,
>>          &error_abort);
>> diff --git a/hw/xen/xen-common.c b/hw/xen/xen-common.c
>> index 5284b0d..6cba30c 100644
>> --- a/hw/xen/xen-common.c
>> +++ b/hw/xen/xen-common.c
>> @@ -124,6 +124,16 @@ static void xen_change_state_handler(void *opaque, int running,
>>      }
>>  }
>>
>> +static bool xen_get_igd_gfx_passthru(Object *obj, Error **errp)
>> +{
>> +    return has_igd_gfx_passthru;
>> +}
>> +
>> +static void xen_set_igd_gfx_passthru(Object *obj, bool value, Error **errp)
>> +{
>> +    has_igd_gfx_passthru = value;
>> +}
>> +
>>  static void xen_setup_post(MachineState *ms, AccelState *accel)
>>  {
>>      int rc;
>> @@ -177,6 +187,12 @@ static void xen_accel_class_init(ObjectClass *oc, void *data)
>>      ac->compat_props = g_ptr_array_new();
>>
>>      compat_props_add(ac->compat_props, compat, G_N_ELEMENTS(compat));
>> +
>> +    object_class_property_add_bool(oc, "igd-passthru",
>> +        xen_get_igd_gfx_passthru, xen_set_igd_gfx_passthru,
>> +        &error_abort);
>> +    object_class_property_set_description(oc, "igd-passthru",
>> +        "Set on/off to enable/disable igd passthrou", &error_abort);
>>  }
>>
>>  #define TYPE_XEN_ACCEL ACCEL_CLASS_NAME("xen")
>> diff --git a/include/hw/boards.h b/include/hw/boards.h
>> index 36fcbda..cdcf481 100644
>> --- a/include/hw/boards.h
>> +++ b/include/hw/boards.h
>> @@ -287,7 +287,6 @@ struct MachineState {
>>      bool mem_merge;
>>      bool usb;
>>      bool usb_disabled;
>> -    bool igd_gfx_passthru;
>>      char *firmware;
>>      bool iommu;
>>      bool suppress_vmdesc;
>> diff --git a/qemu-options.hx b/qemu-options.hx
>> index 3931f90..5b43a83 100644
>> --- a/qemu-options.hx
>> +++ b/qemu-options.hx
>> @@ -37,7 +37,6 @@ DEF("machine", HAS_ARG, QEMU_OPTION_machine, \
>>      "                kvm_shadow_mem=size of KVM shadow MMU in bytes\n"
>>      "                dump-guest-core=on|off include guest memory in a core dump (default=on)\n"
>>      "                mem-merge=on|off controls memory merge support (default: on)\n"
>> -    "                igd-passthru=on|off controls IGD GFX passthrough support (default=off)\n"
>>      "                aes-key-wrap=on|off controls support for AES key wrapping (default=on)\n"
>>      "                dea-key-wrap=on|off controls support for DEA key wrapping (default=on)\n"
>>      "                suppress-vmdesc=on|off disables self-describing migration (default=off)\n"
>> @@ -71,8 +70,6 @@ more than one accelerator specified, the next one is used if the previous one
>>  fails to initialize.
>>  @item kernel_irqchip=on|off
>>  Controls in-kernel irqchip support for the chosen accelerator when available.
>> -@item gfx_passthru=on|off
>> -Enables IGD GFX passthrough support for the chosen machine when available.
>>  @item vmport=on|off|auto
>>  Enables emulation of VMWare IO port, for vmmouse etc. auto says to select the
>>  value based on accel. For accel=xen the default is off otherwise the default
>> @@ -118,8 +115,9 @@ Select CPU model (@code{-cpu help} for list and additional feature selection)
>>  ETEXI
>>
>>  DEF("accel", HAS_ARG, QEMU_OPTION_accel,
>> -    "-accel [accel=]accelerator[,thread=single|multi]\n"
>> +    "-accel [accel=]accelerator[,prop[=value][,...]]\n"
>>      "                select accelerator (kvm, xen, hax, hvf, whpx or tcg; use 'help' for a list)\n"
>> +    "                igd-passthru=on|off (enable Xen integrated Intel graphics passthrough, default=off)\n"
>>      "                tb-size=n (TCG translation block cache size)\n"
>>      "                thread=single|multi (enable multi-threaded TCG)\n", QEMU_ARCH_ALL)
>>  STEXI
>> @@ -130,6 +128,9 @@ kvm, xen, hax, hvf, whpx or tcg can be available. By default, tcg is used. If th
>>  more than one accelerator specified, the next one is used if the previous one
>>  fails to initialize.
>>  @table @option
>> +@item igd-passthru=on|off
>> +When Xen is in use, this option controls whether Intel integrated graphics
>> +devices can be passed through to the guest (default=off)
>>  @item tb-size=@var{n}
>>  Controls the size (in MiB) of the TCG translation block cache.
>>  @item thread=single|multi
>> diff --git a/vl.c b/vl.c
>> index 06c6ad9..7d8fed1 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -1256,13 +1256,6 @@ static void configure_msg(QemuOpts *opts)
>>  }
>>
>>
>> -/* Now we still need this for compatibility with XEN. */
>> -bool has_igd_gfx_passthru;
>> -static void igd_gfx_passthru(void)
>> -{
>> -    has_igd_gfx_passthru = current_machine->igd_gfx_passthru;
>> -}
>> -
>>  /***********************************************************/
>>  /* USB devices */
>>
>> @@ -2645,6 +2638,10 @@ static int machine_set_property(void *opaque,
>>      if (g_str_equal(qom_name, "accel")) {
>>          return 0;
>>      }
>> +    if (g_str_equal(qom_name, "igd-passthru")) {
>> +        object_register_sugar_prop(ACCEL_CLASS_NAME("xen"), qom_name, value);
>> +        return 0;
>> +    }
>>
>>      r = object_parse_property_opt(opaque, name, value, "type", errp);
>>      g_free(qom_name);
>> @@ -4449,9 +4446,6 @@ int main(int argc, char **argv, char **envp)
>>              exit(1);
>>      }
>>
>> -    /* Check if IGD GFX passthrough. */
>> -    igd_gfx_passthru();
>> -
>>      /* init generic devices */
>>      rom_set_order_override(FW_CFG_ORDER_OVERRIDE_DEVICE);
>>      qemu_opts_foreach(qemu_find_opts("device"),
>> --
>> 1.8.3.1
>>
>>
>>
> 

Re: [Xen-devel] [PATCH 13/16] xen: convert "-machine igd-passthru" to an accelerator property

[Paolo Bonzini]

On 14/11/19 10:39, Paul Durrant wrote:
> On Wed, 13 Nov 2019 at 14:53, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>> The first machine property to fall is Xen's Intel integrated graphics
>> passthrough.  The "-machine igd-passthru" option does not set anymore
>> a property on the machine object, but desugars to a GlobalProperty on
>> accelerator objects.
>>
>> The setter is very simple, since the value ends up in a
>> global variable, so this patch also provides an example before the more
>> complicated cases that follow it.
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> Did something go wrong with get_maintainers here? The Xen maintainers
> ought to have been cc-ed. The Xen toolstack will require consequent
> modification.

No, I just didn't use getmaintainers, my bad.  But backwards-compatible
syntactic sugar is provided, so no modifications are needed to Xen.  See
here in the code:

+    if (g_str_equal(qom_name, "igd-passthru")) {
+        object_register_sugar_prop(ACCEL_CLASS_NAME("xen"), qom_name, value);
+        return 0;
+    }

Paolo

> Cc-ing (rest of) Xen maintainers and xen-devel manually.
> 
>   Paul
> 
>> ---
>>  hw/core/machine.c   | 20 --------------------
>>  hw/xen/xen-common.c | 16 ++++++++++++++++
>>  include/hw/boards.h |  1 -
>>  qemu-options.hx     |  9 +++++----
>>  vl.c                | 14 ++++----------
>>  5 files changed, 25 insertions(+), 35 deletions(-)
>>
>> diff --git a/hw/core/machine.c b/hw/core/machine.c
>> index 45ddfb6..d7a0356 100644
>> --- a/hw/core/machine.c
>> +++ b/hw/core/machine.c
>> @@ -412,20 +412,6 @@ static void machine_set_graphics(Object *obj, bool value, Error **errp)
>>      ms->enable_graphics = value;
>>  }
>>
>> -static bool machine_get_igd_gfx_passthru(Object *obj, Error **errp)
>> -{
>> -    MachineState *ms = MACHINE(obj);
>> -
>> -    return ms->igd_gfx_passthru;
>> -}
>> -
>> -static void machine_set_igd_gfx_passthru(Object *obj, bool value, Error **errp)
>> -{
>> -    MachineState *ms = MACHINE(obj);
>> -
>> -    ms->igd_gfx_passthru = value;
>> -}
>> -
>>  static char *machine_get_firmware(Object *obj, Error **errp)
>>  {
>>      MachineState *ms = MACHINE(obj);
>> @@ -862,12 +848,6 @@ static void machine_class_init(ObjectClass *oc, void *data)
>>      object_class_property_set_description(oc, "graphics",
>>          "Set on/off to enable/disable graphics emulation", &error_abort);
>>
>> -    object_class_property_add_bool(oc, "igd-passthru",
>> -        machine_get_igd_gfx_passthru, machine_set_igd_gfx_passthru,
>> -        &error_abort);
>> -    object_class_property_set_description(oc, "igd-passthru",
>> -        "Set on/off to enable/disable igd passthrou", &error_abort);
>> -
>>      object_class_property_add_str(oc, "firmware",
>>          machine_get_firmware, machine_set_firmware,
>>          &error_abort);
>> diff --git a/hw/xen/xen-common.c b/hw/xen/xen-common.c
>> index 5284b0d..6cba30c 100644
>> --- a/hw/xen/xen-common.c
>> +++ b/hw/xen/xen-common.c
>> @@ -124,6 +124,16 @@ static void xen_change_state_handler(void *opaque, int running,
>>      }
>>  }
>>
>> +static bool xen_get_igd_gfx_passthru(Object *obj, Error **errp)
>> +{
>> +    return has_igd_gfx_passthru;
>> +}
>> +
>> +static void xen_set_igd_gfx_passthru(Object *obj, bool value, Error **errp)
>> +{
>> +    has_igd_gfx_passthru = value;
>> +}
>> +
>>  static void xen_setup_post(MachineState *ms, AccelState *accel)
>>  {
>>      int rc;
>> @@ -177,6 +187,12 @@ static void xen_accel_class_init(ObjectClass *oc, void *data)
>>      ac->compat_props = g_ptr_array_new();
>>
>>      compat_props_add(ac->compat_props, compat, G_N_ELEMENTS(compat));
>> +
>> +    object_class_property_add_bool(oc, "igd-passthru",
>> +        xen_get_igd_gfx_passthru, xen_set_igd_gfx_passthru,
>> +        &error_abort);
>> +    object_class_property_set_description(oc, "igd-passthru",
>> +        "Set on/off to enable/disable igd passthrou", &error_abort);
>>  }
>>
>>  #define TYPE_XEN_ACCEL ACCEL_CLASS_NAME("xen")
>> diff --git a/include/hw/boards.h b/include/hw/boards.h
>> index 36fcbda..cdcf481 100644
>> --- a/include/hw/boards.h
>> +++ b/include/hw/boards.h
>> @@ -287,7 +287,6 @@ struct MachineState {
>>      bool mem_merge;
>>      bool usb;
>>      bool usb_disabled;
>> -    bool igd_gfx_passthru;
>>      char *firmware;
>>      bool iommu;
>>      bool suppress_vmdesc;
>> diff --git a/qemu-options.hx b/qemu-options.hx
>> index 3931f90..5b43a83 100644
>> --- a/qemu-options.hx
>> +++ b/qemu-options.hx
>> @@ -37,7 +37,6 @@ DEF("machine", HAS_ARG, QEMU_OPTION_machine, \
>>      "                kvm_shadow_mem=size of KVM shadow MMU in bytes\n"
>>      "                dump-guest-core=on|off include guest memory in a core dump (default=on)\n"
>>      "                mem-merge=on|off controls memory merge support (default: on)\n"
>> -    "                igd-passthru=on|off controls IGD GFX passthrough support (default=off)\n"
>>      "                aes-key-wrap=on|off controls support for AES key wrapping (default=on)\n"
>>      "                dea-key-wrap=on|off controls support for DEA key wrapping (default=on)\n"
>>      "                suppress-vmdesc=on|off disables self-describing migration (default=off)\n"
>> @@ -71,8 +70,6 @@ more than one accelerator specified, the next one is used if the previous one
>>  fails to initialize.
>>  @item kernel_irqchip=on|off
>>  Controls in-kernel irqchip support for the chosen accelerator when available.
>> -@item gfx_passthru=on|off
>> -Enables IGD GFX passthrough support for the chosen machine when available.
>>  @item vmport=on|off|auto
>>  Enables emulation of VMWare IO port, for vmmouse etc. auto says to select the
>>  value based on accel. For accel=xen the default is off otherwise the default
>> @@ -118,8 +115,9 @@ Select CPU model (@code{-cpu help} for list and additional feature selection)
>>  ETEXI
>>
>>  DEF("accel", HAS_ARG, QEMU_OPTION_accel,
>> -    "-accel [accel=]accelerator[,thread=single|multi]\n"
>> +    "-accel [accel=]accelerator[,prop[=value][,...]]\n"
>>      "                select accelerator (kvm, xen, hax, hvf, whpx or tcg; use 'help' for a list)\n"
>> +    "                igd-passthru=on|off (enable Xen integrated Intel graphics passthrough, default=off)\n"
>>      "                tb-size=n (TCG translation block cache size)\n"
>>      "                thread=single|multi (enable multi-threaded TCG)\n", QEMU_ARCH_ALL)
>>  STEXI
>> @@ -130,6 +128,9 @@ kvm, xen, hax, hvf, whpx or tcg can be available. By default, tcg is used. If th
>>  more than one accelerator specified, the next one is used if the previous one
>>  fails to initialize.
>>  @table @option
>> +@item igd-passthru=on|off
>> +When Xen is in use, this option controls whether Intel integrated graphics
>> +devices can be passed through to the guest (default=off)
>>  @item tb-size=@var{n}
>>  Controls the size (in MiB) of the TCG translation block cache.
>>  @item thread=single|multi
>> diff --git a/vl.c b/vl.c
>> index 06c6ad9..7d8fed1 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -1256,13 +1256,6 @@ static void configure_msg(QemuOpts *opts)
>>  }
>>
>>
>> -/* Now we still need this for compatibility with XEN. */
>> -bool has_igd_gfx_passthru;
>> -static void igd_gfx_passthru(void)
>> -{
>> -    has_igd_gfx_passthru = current_machine->igd_gfx_passthru;
>> -}
>> -
>>  /***********************************************************/
>>  /* USB devices */
>>
>> @@ -2645,6 +2638,10 @@ static int machine_set_property(void *opaque,
>>      if (g_str_equal(qom_name, "accel")) {
>>          return 0;
>>      }
>> +    if (g_str_equal(qom_name, "igd-passthru")) {
>> +        object_register_sugar_prop(ACCEL_CLASS_NAME("xen"), qom_name, value);
>> +        return 0;
>> +    }
>>
>>      r = object_parse_property_opt(opaque, name, value, "type", errp);
>>      g_free(qom_name);
>> @@ -4449,9 +4446,6 @@ int main(int argc, char **argv, char **envp)
>>              exit(1);
>>      }
>>
>> -    /* Check if IGD GFX passthrough. */
>> -    igd_gfx_passthru();
>> -
>>      /* init generic devices */
>>      rom_set_order_override(FW_CFG_ORDER_OVERRIDE_DEVICE);
>>      qemu_opts_foreach(qemu_find_opts("device"),
>> --
>> 1.8.3.1
>>
>>
>>
> 


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 13/16] xen: convert "-machine igd-passthru" to an accelerator property

[Paul Durrant]

On Thu, 14 Nov 2019 at 09:47, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 14/11/19 10:39, Paul Durrant wrote:
> > On Wed, 13 Nov 2019 at 14:53, Paolo Bonzini <pbonzini@redhat.com> wrote:
> >>
> >> The first machine property to fall is Xen's Intel integrated graphics
> >> passthrough.  The "-machine igd-passthru" option does not set anymore
> >> a property on the machine object, but desugars to a GlobalProperty on
> >> accelerator objects.
> >>
> >> The setter is very simple, since the value ends up in a
> >> global variable, so this patch also provides an example before the more
> >> complicated cases that follow it.
> >>
> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> >
> > Did something go wrong with get_maintainers here? The Xen maintainers
> > ought to have been cc-ed. The Xen toolstack will require consequent
> > modification.
>
> No, I just didn't use getmaintainers, my bad.  But backwards-compatible
> syntactic sugar is provided, so no modifications are needed to Xen.  See
> here in the code:
>
> +    if (g_str_equal(qom_name, "igd-passthru")) {
> +        object_register_sugar_prop(ACCEL_CLASS_NAME("xen"), qom_name, value);
> +        return 0;
> +    }

Ah, no immediate danger. Thanks for pointing this out.

Cheers,
  Paul

>
> Paolo
>
> > Cc-ing (rest of) Xen maintainers and xen-devel manually.
> >
> >   Paul
> >
> >> ---
> >>  hw/core/machine.c   | 20 --------------------
> >>  hw/xen/xen-common.c | 16 ++++++++++++++++
> >>  include/hw/boards.h |  1 -
> >>  qemu-options.hx     |  9 +++++----
> >>  vl.c                | 14 ++++----------
> >>  5 files changed, 25 insertions(+), 35 deletions(-)
> >>
> >> diff --git a/hw/core/machine.c b/hw/core/machine.c
> >> index 45ddfb6..d7a0356 100644
> >> --- a/hw/core/machine.c
> >> +++ b/hw/core/machine.c
> >> @@ -412,20 +412,6 @@ static void machine_set_graphics(Object *obj, bool value, Error **errp)
> >>      ms->enable_graphics = value;
> >>  }
> >>
> >> -static bool machine_get_igd_gfx_passthru(Object *obj, Error **errp)
> >> -{
> >> -    MachineState *ms = MACHINE(obj);
> >> -
> >> -    return ms->igd_gfx_passthru;
> >> -}
> >> -
> >> -static void machine_set_igd_gfx_passthru(Object *obj, bool value, Error **errp)
> >> -{
> >> -    MachineState *ms = MACHINE(obj);
> >> -
> >> -    ms->igd_gfx_passthru = value;
> >> -}
> >> -
> >>  static char *machine_get_firmware(Object *obj, Error **errp)
> >>  {
> >>      MachineState *ms = MACHINE(obj);
> >> @@ -862,12 +848,6 @@ static void machine_class_init(ObjectClass *oc, void *data)
> >>      object_class_property_set_description(oc, "graphics",
> >>          "Set on/off to enable/disable graphics emulation", &error_abort);
> >>
> >> -    object_class_property_add_bool(oc, "igd-passthru",
> >> -        machine_get_igd_gfx_passthru, machine_set_igd_gfx_passthru,
> >> -        &error_abort);
> >> -    object_class_property_set_description(oc, "igd-passthru",
> >> -        "Set on/off to enable/disable igd passthrou", &error_abort);
> >> -
> >>      object_class_property_add_str(oc, "firmware",
> >>          machine_get_firmware, machine_set_firmware,
> >>          &error_abort);
> >> diff --git a/hw/xen/xen-common.c b/hw/xen/xen-common.c
> >> index 5284b0d..6cba30c 100644
> >> --- a/hw/xen/xen-common.c
> >> +++ b/hw/xen/xen-common.c
> >> @@ -124,6 +124,16 @@ static void xen_change_state_handler(void *opaque, int running,
> >>      }
> >>  }
> >>
> >> +static bool xen_get_igd_gfx_passthru(Object *obj, Error **errp)
> >> +{
> >> +    return has_igd_gfx_passthru;
> >> +}
> >> +
> >> +static void xen_set_igd_gfx_passthru(Object *obj, bool value, Error **errp)
> >> +{
> >> +    has_igd_gfx_passthru = value;
> >> +}
> >> +
> >>  static void xen_setup_post(MachineState *ms, AccelState *accel)
> >>  {
> >>      int rc;
> >> @@ -177,6 +187,12 @@ static void xen_accel_class_init(ObjectClass *oc, void *data)
> >>      ac->compat_props = g_ptr_array_new();
> >>
> >>      compat_props_add(ac->compat_props, compat, G_N_ELEMENTS(compat));
> >> +
> >> +    object_class_property_add_bool(oc, "igd-passthru",
> >> +        xen_get_igd_gfx_passthru, xen_set_igd_gfx_passthru,
> >> +        &error_abort);
> >> +    object_class_property_set_description(oc, "igd-passthru",
> >> +        "Set on/off to enable/disable igd passthrou", &error_abort);
> >>  }
> >>
> >>  #define TYPE_XEN_ACCEL ACCEL_CLASS_NAME("xen")
> >> diff --git a/include/hw/boards.h b/include/hw/boards.h
> >> index 36fcbda..cdcf481 100644
> >> --- a/include/hw/boards.h
> >> +++ b/include/hw/boards.h
> >> @@ -287,7 +287,6 @@ struct MachineState {
> >>      bool mem_merge;
> >>      bool usb;
> >>      bool usb_disabled;
> >> -    bool igd_gfx_passthru;
> >>      char *firmware;
> >>      bool iommu;
> >>      bool suppress_vmdesc;
> >> diff --git a/qemu-options.hx b/qemu-options.hx
> >> index 3931f90..5b43a83 100644
> >> --- a/qemu-options.hx
> >> +++ b/qemu-options.hx
> >> @@ -37,7 +37,6 @@ DEF("machine", HAS_ARG, QEMU_OPTION_machine, \
> >>      "                kvm_shadow_mem=size of KVM shadow MMU in bytes\n"
> >>      "                dump-guest-core=on|off include guest memory in a core dump (default=on)\n"
> >>      "                mem-merge=on|off controls memory merge support (default: on)\n"
> >> -    "                igd-passthru=on|off controls IGD GFX passthrough support (default=off)\n"
> >>      "                aes-key-wrap=on|off controls support for AES key wrapping (default=on)\n"
> >>      "                dea-key-wrap=on|off controls support for DEA key wrapping (default=on)\n"
> >>      "                suppress-vmdesc=on|off disables self-describing migration (default=off)\n"
> >> @@ -71,8 +70,6 @@ more than one accelerator specified, the next one is used if the previous one
> >>  fails to initialize.
> >>  @item kernel_irqchip=on|off
> >>  Controls in-kernel irqchip support for the chosen accelerator when available.
> >> -@item gfx_passthru=on|off
> >> -Enables IGD GFX passthrough support for the chosen machine when available.
> >>  @item vmport=on|off|auto
> >>  Enables emulation of VMWare IO port, for vmmouse etc. auto says to select the
> >>  value based on accel. For accel=xen the default is off otherwise the default
> >> @@ -118,8 +115,9 @@ Select CPU model (@code{-cpu help} for list and additional feature selection)
> >>  ETEXI
> >>
> >>  DEF("accel", HAS_ARG, QEMU_OPTION_accel,
> >> -    "-accel [accel=]accelerator[,thread=single|multi]\n"
> >> +    "-accel [accel=]accelerator[,prop[=value][,...]]\n"
> >>      "                select accelerator (kvm, xen, hax, hvf, whpx or tcg; use 'help' for a list)\n"
> >> +    "                igd-passthru=on|off (enable Xen integrated Intel graphics passthrough, default=off)\n"
> >>      "                tb-size=n (TCG translation block cache size)\n"
> >>      "                thread=single|multi (enable multi-threaded TCG)\n", QEMU_ARCH_ALL)
> >>  STEXI
> >> @@ -130,6 +128,9 @@ kvm, xen, hax, hvf, whpx or tcg can be available. By default, tcg is used. If th
> >>  more than one accelerator specified, the next one is used if the previous one
> >>  fails to initialize.
> >>  @table @option
> >> +@item igd-passthru=on|off
> >> +When Xen is in use, this option controls whether Intel integrated graphics
> >> +devices can be passed through to the guest (default=off)
> >>  @item tb-size=@var{n}
> >>  Controls the size (in MiB) of the TCG translation block cache.
> >>  @item thread=single|multi
> >> diff --git a/vl.c b/vl.c
> >> index 06c6ad9..7d8fed1 100644
> >> --- a/vl.c
> >> +++ b/vl.c
> >> @@ -1256,13 +1256,6 @@ static void configure_msg(QemuOpts *opts)
> >>  }
> >>
> >>
> >> -/* Now we still need this for compatibility with XEN. */
> >> -bool has_igd_gfx_passthru;
> >> -static void igd_gfx_passthru(void)
> >> -{
> >> -    has_igd_gfx_passthru = current_machine->igd_gfx_passthru;
> >> -}
> >> -
> >>  /***********************************************************/
> >>  /* USB devices */
> >>
> >> @@ -2645,6 +2638,10 @@ static int machine_set_property(void *opaque,
> >>      if (g_str_equal(qom_name, "accel")) {
> >>          return 0;
> >>      }
> >> +    if (g_str_equal(qom_name, "igd-passthru")) {
> >> +        object_register_sugar_prop(ACCEL_CLASS_NAME("xen"), qom_name, value);
> >> +        return 0;
> >> +    }
> >>
> >>      r = object_parse_property_opt(opaque, name, value, "type", errp);
> >>      g_free(qom_name);
> >> @@ -4449,9 +4446,6 @@ int main(int argc, char **argv, char **envp)
> >>              exit(1);
> >>      }
> >>
> >> -    /* Check if IGD GFX passthrough. */
> >> -    igd_gfx_passthru();
> >> -
> >>      /* init generic devices */
> >>      rom_set_order_override(FW_CFG_ORDER_OVERRIDE_DEVICE);
> >>      qemu_opts_foreach(qemu_find_opts("device"),
> >> --
> >> 1.8.3.1
> >>
> >>
> >>
> >
>

Re: [Xen-devel] [PATCH 13/16] xen: convert "-machine igd-passthru" to an accelerator property

[Paul Durrant]

On Thu, 14 Nov 2019 at 09:47, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 14/11/19 10:39, Paul Durrant wrote:
> > On Wed, 13 Nov 2019 at 14:53, Paolo Bonzini <pbonzini@redhat.com> wrote:
> >>
> >> The first machine property to fall is Xen's Intel integrated graphics
> >> passthrough.  The "-machine igd-passthru" option does not set anymore
> >> a property on the machine object, but desugars to a GlobalProperty on
> >> accelerator objects.
> >>
> >> The setter is very simple, since the value ends up in a
> >> global variable, so this patch also provides an example before the more
> >> complicated cases that follow it.
> >>
> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> >
> > Did something go wrong with get_maintainers here? The Xen maintainers
> > ought to have been cc-ed. The Xen toolstack will require consequent
> > modification.
>
> No, I just didn't use getmaintainers, my bad.  But backwards-compatible
> syntactic sugar is provided, so no modifications are needed to Xen.  See
> here in the code:
>
> +    if (g_str_equal(qom_name, "igd-passthru")) {
> +        object_register_sugar_prop(ACCEL_CLASS_NAME("xen"), qom_name, value);
> +        return 0;
> +    }

Ah, no immediate danger. Thanks for pointing this out.

Cheers,
  Paul

>
> Paolo
>
> > Cc-ing (rest of) Xen maintainers and xen-devel manually.
> >
> >   Paul
> >
> >> ---
> >>  hw/core/machine.c   | 20 --------------------
> >>  hw/xen/xen-common.c | 16 ++++++++++++++++
> >>  include/hw/boards.h |  1 -
> >>  qemu-options.hx     |  9 +++++----
> >>  vl.c                | 14 ++++----------
> >>  5 files changed, 25 insertions(+), 35 deletions(-)
> >>
> >> diff --git a/hw/core/machine.c b/hw/core/machine.c
> >> index 45ddfb6..d7a0356 100644
> >> --- a/hw/core/machine.c
> >> +++ b/hw/core/machine.c
> >> @@ -412,20 +412,6 @@ static void machine_set_graphics(Object *obj, bool value, Error **errp)
> >>      ms->enable_graphics = value;
> >>  }
> >>
> >> -static bool machine_get_igd_gfx_passthru(Object *obj, Error **errp)
> >> -{
> >> -    MachineState *ms = MACHINE(obj);
> >> -
> >> -    return ms->igd_gfx_passthru;
> >> -}
> >> -
> >> -static void machine_set_igd_gfx_passthru(Object *obj, bool value, Error **errp)
> >> -{
> >> -    MachineState *ms = MACHINE(obj);
> >> -
> >> -    ms->igd_gfx_passthru = value;
> >> -}
> >> -
> >>  static char *machine_get_firmware(Object *obj, Error **errp)
> >>  {
> >>      MachineState *ms = MACHINE(obj);
> >> @@ -862,12 +848,6 @@ static void machine_class_init(ObjectClass *oc, void *data)
> >>      object_class_property_set_description(oc, "graphics",
> >>          "Set on/off to enable/disable graphics emulation", &error_abort);
> >>
> >> -    object_class_property_add_bool(oc, "igd-passthru",
> >> -        machine_get_igd_gfx_passthru, machine_set_igd_gfx_passthru,
> >> -        &error_abort);
> >> -    object_class_property_set_description(oc, "igd-passthru",
> >> -        "Set on/off to enable/disable igd passthrou", &error_abort);
> >> -
> >>      object_class_property_add_str(oc, "firmware",
> >>          machine_get_firmware, machine_set_firmware,
> >>          &error_abort);
> >> diff --git a/hw/xen/xen-common.c b/hw/xen/xen-common.c
> >> index 5284b0d..6cba30c 100644
> >> --- a/hw/xen/xen-common.c
> >> +++ b/hw/xen/xen-common.c
> >> @@ -124,6 +124,16 @@ static void xen_change_state_handler(void *opaque, int running,
> >>      }
> >>  }
> >>
> >> +static bool xen_get_igd_gfx_passthru(Object *obj, Error **errp)
> >> +{
> >> +    return has_igd_gfx_passthru;
> >> +}
> >> +
> >> +static void xen_set_igd_gfx_passthru(Object *obj, bool value, Error **errp)
> >> +{
> >> +    has_igd_gfx_passthru = value;
> >> +}
> >> +
> >>  static void xen_setup_post(MachineState *ms, AccelState *accel)
> >>  {
> >>      int rc;
> >> @@ -177,6 +187,12 @@ static void xen_accel_class_init(ObjectClass *oc, void *data)
> >>      ac->compat_props = g_ptr_array_new();
> >>
> >>      compat_props_add(ac->compat_props, compat, G_N_ELEMENTS(compat));
> >> +
> >> +    object_class_property_add_bool(oc, "igd-passthru",
> >> +        xen_get_igd_gfx_passthru, xen_set_igd_gfx_passthru,
> >> +        &error_abort);
> >> +    object_class_property_set_description(oc, "igd-passthru",
> >> +        "Set on/off to enable/disable igd passthrou", &error_abort);
> >>  }
> >>
> >>  #define TYPE_XEN_ACCEL ACCEL_CLASS_NAME("xen")
> >> diff --git a/include/hw/boards.h b/include/hw/boards.h
> >> index 36fcbda..cdcf481 100644
> >> --- a/include/hw/boards.h
> >> +++ b/include/hw/boards.h
> >> @@ -287,7 +287,6 @@ struct MachineState {
> >>      bool mem_merge;
> >>      bool usb;
> >>      bool usb_disabled;
> >> -    bool igd_gfx_passthru;
> >>      char *firmware;
> >>      bool iommu;
> >>      bool suppress_vmdesc;
> >> diff --git a/qemu-options.hx b/qemu-options.hx
> >> index 3931f90..5b43a83 100644
> >> --- a/qemu-options.hx
> >> +++ b/qemu-options.hx
> >> @@ -37,7 +37,6 @@ DEF("machine", HAS_ARG, QEMU_OPTION_machine, \
> >>      "                kvm_shadow_mem=size of KVM shadow MMU in bytes\n"
> >>      "                dump-guest-core=on|off include guest memory in a core dump (default=on)\n"
> >>      "                mem-merge=on|off controls memory merge support (default: on)\n"
> >> -    "                igd-passthru=on|off controls IGD GFX passthrough support (default=off)\n"
> >>      "                aes-key-wrap=on|off controls support for AES key wrapping (default=on)\n"
> >>      "                dea-key-wrap=on|off controls support for DEA key wrapping (default=on)\n"
> >>      "                suppress-vmdesc=on|off disables self-describing migration (default=off)\n"
> >> @@ -71,8 +70,6 @@ more than one accelerator specified, the next one is used if the previous one
> >>  fails to initialize.
> >>  @item kernel_irqchip=on|off
> >>  Controls in-kernel irqchip support for the chosen accelerator when available.
> >> -@item gfx_passthru=on|off
> >> -Enables IGD GFX passthrough support for the chosen machine when available.
> >>  @item vmport=on|off|auto
> >>  Enables emulation of VMWare IO port, for vmmouse etc. auto says to select the
> >>  value based on accel. For accel=xen the default is off otherwise the default
> >> @@ -118,8 +115,9 @@ Select CPU model (@code{-cpu help} for list and additional feature selection)
> >>  ETEXI
> >>
> >>  DEF("accel", HAS_ARG, QEMU_OPTION_accel,
> >> -    "-accel [accel=]accelerator[,thread=single|multi]\n"
> >> +    "-accel [accel=]accelerator[,prop[=value][,...]]\n"
> >>      "                select accelerator (kvm, xen, hax, hvf, whpx or tcg; use 'help' for a list)\n"
> >> +    "                igd-passthru=on|off (enable Xen integrated Intel graphics passthrough, default=off)\n"
> >>      "                tb-size=n (TCG translation block cache size)\n"
> >>      "                thread=single|multi (enable multi-threaded TCG)\n", QEMU_ARCH_ALL)
> >>  STEXI
> >> @@ -130,6 +128,9 @@ kvm, xen, hax, hvf, whpx or tcg can be available. By default, tcg is used. If th
> >>  more than one accelerator specified, the next one is used if the previous one
> >>  fails to initialize.
> >>  @table @option
> >> +@item igd-passthru=on|off
> >> +When Xen is in use, this option controls whether Intel integrated graphics
> >> +devices can be passed through to the guest (default=off)
> >>  @item tb-size=@var{n}
> >>  Controls the size (in MiB) of the TCG translation block cache.
> >>  @item thread=single|multi
> >> diff --git a/vl.c b/vl.c
> >> index 06c6ad9..7d8fed1 100644
> >> --- a/vl.c
> >> +++ b/vl.c
> >> @@ -1256,13 +1256,6 @@ static void configure_msg(QemuOpts *opts)
> >>  }
> >>
> >>
> >> -/* Now we still need this for compatibility with XEN. */
> >> -bool has_igd_gfx_passthru;
> >> -static void igd_gfx_passthru(void)
> >> -{
> >> -    has_igd_gfx_passthru = current_machine->igd_gfx_passthru;
> >> -}
> >> -
> >>  /***********************************************************/
> >>  /* USB devices */
> >>
> >> @@ -2645,6 +2638,10 @@ static int machine_set_property(void *opaque,
> >>      if (g_str_equal(qom_name, "accel")) {
> >>          return 0;
> >>      }
> >> +    if (g_str_equal(qom_name, "igd-passthru")) {
> >> +        object_register_sugar_prop(ACCEL_CLASS_NAME("xen"), qom_name, value);
> >> +        return 0;
> >> +    }
> >>
> >>      r = object_parse_property_opt(opaque, name, value, "type", errp);
> >>      g_free(qom_name);
> >> @@ -4449,9 +4446,6 @@ int main(int argc, char **argv, char **envp)
> >>              exit(1);
> >>      }
> >>
> >> -    /* Check if IGD GFX passthrough. */
> >> -    igd_gfx_passthru();
> >> -
> >>      /* init generic devices */
> >>      rom_set_order_override(FW_CFG_ORDER_OVERRIDE_DEVICE);
> >>      qemu_opts_foreach(qemu_find_opts("device"),
> >> --
> >> 1.8.3.1
> >>
> >>
> >>
> >
>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 13/16] xen: convert "-machine igd-passthru" to an accelerator property

[Thomas Huth]

On 13/11/2019 15.39, Paolo Bonzini wrote:
> The first machine property to fall is Xen's Intel integrated graphics
> passthrough.  The "-machine igd-passthru" option does not set anymore
> a property on the machine object, but desugars to a GlobalProperty on
> accelerator objects.
> 
> The setter is very simple, since the value ends up in a
> global variable, so this patch also provides an example before the more
> complicated cases that follow it.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/core/machine.c   | 20 --------------------
>  hw/xen/xen-common.c | 16 ++++++++++++++++
>  include/hw/boards.h |  1 -
>  qemu-options.hx     |  9 +++++----
>  vl.c                | 14 ++++----------
>  5 files changed, 25 insertions(+), 35 deletions(-)

Reviewed-by: Thomas Huth <thuth@redhat.com>

[PATCH 14/16] kvm: convert "-machine kvm_shadow_mem" to an accelerator property

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 accel/kvm/kvm-all.c | 43 +++++++++++++++++++++++++++++++++++++++++++
 hw/core/machine.c   | 39 ---------------------------------------
 include/hw/boards.h |  2 --
 qemu-options.hx     |  6 +++---
 target/i386/kvm.c   |  2 +-
 vl.c                |  4 ++++
 6 files changed, 51 insertions(+), 45 deletions(-)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 140b0bd..c016319 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -41,6 +41,7 @@
 #include "hw/irq.h"
 #include "sysemu/sev.h"
 #include "sysemu/balloon.h"
+#include "qapi/visitor.h"
 
 #include "hw/boards.h"
 
@@ -92,6 +93,7 @@ struct KVMState
     int max_nested_state_len;
     int many_ioeventfds;
     int intx_set_mask;
+    int kvm_shadow_mem;
     bool sync_mmu;
     bool manual_dirty_log_protect;
     /* The man page (and posix) say ioctl numbers are signed int, but
@@ -2922,6 +2924,40 @@ static bool kvm_accel_has_memory(MachineState *ms, AddressSpace *as,
     return false;
 }
 
+static void kvm_get_kvm_shadow_mem(Object *obj, Visitor *v,
+                                   const char *name, void *opaque,
+                                   Error **errp)
+{
+    KVMState *s = KVM_STATE(obj);
+    int64_t value = s->kvm_shadow_mem;
+
+    visit_type_int(v, name, &value, errp);
+}
+
+static void kvm_set_kvm_shadow_mem(Object *obj, Visitor *v,
+                                   const char *name, void *opaque,
+                                   Error **errp)
+{
+    KVMState *s = KVM_STATE(obj);
+    Error *error = NULL;
+    int64_t value;
+
+    visit_type_int(v, name, &value, &error);
+    if (error) {
+        error_propagate(errp, error);
+        return;
+    }
+
+    s->kvm_shadow_mem = value;
+}
+
+static void kvm_accel_instance_init(Object *obj)
+{
+    KVMState *s = KVM_STATE(obj);
+
+    s->kvm_shadow_mem = -1;
+}
+
 static void kvm_accel_class_init(ObjectClass *oc, void *data)
 {
     AccelClass *ac = ACCEL_CLASS(oc);
@@ -2929,11 +2965,18 @@ static void kvm_accel_class_init(ObjectClass *oc, void *data)
     ac->init_machine = kvm_init;
     ac->has_memory = kvm_accel_has_memory;
     ac->allowed = &kvm_allowed;
+
+    object_class_property_add(oc, "kvm-shadow-mem", "int",
+        kvm_get_kvm_shadow_mem, kvm_set_kvm_shadow_mem,
+        NULL, NULL, &error_abort);
+    object_class_property_set_description(oc, "kvm-shadow-mem",
+        "KVM shadow MMU size", &error_abort);
 }
 
 static const TypeInfo kvm_accel_type = {
     .name = TYPE_KVM_ACCEL,
     .parent = TYPE_ACCEL,
+    .instance_init = kvm_accel_instance_init,
     .class_init = kvm_accel_class_init,
     .instance_size = sizeof(KVMState),
 };
diff --git a/hw/core/machine.c b/hw/core/machine.c
index d7a0356..bfb5f6f 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -211,33 +211,6 @@ static void machine_set_kernel_irqchip(Object *obj, Visitor *v,
     }
 }
 
-static void machine_get_kvm_shadow_mem(Object *obj, Visitor *v,
-                                       const char *name, void *opaque,
-                                       Error **errp)
-{
-    MachineState *ms = MACHINE(obj);
-    int64_t value = ms->kvm_shadow_mem;
-
-    visit_type_int(v, name, &value, errp);
-}
-
-static void machine_set_kvm_shadow_mem(Object *obj, Visitor *v,
-                                       const char *name, void *opaque,
-                                       Error **errp)
-{
-    MachineState *ms = MACHINE(obj);
-    Error *error = NULL;
-    int64_t value;
-
-    visit_type_int(v, name, &value, &error);
-    if (error) {
-        error_propagate(errp, error);
-        return;
-    }
-
-    ms->kvm_shadow_mem = value;
-}
-
 static char *machine_get_kernel(Object *obj, Error **errp)
 {
     MachineState *ms = MACHINE(obj);
@@ -785,12 +758,6 @@ static void machine_class_init(ObjectClass *oc, void *data)
     object_class_property_set_description(oc, "kernel-irqchip",
         "Configure KVM in-kernel irqchip", &error_abort);
 
-    object_class_property_add(oc, "kvm-shadow-mem", "int",
-        machine_get_kvm_shadow_mem, machine_set_kvm_shadow_mem,
-        NULL, NULL, &error_abort);
-    object_class_property_set_description(oc, "kvm-shadow-mem",
-        "KVM shadow MMU size", &error_abort);
-
     object_class_property_add_str(oc, "kernel",
         machine_get_kernel, machine_set_kernel, &error_abort);
     object_class_property_set_description(oc, "kernel",
@@ -892,7 +859,6 @@ static void machine_initfn(Object *obj)
 
     ms->kernel_irqchip_allowed = true;
     ms->kernel_irqchip_split = mc->default_kernel_irqchip_split;
-    ms->kvm_shadow_mem = -1;
     ms->dump_guest_core = true;
     ms->mem_merge = true;
     ms->enable_graphics = true;
@@ -963,11 +929,6 @@ bool machine_kernel_irqchip_split(MachineState *machine)
     return machine->kernel_irqchip_split;
 }
 
-int machine_kvm_shadow_mem(MachineState *machine)
-{
-    return machine->kvm_shadow_mem;
-}
-
 int machine_phandle_start(MachineState *machine)
 {
     return machine->phandle_start;
diff --git a/include/hw/boards.h b/include/hw/boards.h
index cdcf481..46119eb 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -66,7 +66,6 @@ bool machine_usb(MachineState *machine);
 bool machine_kernel_irqchip_allowed(MachineState *machine);
 bool machine_kernel_irqchip_required(MachineState *machine);
 bool machine_kernel_irqchip_split(MachineState *machine);
-int machine_kvm_shadow_mem(MachineState *machine);
 int machine_phandle_start(MachineState *machine);
 bool machine_dump_guest_core(MachineState *machine);
 bool machine_mem_merge(MachineState *machine);
@@ -278,7 +277,6 @@ struct MachineState {
     bool kernel_irqchip_allowed;
     bool kernel_irqchip_required;
     bool kernel_irqchip_split;
-    int kvm_shadow_mem;
     char *dtb;
     char *dumpdtb;
     int phandle_start;
diff --git a/qemu-options.hx b/qemu-options.hx
index 5b43a83..ae56eca 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -34,7 +34,6 @@ DEF("machine", HAS_ARG, QEMU_OPTION_machine, \
     "                supported accelerators are kvm, xen, hax, hvf, whpx or tcg (default: tcg)\n"
     "                kernel_irqchip=on|off|split controls accelerated irqchip support (default=off)\n"
     "                vmport=on|off|auto controls emulation of vmport (default: auto)\n"
-    "                kvm_shadow_mem=size of KVM shadow MMU in bytes\n"
     "                dump-guest-core=on|off include guest memory in a core dump (default=on)\n"
     "                mem-merge=on|off controls memory merge support (default: on)\n"
     "                aes-key-wrap=on|off controls support for AES key wrapping (default=on)\n"
@@ -74,8 +73,6 @@ Controls in-kernel irqchip support for the chosen accelerator when available.
 Enables emulation of VMWare IO port, for vmmouse etc. auto says to select the
 value based on accel. For accel=xen the default is off otherwise the default
 is on.
-@item kvm_shadow_mem=size
-Defines the size of the KVM shadow MMU.
 @item dump-guest-core=on|off
 Include guest memory in a core dump. The default is on.
 @item mem-merge=on|off
@@ -118,6 +115,7 @@ DEF("accel", HAS_ARG, QEMU_OPTION_accel,
     "-accel [accel=]accelerator[,prop[=value][,...]]\n"
     "                select accelerator (kvm, xen, hax, hvf, whpx or tcg; use 'help' for a list)\n"
     "                igd-passthru=on|off (enable Xen integrated Intel graphics passthrough, default=off)\n"
+    "                kvm-shadow-mem=size of KVM shadow MMU in bytes\n"
     "                tb-size=n (TCG translation block cache size)\n"
     "                thread=single|multi (enable multi-threaded TCG)\n", QEMU_ARCH_ALL)
 STEXI
@@ -131,6 +129,8 @@ fails to initialize.
 @item igd-passthru=on|off
 When Xen is in use, this option controls whether Intel integrated graphics
 devices can be passed through to the guest (default=off)
+@item kvm-shadow-mem=size
+Defines the size of the KVM shadow MMU.
 @item tb-size=@var{n}
 Controls the size (in MiB) of the TCG translation block cache.
 @item thread=single|multi
diff --git a/target/i386/kvm.c b/target/i386/kvm.c
index bfd09bd..7dc05c6 100644
--- a/target/i386/kvm.c
+++ b/target/i386/kvm.c
@@ -2159,7 +2159,7 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
     }
     qemu_register_reset(kvm_unpoison_all, NULL);
 
-    shadow_mem = machine_kvm_shadow_mem(ms);
+    shadow_mem = object_property_get_int(OBJECT(s), "kvm-shadow-mem", &error_abort);
     if (shadow_mem != -1) {
         shadow_mem /= 4096;
         ret = kvm_vm_ioctl(s, KVM_SET_NR_MMU_PAGES, shadow_mem);
diff --git a/vl.c b/vl.c
index 7d8fed1..1d799b6 100644
--- a/vl.c
+++ b/vl.c
@@ -2642,6 +2642,10 @@ static int machine_set_property(void *opaque,
         object_register_sugar_prop(ACCEL_CLASS_NAME("xen"), qom_name, value);
         return 0;
     }
+    if (g_str_equal(qom_name, "kvm-shadow-mem")) {
+        object_register_sugar_prop(ACCEL_CLASS_NAME("kvm"), qom_name, value);
+        return 0;
+    }
 
     r = object_parse_property_opt(opaque, name, value, "type", errp);
     g_free(qom_name);
-- 
1.8.3.1

Re: [PATCH 14/16] kvm: convert "-machine kvm_shadow_mem" to an accelerator property

[Thomas Huth]

On 13/11/2019 15.39, Paolo Bonzini wrote:
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  accel/kvm/kvm-all.c | 43 +++++++++++++++++++++++++++++++++++++++++++
>  hw/core/machine.c   | 39 ---------------------------------------
>  include/hw/boards.h |  2 --
>  qemu-options.hx     |  6 +++---
>  target/i386/kvm.c   |  2 +-
>  vl.c                |  4 ++++
>  6 files changed, 51 insertions(+), 45 deletions(-)
> 
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index 140b0bd..c016319 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -41,6 +41,7 @@
>  #include "hw/irq.h"
>  #include "sysemu/sev.h"
>  #include "sysemu/balloon.h"
> +#include "qapi/visitor.h"
>  
>  #include "hw/boards.h"
>  
> @@ -92,6 +93,7 @@ struct KVMState
>      int max_nested_state_len;
>      int many_ioeventfds;
>      int intx_set_mask;
> +    int kvm_shadow_mem;

It's an "int" here...

>      bool sync_mmu;
>      bool manual_dirty_log_protect;
>      /* The man page (and posix) say ioctl numbers are signed int, but
> @@ -2922,6 +2924,40 @@ static bool kvm_accel_has_memory(MachineState *ms, AddressSpace *as,
>      return false;
>  }
>  
> +static void kvm_get_kvm_shadow_mem(Object *obj, Visitor *v,
> +                                   const char *name, void *opaque,
> +                                   Error **errp)
> +{
> +    KVMState *s = KVM_STATE(obj);
> +    int64_t value = s->kvm_shadow_mem;
> +
> +    visit_type_int(v, name, &value, errp);
> +}
> +
> +static void kvm_set_kvm_shadow_mem(Object *obj, Visitor *v,
> +                                   const char *name, void *opaque,
> +                                   Error **errp)
> +{
> +    KVMState *s = KVM_STATE(obj);
> +    Error *error = NULL;
> +    int64_t value;
> +
> +    visit_type_int(v, name, &value, &error);
> +    if (error) {
> +        error_propagate(errp, error);
> +        return;
> +    }
> +
> +    s->kvm_shadow_mem = value;
> +}

... but the get and set functions are using an int64_t internally? Looks
somewhat weird. Well, it has been like this in the old code already, but
maybe it's now a good point in time to clean this up?

 Thomas

[PATCH 15/16] kvm: introduce kvm_kernel_irqchip_* functions

[Paolo Bonzini]

The KVMState struct is opaque, so provide accessors for the fields
that will be moved from current_machine to the accelerator.  For now
they just forward to the machine object, but this will change.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 accel/kvm/kvm-all.c  | 23 +++++++++++++++++++----
 hw/ppc/e500.c        |  4 ++--
 hw/ppc/spapr_irq.c   | 10 +++++-----
 include/sysemu/kvm.h |  7 +++++--
 target/arm/kvm.c     |  8 ++++----
 target/i386/kvm.c    |  4 ++--
 target/mips/kvm.c    |  2 +-
 target/ppc/kvm.c     |  2 +-
 target/s390x/kvm.c   |  2 +-
 9 files changed, 40 insertions(+), 22 deletions(-)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index c016319..a1c3b78 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -1742,7 +1742,7 @@ void kvm_irqchip_set_qemuirq_gsi(KVMState *s, qemu_irq irq, int gsi)
     g_hash_table_insert(s->gsimap, irq, GINT_TO_POINTER(gsi));
 }
 
-static void kvm_irqchip_create(MachineState *machine, KVMState *s)
+static void kvm_irqchip_create(KVMState *s)
 {
     int ret;
 
@@ -1760,9 +1760,9 @@ static void kvm_irqchip_create(MachineState *machine, KVMState *s)
 
     /* First probe and see if there's a arch-specific hook to create the
      * in-kernel irqchip for us */
-    ret = kvm_arch_irqchip_create(machine, s);
+    ret = kvm_arch_irqchip_create(s);
     if (ret == 0) {
-        if (machine_kernel_irqchip_split(machine)) {
+        if (kvm_kernel_irqchip_split()) {
             perror("Split IRQ chip mode not supported.");
             exit(1);
         } else {
@@ -2034,7 +2034,7 @@ static int kvm_init(MachineState *ms)
     }
 
     if (machine_kernel_irqchip_allowed(ms)) {
-        kvm_irqchip_create(ms, s);
+        kvm_irqchip_create(s);
     }
 
     if (kvm_eventfds_allowed) {
@@ -2951,6 +2951,21 @@ static void kvm_set_kvm_shadow_mem(Object *obj, Visitor *v,
     s->kvm_shadow_mem = value;
 }
 
+bool kvm_kernel_irqchip_allowed(void)
+{
+    return machine_kernel_irqchip_allowed(current_machine);
+}
+
+bool kvm_kernel_irqchip_required(void)
+{
+    return machine_kernel_irqchip_required(current_machine);
+}
+
+bool kvm_kernel_irqchip_split(void)
+{
+    return machine_kernel_irqchip_split(current_machine);
+}
+
 static void kvm_accel_instance_init(Object *obj)
 {
     KVMState *s = KVM_STATE(obj);
diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index 91cd4c2..928efaa 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -801,10 +801,10 @@ static DeviceState *ppce500_init_mpic(PPCE500MachineState *pms,
     if (kvm_enabled()) {
         Error *err = NULL;
 
-        if (machine_kernel_irqchip_allowed(machine)) {
+        if (kvm_kernel_irqchip_allowed()) {
             dev = ppce500_init_mpic_kvm(pmc, irqs, &err);
         }
-        if (machine_kernel_irqchip_required(machine) && !dev) {
+        if (kvm_kernel_irqchip_required() && !dev) {
             error_reportf_err(err,
                               "kernel_irqchip requested but unavailable: ");
             exit(1);
diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
index b941608..8b66eb9 100644
--- a/hw/ppc/spapr_irq.c
+++ b/hw/ppc/spapr_irq.c
@@ -77,9 +77,9 @@ int spapr_irq_init_kvm(int (*fn)(SpaprInterruptController *, Error **),
     MachineState *machine = MACHINE(qdev_get_machine());
     Error *local_err = NULL;
 
-    if (kvm_enabled() && machine_kernel_irqchip_allowed(machine)) {
+    if (kvm_enabled() && kvm_kernel_irqchip_allowed()) {
         if (fn(intc, &local_err) < 0) {
-            if (machine_kernel_irqchip_required(machine)) {
+            if (kvm_kernel_irqchip_required()) {
                 error_prepend(&local_err,
                               "kernel_irqchip requested but unavailable: ");
                 error_propagate(errp, local_err);
@@ -184,7 +184,7 @@ static int spapr_irq_check(SpaprMachineState *spapr, Error **errp)
      */
     if (kvm_enabled() &&
         spapr->irq == &spapr_irq_dual &&
-        machine_kernel_irqchip_required(machine) &&
+        kvm_kernel_irqchip_required() &&
         xics_kvm_has_broken_disconnect(spapr)) {
         error_setg(errp, "KVM is too old to support ic-mode=dual,kernel-irqchip=on");
         return -1;
@@ -276,12 +276,12 @@ void spapr_irq_init(SpaprMachineState *spapr, Error **errp)
     MachineState *machine = MACHINE(spapr);
     SpaprMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
 
-    if (machine_kernel_irqchip_split(machine)) {
+    if (kvm_enabled() && kvm_kernel_irqchip_split()) {
         error_setg(errp, "kernel_irqchip split mode not supported on pseries");
         return;
     }
 
-    if (!kvm_enabled() && machine_kernel_irqchip_required(machine)) {
+    if (!kvm_enabled() && kvm_kernel_irqchip_required()) {
         error_setg(errp,
                    "kernel_irqchip requested but only available with KVM");
         return;
diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
index 9d14328..46a9f18 100644
--- a/include/sysemu/kvm.h
+++ b/include/sysemu/kvm.h
@@ -514,10 +514,13 @@ void kvm_pc_gsi_handler(void *opaque, int n, int level);
 void kvm_pc_setup_irq_routing(bool pci_enabled);
 void kvm_init_irq_routing(KVMState *s);
 
+bool kvm_kernel_irqchip_allowed(void);
+bool kvm_kernel_irqchip_required(void);
+bool kvm_kernel_irqchip_split(void);
+
 /**
  * kvm_arch_irqchip_create:
  * @KVMState: The KVMState pointer
- * @MachineState: The MachineState pointer
  *
  * Allow architectures to create an in-kernel irq chip themselves.
  *
@@ -525,7 +528,7 @@ void kvm_init_irq_routing(KVMState *s);
  *            0: irq chip was not created
  *          > 0: irq chip was created
  */
-int kvm_arch_irqchip_create(MachineState *ms, KVMState *s);
+int kvm_arch_irqchip_create(KVMState *s);
 
 /**
  * kvm_set_one_reg - set a register value in KVM via KVM_SET_ONE_REG ioctl
diff --git a/target/arm/kvm.c b/target/arm/kvm.c
index b473c63..d10383c 100644
--- a/target/arm/kvm.c
+++ b/target/arm/kvm.c
@@ -726,11 +726,11 @@ void kvm_arch_init_irq_routing(KVMState *s)
 {
 }
 
-int kvm_arch_irqchip_create(MachineState *ms, KVMState *s)
+int kvm_arch_irqchip_create(KVMState *s)
 {
-     if (machine_kernel_irqchip_split(ms)) {
-         perror("-machine kernel_irqchip=split is not supported on ARM.");
-         exit(1);
+    if (kvm_kernel_irqchip_split()) {
+        perror("-machine kernel_irqchip=split is not supported on ARM.");
+        exit(1);
     }
 
     /* If we can create the VGIC using the newer device control API, we
diff --git a/target/i386/kvm.c b/target/i386/kvm.c
index 7dc05c6..72bb215 100644
--- a/target/i386/kvm.c
+++ b/target/i386/kvm.c
@@ -4473,10 +4473,10 @@ void kvm_arch_init_irq_routing(KVMState *s)
     }
 }
 
-int kvm_arch_irqchip_create(MachineState *ms, KVMState *s)
+int kvm_arch_irqchip_create(KVMState *s)
 {
     int ret;
-    if (machine_kernel_irqchip_split(ms)) {
+    if (kvm_kernel_irqchip_split()) {
         ret = kvm_vm_enable_cap(s, KVM_CAP_SPLIT_IRQCHIP, 0, 24);
         if (ret) {
             error_report("Could not enable split irqchip mode: %s",
diff --git a/target/mips/kvm.c b/target/mips/kvm.c
index 578bc14..de3e26e 100644
--- a/target/mips/kvm.c
+++ b/target/mips/kvm.c
@@ -57,7 +57,7 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
     return 0;
 }
 
-int kvm_arch_irqchip_create(MachineState *ms, KVMState *s)
+int kvm_arch_irqchip_create(KVMState *s)
 {
     return 0;
 }
diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
index 7d2e896..02face1 100644
--- a/target/ppc/kvm.c
+++ b/target/ppc/kvm.c
@@ -152,7 +152,7 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
     return 0;
 }
 
-int kvm_arch_irqchip_create(MachineState *ms, KVMState *s)
+int kvm_arch_irqchip_create(KVMState *s)
 {
     return 0;
 }
diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index 0c9d14b..f3a742b 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -374,7 +374,7 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
     return 0;
 }
 
-int kvm_arch_irqchip_create(MachineState *ms, KVMState *s)
+int kvm_arch_irqchip_create(KVMState *s)
 {
     return 0;
 }
-- 
1.8.3.1

Re: [PATCH 15/16] kvm: introduce kvm_kernel_irqchip_* functions

[Thomas Huth]

On 13/11/2019 15.39, Paolo Bonzini wrote:
> The KVMState struct is opaque, so provide accessors for the fields
> that will be moved from current_machine to the accelerator.  For now
> they just forward to the machine object, but this will change.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  accel/kvm/kvm-all.c  | 23 +++++++++++++++++++----
>  hw/ppc/e500.c        |  4 ++--
>  hw/ppc/spapr_irq.c   | 10 +++++-----
>  include/sysemu/kvm.h |  7 +++++--
>  target/arm/kvm.c     |  8 ++++----
>  target/i386/kvm.c    |  4 ++--
>  target/mips/kvm.c    |  2 +-
>  target/ppc/kvm.c     |  2 +-
>  target/s390x/kvm.c   |  2 +-
>  9 files changed, 40 insertions(+), 22 deletions(-)
[...]
> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
> index b941608..8b66eb9 100644
> --- a/hw/ppc/spapr_irq.c
> +++ b/hw/ppc/spapr_irq.c
> @@ -77,9 +77,9 @@ int spapr_irq_init_kvm(int (*fn)(SpaprInterruptController *, Error **),
>      MachineState *machine = MACHINE(qdev_get_machine());
>      Error *local_err = NULL;
>  
> -    if (kvm_enabled() && machine_kernel_irqchip_allowed(machine)) {
> +    if (kvm_enabled() && kvm_kernel_irqchip_allowed()) {
>          if (fn(intc, &local_err) < 0) {
> -            if (machine_kernel_irqchip_required(machine)) {
> +            if (kvm_kernel_irqchip_required()) {
>                  error_prepend(&local_err,
>                                "kernel_irqchip requested but unavailable: ");
>                  error_propagate(errp, local_err);
> @@ -184,7 +184,7 @@ static int spapr_irq_check(SpaprMachineState *spapr, Error **errp)
>       */
>      if (kvm_enabled() &&
>          spapr->irq == &spapr_irq_dual &&
> -        machine_kernel_irqchip_required(machine) &&
> +        kvm_kernel_irqchip_required() &&
>          xics_kvm_has_broken_disconnect(spapr)) {
>          error_setg(errp, "KVM is too old to support ic-mode=dual,kernel-irqchip=on");
>          return -1;
> @@ -276,12 +276,12 @@ void spapr_irq_init(SpaprMachineState *spapr, Error **errp)
>      MachineState *machine = MACHINE(spapr);
>      SpaprMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
>  
> -    if (machine_kernel_irqchip_split(machine)) {
> +    if (kvm_enabled() && kvm_kernel_irqchip_split()) {
>          error_setg(errp, "kernel_irqchip split mode not supported on pseries");
>          return;
>      }

Any reason for the additional kvm_enabled() here? I think it should also
be ok without that?

Apart from that question, patch looks fine to me.

 Thomas

Re: [PATCH 15/16] kvm: introduce kvm_kernel_irqchip_* functions

[Paolo Bonzini]

On 19/11/19 12:56, Thomas Huth wrote:
>> -    if (machine_kernel_irqchip_split(machine)) {
>> +    if (kvm_enabled() && kvm_kernel_irqchip_split()) {
>>          error_setg(errp, "kernel_irqchip split mode not supported on pseries");
>>          return;
>>      }
> Any reason for the additional kvm_enabled() here? I think it should also
> be ok without that?
> 
> Apart from that question, patch looks fine to me.

It won't compile without that, kvm_kernel_irqchip_split() is defined in
accel/kvm/kvm-all.c.

Paolo

Re: [PATCH 15/16] kvm: introduce kvm_kernel_irqchip_* functions

[Thomas Huth]

On 19/11/2019 13.13, Paolo Bonzini wrote:
> On 19/11/19 12:56, Thomas Huth wrote:
>>> -    if (machine_kernel_irqchip_split(machine)) {
>>> +    if (kvm_enabled() && kvm_kernel_irqchip_split()) {
>>>          error_setg(errp, "kernel_irqchip split mode not supported on pseries");
>>>          return;
>>>      }
>> Any reason for the additional kvm_enabled() here? I think it should also
>> be ok without that?
>>
>> Apart from that question, patch looks fine to me.
> 
> It won't compile without that, kvm_kernel_irqchip_split() is defined in
> accel/kvm/kvm-all.c.

Oh, I wonder whether you need a stub for certain compilers? ... but if
all our supported compilers are fine with this, I'm fine with it, too.

Reviewed-by: Thomas Huth <thuth@redhat.com>

[PATCH 16/16] kvm: convert "-machine kernel_irqchip" to an accelerator property

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 accel/kvm/kvm-all.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++-----
 hw/core/machine.c   | 61 -----------------------------------------------------
 include/hw/boards.h |  3 ---
 qemu-options.hx     |  9 +++++---
 vl.c                |  3 ++-
 5 files changed, 62 insertions(+), 73 deletions(-)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index a1c3b78..a0bb4f5 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -42,6 +42,8 @@
 #include "sysemu/sev.h"
 #include "sysemu/balloon.h"
 #include "qapi/visitor.h"
+#include "qapi/qapi-types-common.h"
+#include "qapi/qapi-visit-common.h"
 
 #include "hw/boards.h"
 
@@ -94,6 +96,9 @@ struct KVMState
     int many_ioeventfds;
     int intx_set_mask;
     int kvm_shadow_mem;
+    bool kernel_irqchip_allowed;
+    bool kernel_irqchip_required;
+    bool kernel_irqchip_split;
     bool sync_mmu;
     bool manual_dirty_log_protect;
     /* The man page (and posix) say ioctl numbers are signed int, but
@@ -1762,7 +1767,7 @@ static void kvm_irqchip_create(KVMState *s)
      * in-kernel irqchip for us */
     ret = kvm_arch_irqchip_create(s);
     if (ret == 0) {
-        if (kvm_kernel_irqchip_split()) {
+        if (s->kernel_irqchip_split) {
             perror("Split IRQ chip mode not supported.");
             exit(1);
         } else {
@@ -2033,7 +2038,7 @@ static int kvm_init(MachineState *ms)
         goto err;
     }
 
-    if (machine_kernel_irqchip_allowed(ms)) {
+    if (s->kernel_irqchip_allowed) {
         kvm_irqchip_create(s);
     }
 
@@ -2951,19 +2956,57 @@ static void kvm_set_kvm_shadow_mem(Object *obj, Visitor *v,
     s->kvm_shadow_mem = value;
 }
 
+static void kvm_set_kernel_irqchip(Object *obj, Visitor *v,
+                                   const char *name, void *opaque,
+                                   Error **errp)
+{
+    Error *err = NULL;
+    KVMState *s = KVM_STATE(obj);
+    OnOffSplit mode;
+
+    visit_type_OnOffSplit(v, name, &mode, &err);
+    if (err) {
+        error_propagate(errp, err);
+        return;
+    } else {
+        switch (mode) {
+        case ON_OFF_SPLIT_ON:
+            s->kernel_irqchip_allowed = true;
+            s->kernel_irqchip_required = true;
+            s->kernel_irqchip_split = false;
+            break;
+        case ON_OFF_SPLIT_OFF:
+            s->kernel_irqchip_allowed = false;
+            s->kernel_irqchip_required = false;
+            s->kernel_irqchip_split = false;
+            break;
+        case ON_OFF_SPLIT_SPLIT:
+            s->kernel_irqchip_allowed = true;
+            s->kernel_irqchip_required = true;
+            s->kernel_irqchip_split = true;
+            break;
+        default:
+            /* The value was checked in visit_type_OnOffSplit() above. If
+             * we get here, then something is wrong in QEMU.
+             */
+            abort();
+        }
+    }
+}
+
 bool kvm_kernel_irqchip_allowed(void)
 {
-    return machine_kernel_irqchip_allowed(current_machine);
+    return kvm_state->kernel_irqchip_allowed;
 }
 
 bool kvm_kernel_irqchip_required(void)
 {
-    return machine_kernel_irqchip_required(current_machine);
+    return kvm_state->kernel_irqchip_required;
 }
 
 bool kvm_kernel_irqchip_split(void)
 {
-    return machine_kernel_irqchip_split(current_machine);
+    return kvm_state->kernel_irqchip_split;
 }
 
 static void kvm_accel_instance_init(Object *obj)
@@ -2981,6 +3024,12 @@ static void kvm_accel_class_init(ObjectClass *oc, void *data)
     ac->has_memory = kvm_accel_has_memory;
     ac->allowed = &kvm_allowed;
 
+    object_class_property_add(oc, "kernel-irqchip", "on|off|split",
+        NULL, kvm_set_kernel_irqchip,
+        NULL, NULL, &error_abort);
+    object_class_property_set_description(oc, "kernel-irqchip",
+        "Configure KVM in-kernel irqchip", &error_abort);
+
     object_class_property_add(oc, "kvm-shadow-mem", "int",
         kvm_get_kvm_shadow_mem, kvm_set_kvm_shadow_mem,
         NULL, NULL, &error_abort);
diff --git a/hw/core/machine.c b/hw/core/machine.c
index bfb5f6f..c1cad2c 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -173,44 +173,6 @@ GlobalProperty hw_compat_2_1[] = {
 };
 const size_t hw_compat_2_1_len = G_N_ELEMENTS(hw_compat_2_1);
 
-static void machine_set_kernel_irqchip(Object *obj, Visitor *v,
-                                       const char *name, void *opaque,
-                                       Error **errp)
-{
-    Error *err = NULL;
-    MachineState *ms = MACHINE(obj);
-    OnOffSplit mode;
-
-    visit_type_OnOffSplit(v, name, &mode, &err);
-    if (err) {
-        error_propagate(errp, err);
-        return;
-    } else {
-        switch (mode) {
-        case ON_OFF_SPLIT_ON:
-            ms->kernel_irqchip_allowed = true;
-            ms->kernel_irqchip_required = true;
-            ms->kernel_irqchip_split = false;
-            break;
-        case ON_OFF_SPLIT_OFF:
-            ms->kernel_irqchip_allowed = false;
-            ms->kernel_irqchip_required = false;
-            ms->kernel_irqchip_split = false;
-            break;
-        case ON_OFF_SPLIT_SPLIT:
-            ms->kernel_irqchip_allowed = true;
-            ms->kernel_irqchip_required = true;
-            ms->kernel_irqchip_split = true;
-            break;
-        default:
-            /* The value was checked in visit_type_OnOffSplit() above. If
-             * we get here, then something is wrong in QEMU.
-             */
-            abort();
-        }
-    }
-}
-
 static char *machine_get_kernel(Object *obj, Error **errp)
 {
     MachineState *ms = MACHINE(obj);
@@ -752,12 +714,6 @@ static void machine_class_init(ObjectClass *oc, void *data)
     mc->numa_mem_align_shift = 23;
     mc->numa_auto_assign_ram = numa_default_auto_assign_ram;
 
-    object_class_property_add(oc, "kernel-irqchip", "on|off|split",
-        NULL, machine_set_kernel_irqchip,
-        NULL, NULL, &error_abort);
-    object_class_property_set_description(oc, "kernel-irqchip",
-        "Configure KVM in-kernel irqchip", &error_abort);
-
     object_class_property_add_str(oc, "kernel",
         machine_get_kernel, machine_set_kernel, &error_abort);
     object_class_property_set_description(oc, "kernel",
@@ -857,8 +813,6 @@ static void machine_initfn(Object *obj)
     MachineState *ms = MACHINE(obj);
     MachineClass *mc = MACHINE_GET_CLASS(obj);
 
-    ms->kernel_irqchip_allowed = true;
-    ms->kernel_irqchip_split = mc->default_kernel_irqchip_split;
     ms->dump_guest_core = true;
     ms->mem_merge = true;
     ms->enable_graphics = true;
@@ -914,21 +868,6 @@ bool machine_usb(MachineState *machine)
     return machine->usb;
 }
 
-bool machine_kernel_irqchip_allowed(MachineState *machine)
-{
-    return machine->kernel_irqchip_allowed;
-}
-
-bool machine_kernel_irqchip_required(MachineState *machine)
-{
-    return machine->kernel_irqchip_required;
-}
-
-bool machine_kernel_irqchip_split(MachineState *machine)
-{
-    return machine->kernel_irqchip_split;
-}
-
 int machine_phandle_start(MachineState *machine)
 {
     return machine->phandle_start;
diff --git a/include/hw/boards.h b/include/hw/boards.h
index 46119eb..b42fedd 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -63,9 +63,6 @@ extern MachineState *current_machine;
 
 void machine_run_board_init(MachineState *machine);
 bool machine_usb(MachineState *machine);
-bool machine_kernel_irqchip_allowed(MachineState *machine);
-bool machine_kernel_irqchip_required(MachineState *machine);
-bool machine_kernel_irqchip_split(MachineState *machine);
 int machine_phandle_start(MachineState *machine);
 bool machine_dump_guest_core(MachineState *machine);
 bool machine_mem_merge(MachineState *machine);
diff --git a/qemu-options.hx b/qemu-options.hx
index ae56eca..7bb58a1 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -32,7 +32,6 @@ DEF("machine", HAS_ARG, QEMU_OPTION_machine, \
     "                selects emulated machine ('-machine help' for list)\n"
     "                property accel=accel1[:accel2[:...]] selects accelerator\n"
     "                supported accelerators are kvm, xen, hax, hvf, whpx or tcg (default: tcg)\n"
-    "                kernel_irqchip=on|off|split controls accelerated irqchip support (default=off)\n"
     "                vmport=on|off|auto controls emulation of vmport (default: auto)\n"
     "                dump-guest-core=on|off include guest memory in a core dump (default=on)\n"
     "                mem-merge=on|off controls memory merge support (default: on)\n"
@@ -67,8 +66,6 @@ This is used to enable an accelerator. Depending on the target architecture,
 kvm, xen, hax, hvf, whpx or tcg can be available. By default, tcg is used. If there is
 more than one accelerator specified, the next one is used if the previous one
 fails to initialize.
-@item kernel_irqchip=on|off
-Controls in-kernel irqchip support for the chosen accelerator when available.
 @item vmport=on|off|auto
 Enables emulation of VMWare IO port, for vmmouse etc. auto says to select the
 value based on accel. For accel=xen the default is off otherwise the default
@@ -115,6 +112,7 @@ DEF("accel", HAS_ARG, QEMU_OPTION_accel,
     "-accel [accel=]accelerator[,prop[=value][,...]]\n"
     "                select accelerator (kvm, xen, hax, hvf, whpx or tcg; use 'help' for a list)\n"
     "                igd-passthru=on|off (enable Xen integrated Intel graphics passthrough, default=off)\n"
+    "                kernel-irqchip=on|off|split controls accelerated irqchip support (default=on)\n"
     "                kvm-shadow-mem=size of KVM shadow MMU in bytes\n"
     "                tb-size=n (TCG translation block cache size)\n"
     "                thread=single|multi (enable multi-threaded TCG)\n", QEMU_ARCH_ALL)
@@ -129,6 +127,11 @@ fails to initialize.
 @item igd-passthru=on|off
 When Xen is in use, this option controls whether Intel integrated graphics
 devices can be passed through to the guest (default=off)
+@item kernel-irqchip=on|off|split
+Controls KVM in-kernel irqchip support.  The default is full acceleration of the
+interrupt controllers.  On x86, split irqchip reduces the kernel attack
+surface, at a performance cost for non-MSI interrupts.  Disabling the in-kernel
+irqchip completely is not recommended except for debugging purposes.
 @item kvm-shadow-mem=size
 Defines the size of the KVM shadow MMU.
 @item tb-size=@var{n}
diff --git a/vl.c b/vl.c
index 1d799b6..aaca7bf 100644
--- a/vl.c
+++ b/vl.c
@@ -2642,7 +2642,8 @@ static int machine_set_property(void *opaque,
         object_register_sugar_prop(ACCEL_CLASS_NAME("xen"), qom_name, value);
         return 0;
     }
-    if (g_str_equal(qom_name, "kvm-shadow-mem")) {
+    if (g_str_equal(qom_name, "kvm-shadow-mem") ||
+        g_str_equal(qom_name, "kernel-irqchip")) {
         object_register_sugar_prop(ACCEL_CLASS_NAME("kvm"), qom_name, value);
         return 0;
     }
-- 
1.8.3.1