[PATCH v2 00/11] hw/accel: Exit gracefully when accelerator is invalid

[PATCH v2 00/11] hw/accel: Exit gracefully when accelerator is invalid

[Philippe Mathieu-Daudé]

Hi,

This series aims to improve user experience by providing
a better error message when the user tries to enable KVM
on machines not supporting it.

Since v1:
- added missing x86 arch (Peter)
- consider all accelerators (Daniel and Peter)
- do not enable KVM on sbsa-ref (Leif)
- updated 'query-machines' (Daniel)
- new patch for XenPV

Supersedes: <20210219114428.1936109-1-philmd@redhat.com>

Philippe Mathieu-Daudé (11):
  accel/kvm: Check MachineClass kvm_type() return value
  hw/boards: Introduce machine_class_valid_for_accelerator()
  hw/core: Restrict 'query-machines' to those supported by current accel
  hw/arm: Restrit KVM to the virt & versal machines
  hw/mips: Restrict KVM to the malta & virt machines
  hw/ppc: Restrict KVM to various PPC machines
  hw/s390x: Explicit the s390-ccw-virtio machines support TCG and KVM
  hw/i386: Explicit x86 machines support all current accelerators
  hw/xenpv: Restrict Xen Para-virtualized machine to Xen accelerator
  hw/board: Only allow TCG accelerator by default
  softmmu/vl: Exit gracefully when accelerator is not supported

 include/hw/boards.h        | 27 ++++++++++++++++++++++++++-
 accel/kvm/kvm-all.c        |  6 ++++++
 hw/arm/virt.c              |  5 +++++
 hw/arm/xlnx-versal-virt.c  |  5 +++++
 hw/core/machine-qmp-cmds.c |  4 ++++
 hw/core/machine.c          | 26 ++++++++++++++++++++++++++
 hw/i386/x86.c              |  5 +++++
 hw/mips/loongson3_virt.c   |  5 +++++
 hw/mips/malta.c            |  5 +++++
 hw/ppc/e500plat.c          |  5 +++++
 hw/ppc/mac_newworld.c      |  6 ++++++
 hw/ppc/mac_oldworld.c      |  5 +++++
 hw/ppc/mpc8544ds.c         |  5 +++++
 hw/ppc/ppc440_bamboo.c     |  5 +++++
 hw/ppc/prep.c              |  5 +++++
 hw/ppc/sam460ex.c          |  5 +++++
 hw/ppc/spapr.c             |  5 +++++
 hw/s390x/s390-virtio-ccw.c |  5 +++++
 hw/xenpv/xen_machine_pv.c  |  5 +++++
 softmmu/vl.c               |  7 +++++++
 20 files changed, 145 insertions(+), 1 deletion(-)

-- 
2.26.2

[PATCH v2 01/11] accel/kvm: Check MachineClass kvm_type() return value

[Philippe Mathieu-Daudé]

MachineClass::kvm_type() can return -1 on failure.
Document it, and add a check in kvm_init().

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/hw/boards.h | 3 ++-
 accel/kvm/kvm-all.c | 6 ++++++
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/include/hw/boards.h b/include/hw/boards.h
index a46dfe5d1a6..68d3d10f6b0 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -127,7 +127,8 @@ typedef struct {
  *    implement and a stub device is required.
  * @kvm_type:
  *    Return the type of KVM corresponding to the kvm-type string option or
- *    computed based on other criteria such as the host kernel capabilities.
+ *    computed based on other criteria such as the host kernel capabilities
+ *    (which can't be negative), or -1 on error.
  * @numa_mem_supported:
  *    true if '--numa node.mem' option is supported and false otherwise
  * @smp_parse:
diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 84c943fcdb2..b069938d881 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -2057,6 +2057,12 @@ static int kvm_init(MachineState *ms)
                                                             "kvm-type",
                                                             &error_abort);
         type = mc->kvm_type(ms, kvm_type);
+        if (type < 0) {
+            ret = -EINVAL;
+            fprintf(stderr, "Failed to detect kvm-type for machine '%s'\n",
+                    mc->name);
+            goto err;
+        }
     }
 
     do {
-- 
2.26.2

Re: [PATCH v2 01/11] accel/kvm: Check MachineClass kvm_type() return value

[Cornelia Huck]

On Fri, 19 Feb 2021 18:38:37 +0100
Philippe Mathieu-Daudé <philmd@redhat.com> wrote:

> MachineClass::kvm_type() can return -1 on failure.
> Document it, and add a check in kvm_init().
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  include/hw/boards.h | 3 ++-
>  accel/kvm/kvm-all.c | 6 ++++++
>  2 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index a46dfe5d1a6..68d3d10f6b0 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -127,7 +127,8 @@ typedef struct {
>   *    implement and a stub device is required.
>   * @kvm_type:
>   *    Return the type of KVM corresponding to the kvm-type string option or
> - *    computed based on other criteria such as the host kernel capabilities.
> + *    computed based on other criteria such as the host kernel capabilities
> + *    (which can't be negative), or -1 on error.
>   * @numa_mem_supported:
>   *    true if '--numa node.mem' option is supported and false otherwise
>   * @smp_parse:
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index 84c943fcdb2..b069938d881 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -2057,6 +2057,12 @@ static int kvm_init(MachineState *ms)
>                                                              "kvm-type",
>                                                              &error_abort);
>          type = mc->kvm_type(ms, kvm_type);
> +        if (type < 0) {
> +            ret = -EINVAL;
> +            fprintf(stderr, "Failed to detect kvm-type for machine '%s'\n",
> +                    mc->name);
> +            goto err;
> +        }
>      }
>  
>      do {

No objection to this patch; but I'm wondering why some non-pseries
machines implement the kvm_type callback, when I see the kvm-type
property only for pseries? Am I holding my git grep wrong?

Re: [PATCH v2 01/11] accel/kvm: Check MachineClass kvm_type() return value

[Philippe Mathieu-Daudé]

On 2/22/21 6:24 PM, Cornelia Huck wrote:
> On Fri, 19 Feb 2021 18:38:37 +0100
> Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> 
>> MachineClass::kvm_type() can return -1 on failure.
>> Document it, and add a check in kvm_init().
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>  include/hw/boards.h | 3 ++-
>>  accel/kvm/kvm-all.c | 6 ++++++
>>  2 files changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/hw/boards.h b/include/hw/boards.h
>> index a46dfe5d1a6..68d3d10f6b0 100644
>> --- a/include/hw/boards.h
>> +++ b/include/hw/boards.h
>> @@ -127,7 +127,8 @@ typedef struct {
>>   *    implement and a stub device is required.
>>   * @kvm_type:
>>   *    Return the type of KVM corresponding to the kvm-type string option or
>> - *    computed based on other criteria such as the host kernel capabilities.
>> + *    computed based on other criteria such as the host kernel capabilities
>> + *    (which can't be negative), or -1 on error.
>>   * @numa_mem_supported:
>>   *    true if '--numa node.mem' option is supported and false otherwise
>>   * @smp_parse:
>> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
>> index 84c943fcdb2..b069938d881 100644
>> --- a/accel/kvm/kvm-all.c
>> +++ b/accel/kvm/kvm-all.c
>> @@ -2057,6 +2057,12 @@ static int kvm_init(MachineState *ms)
>>                                                              "kvm-type",
>>                                                              &error_abort);
>>          type = mc->kvm_type(ms, kvm_type);
>> +        if (type < 0) {
>> +            ret = -EINVAL;
>> +            fprintf(stderr, "Failed to detect kvm-type for machine '%s'\n",
>> +                    mc->name);
>> +            goto err;
>> +        }
>>      }
>>  
>>      do {
> 
> No objection to this patch; but I'm wondering why some non-pseries
> machines implement the kvm_type callback, when I see the kvm-type
> property only for pseries? Am I holding my git grep wrong?

Can it be what David commented here?
https://www.mail-archive.com/qemu-devel@nongnu.org/msg784508.html

Re: [PATCH v2 01/11] accel/kvm: Check MachineClass kvm_type() return value

[Cornelia Huck]

On Mon, 22 Feb 2021 18:41:07 +0100
Philippe Mathieu-Daudé <philmd@redhat.com> wrote:

> On 2/22/21 6:24 PM, Cornelia Huck wrote:
> > On Fri, 19 Feb 2021 18:38:37 +0100
> > Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> >   
> >> MachineClass::kvm_type() can return -1 on failure.
> >> Document it, and add a check in kvm_init().
> >>
> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> >> ---
> >>  include/hw/boards.h | 3 ++-
> >>  accel/kvm/kvm-all.c | 6 ++++++
> >>  2 files changed, 8 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/include/hw/boards.h b/include/hw/boards.h
> >> index a46dfe5d1a6..68d3d10f6b0 100644
> >> --- a/include/hw/boards.h
> >> +++ b/include/hw/boards.h
> >> @@ -127,7 +127,8 @@ typedef struct {
> >>   *    implement and a stub device is required.
> >>   * @kvm_type:
> >>   *    Return the type of KVM corresponding to the kvm-type string option or
> >> - *    computed based on other criteria such as the host kernel capabilities.
> >> + *    computed based on other criteria such as the host kernel capabilities
> >> + *    (which can't be negative), or -1 on error.
> >>   * @numa_mem_supported:
> >>   *    true if '--numa node.mem' option is supported and false otherwise
> >>   * @smp_parse:
> >> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> >> index 84c943fcdb2..b069938d881 100644
> >> --- a/accel/kvm/kvm-all.c
> >> +++ b/accel/kvm/kvm-all.c
> >> @@ -2057,6 +2057,12 @@ static int kvm_init(MachineState *ms)
> >>                                                              "kvm-type",
> >>                                                              &error_abort);
> >>          type = mc->kvm_type(ms, kvm_type);
> >> +        if (type < 0) {
> >> +            ret = -EINVAL;
> >> +            fprintf(stderr, "Failed to detect kvm-type for machine '%s'\n",
> >> +                    mc->name);
> >> +            goto err;
> >> +        }
> >>      }
> >>  
> >>      do {  
> > 
> > No objection to this patch; but I'm wondering why some non-pseries
> > machines implement the kvm_type callback, when I see the kvm-type
> > property only for pseries? Am I holding my git grep wrong?  
> 
> Can it be what David commented here?
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg784508.html
> 

Ok, I might be confused about the other ppc machines; but I'm wondering
about the kvm_type callback for mips and arm/virt. Maybe I'm just
confused by the whole mechanism?

Re: [PATCH v2 01/11] accel/kvm: Check MachineClass kvm_type() return value

[Philippe Mathieu-Daudé]

On 2/22/21 6:50 PM, Cornelia Huck wrote:
> On Mon, 22 Feb 2021 18:41:07 +0100
> Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> 
>> On 2/22/21 6:24 PM, Cornelia Huck wrote:
>>> On Fri, 19 Feb 2021 18:38:37 +0100
>>> Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>>>   
>>>> MachineClass::kvm_type() can return -1 on failure.
>>>> Document it, and add a check in kvm_init().
>>>>
>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>>> ---
>>>>  include/hw/boards.h | 3 ++-
>>>>  accel/kvm/kvm-all.c | 6 ++++++
>>>>  2 files changed, 8 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/include/hw/boards.h b/include/hw/boards.h
>>>> index a46dfe5d1a6..68d3d10f6b0 100644
>>>> --- a/include/hw/boards.h
>>>> +++ b/include/hw/boards.h
>>>> @@ -127,7 +127,8 @@ typedef struct {
>>>>   *    implement and a stub device is required.
>>>>   * @kvm_type:
>>>>   *    Return the type of KVM corresponding to the kvm-type string option or
>>>> - *    computed based on other criteria such as the host kernel capabilities.
>>>> + *    computed based on other criteria such as the host kernel capabilities
>>>> + *    (which can't be negative), or -1 on error.
>>>>   * @numa_mem_supported:
>>>>   *    true if '--numa node.mem' option is supported and false otherwise
>>>>   * @smp_parse:
>>>> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
>>>> index 84c943fcdb2..b069938d881 100644
>>>> --- a/accel/kvm/kvm-all.c
>>>> +++ b/accel/kvm/kvm-all.c
>>>> @@ -2057,6 +2057,12 @@ static int kvm_init(MachineState *ms)
>>>>                                                              "kvm-type",
>>>>                                                              &error_abort);
>>>>          type = mc->kvm_type(ms, kvm_type);
>>>> +        if (type < 0) {
>>>> +            ret = -EINVAL;
>>>> +            fprintf(stderr, "Failed to detect kvm-type for machine '%s'\n",
>>>> +                    mc->name);
>>>> +            goto err;
>>>> +        }
>>>>      }
>>>>  
>>>>      do {  
>>>
>>> No objection to this patch; but I'm wondering why some non-pseries
>>> machines implement the kvm_type callback, when I see the kvm-type
>>> property only for pseries? Am I holding my git grep wrong?  
>>
>> Can it be what David commented here?
>> https://www.mail-archive.com/qemu-devel@nongnu.org/msg784508.html
>>
> 
> Ok, I might be confused about the other ppc machines; but I'm wondering
> about the kvm_type callback for mips and arm/virt. Maybe I'm just
> confused by the whole mechanism?

For MIPS see https://www.linux-kvm.org/images/f/f2/01x08a-MIPS.pdf
and Jiaxun comment here:
https://lore.kernel.org/linux-mips/a2a2cfe3-5618-43b1-a6a4-cc768fc1b9fb@www.fastmail.com/

TE KVM: Trap-and-Emul guest kernel
VZ KVM: HW Virtualized

For "the whole mechanism" I'll defer to Paolo =)

Re: [PATCH v2 01/11] accel/kvm: Check MachineClass kvm_type() return value

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3117 bytes --]

On Mon, Feb 22, 2021 at 06:50:44PM +0100, Cornelia Huck wrote:
> On Mon, 22 Feb 2021 18:41:07 +0100
> Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> 
> > On 2/22/21 6:24 PM, Cornelia Huck wrote:
> > > On Fri, 19 Feb 2021 18:38:37 +0100
> > > Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> > >   
> > >> MachineClass::kvm_type() can return -1 on failure.
> > >> Document it, and add a check in kvm_init().
> > >>
> > >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> > >> ---
> > >>  include/hw/boards.h | 3 ++-
> > >>  accel/kvm/kvm-all.c | 6 ++++++
> > >>  2 files changed, 8 insertions(+), 1 deletion(-)
> > >>
> > >> diff --git a/include/hw/boards.h b/include/hw/boards.h
> > >> index a46dfe5d1a6..68d3d10f6b0 100644
> > >> --- a/include/hw/boards.h
> > >> +++ b/include/hw/boards.h
> > >> @@ -127,7 +127,8 @@ typedef struct {
> > >>   *    implement and a stub device is required.
> > >>   * @kvm_type:
> > >>   *    Return the type of KVM corresponding to the kvm-type string option or
> > >> - *    computed based on other criteria such as the host kernel capabilities.
> > >> + *    computed based on other criteria such as the host kernel capabilities
> > >> + *    (which can't be negative), or -1 on error.
> > >>   * @numa_mem_supported:
> > >>   *    true if '--numa node.mem' option is supported and false otherwise
> > >>   * @smp_parse:
> > >> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> > >> index 84c943fcdb2..b069938d881 100644
> > >> --- a/accel/kvm/kvm-all.c
> > >> +++ b/accel/kvm/kvm-all.c
> > >> @@ -2057,6 +2057,12 @@ static int kvm_init(MachineState *ms)
> > >>                                                              "kvm-type",
> > >>                                                              &error_abort);
> > >>          type = mc->kvm_type(ms, kvm_type);
> > >> +        if (type < 0) {
> > >> +            ret = -EINVAL;
> > >> +            fprintf(stderr, "Failed to detect kvm-type for machine '%s'\n",
> > >> +                    mc->name);
> > >> +            goto err;
> > >> +        }
> > >>      }
> > >>  
> > >>      do {  
> > > 
> > > No objection to this patch; but I'm wondering why some non-pseries
> > > machines implement the kvm_type callback, when I see the kvm-type
> > > property only for pseries? Am I holding my git grep wrong?  
> > 
> > Can it be what David commented here?
> > https://www.mail-archive.com/qemu-devel@nongnu.org/msg784508.html
> > 
> 
> Ok, I might be confused about the other ppc machines; but I'm wondering
> about the kvm_type callback for mips and arm/virt. Maybe I'm just
> confused by the whole mechanism?

For ppc at least, not sure about in general, pseries is the only
machine type that can possibly work under more than one KVM flavour
(HV or PR).  So, it's the only one where it's actually useful to be
able to configure this.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 01/11] accel/kvm: Check MachineClass kvm_type() return value

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3665 bytes --]

On Tue, Feb 23, 2021 at 10:33:55AM +1100, David Gibson wrote:
> On Mon, Feb 22, 2021 at 06:50:44PM +0100, Cornelia Huck wrote:
> > On Mon, 22 Feb 2021 18:41:07 +0100
> > Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> > 
> > > On 2/22/21 6:24 PM, Cornelia Huck wrote:
> > > > On Fri, 19 Feb 2021 18:38:37 +0100
> > > > Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> > > >   
> > > >> MachineClass::kvm_type() can return -1 on failure.
> > > >> Document it, and add a check in kvm_init().
> > > >>
> > > >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> > > >> ---
> > > >>  include/hw/boards.h | 3 ++-
> > > >>  accel/kvm/kvm-all.c | 6 ++++++
> > > >>  2 files changed, 8 insertions(+), 1 deletion(-)
> > > >>
> > > >> diff --git a/include/hw/boards.h b/include/hw/boards.h
> > > >> index a46dfe5d1a6..68d3d10f6b0 100644
> > > >> --- a/include/hw/boards.h
> > > >> +++ b/include/hw/boards.h
> > > >> @@ -127,7 +127,8 @@ typedef struct {
> > > >>   *    implement and a stub device is required.
> > > >>   * @kvm_type:
> > > >>   *    Return the type of KVM corresponding to the kvm-type string option or
> > > >> - *    computed based on other criteria such as the host kernel capabilities.
> > > >> + *    computed based on other criteria such as the host kernel capabilities
> > > >> + *    (which can't be negative), or -1 on error.
> > > >>   * @numa_mem_supported:
> > > >>   *    true if '--numa node.mem' option is supported and false otherwise
> > > >>   * @smp_parse:
> > > >> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> > > >> index 84c943fcdb2..b069938d881 100644
> > > >> --- a/accel/kvm/kvm-all.c
> > > >> +++ b/accel/kvm/kvm-all.c
> > > >> @@ -2057,6 +2057,12 @@ static int kvm_init(MachineState *ms)
> > > >>                                                              "kvm-type",
> > > >>                                                              &error_abort);
> > > >>          type = mc->kvm_type(ms, kvm_type);
> > > >> +        if (type < 0) {
> > > >> +            ret = -EINVAL;
> > > >> +            fprintf(stderr, "Failed to detect kvm-type for machine '%s'\n",
> > > >> +                    mc->name);
> > > >> +            goto err;
> > > >> +        }
> > > >>      }
> > > >>  
> > > >>      do {  
> > > > 
> > > > No objection to this patch; but I'm wondering why some non-pseries
> > > > machines implement the kvm_type callback, when I see the kvm-type
> > > > property only for pseries? Am I holding my git grep wrong?  
> > > 
> > > Can it be what David commented here?
> > > https://www.mail-archive.com/qemu-devel@nongnu.org/msg784508.html
> > > 
> > 
> > Ok, I might be confused about the other ppc machines; but I'm wondering
> > about the kvm_type callback for mips and arm/virt. Maybe I'm just
> > confused by the whole mechanism?
> 
> For ppc at least, not sure about in general, pseries is the only
> machine type that can possibly work under more than one KVM flavour
> (HV or PR).  So, it's the only one where it's actually useful to be
> able to configure this.

Wait... I'm not sure that's true.  At least theoretically, some of the
Book3E platforms could work with either PR or the Book3E specific
KVM.  Not sure if KVM PR supports all the BookE instructions it would
need to in practice.

Possibly pseries is just the platform where there's been enough people
interested in setting the KVM flavour so far.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 01/11] accel/kvm: Check MachineClass kvm_type() return value

[Cornelia Huck]

[-- Attachment #1: Type: text/plain, Size: 4030 bytes --]

On Tue, 23 Feb 2021 10:37:01 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Tue, Feb 23, 2021 at 10:33:55AM +1100, David Gibson wrote:
> > On Mon, Feb 22, 2021 at 06:50:44PM +0100, Cornelia Huck wrote:  
> > > On Mon, 22 Feb 2021 18:41:07 +0100
> > > Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> > >   
> > > > On 2/22/21 6:24 PM, Cornelia Huck wrote:  
> > > > > On Fri, 19 Feb 2021 18:38:37 +0100
> > > > > Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> > > > >     
> > > > >> MachineClass::kvm_type() can return -1 on failure.
> > > > >> Document it, and add a check in kvm_init().
> > > > >>
> > > > >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> > > > >> ---
> > > > >>  include/hw/boards.h | 3 ++-
> > > > >>  accel/kvm/kvm-all.c | 6 ++++++
> > > > >>  2 files changed, 8 insertions(+), 1 deletion(-)
> > > > >>
> > > > >> diff --git a/include/hw/boards.h b/include/hw/boards.h
> > > > >> index a46dfe5d1a6..68d3d10f6b0 100644
> > > > >> --- a/include/hw/boards.h
> > > > >> +++ b/include/hw/boards.h
> > > > >> @@ -127,7 +127,8 @@ typedef struct {
> > > > >>   *    implement and a stub device is required.
> > > > >>   * @kvm_type:
> > > > >>   *    Return the type of KVM corresponding to the kvm-type string option or
> > > > >> - *    computed based on other criteria such as the host kernel capabilities.
> > > > >> + *    computed based on other criteria such as the host kernel capabilities
> > > > >> + *    (which can't be negative), or -1 on error.
> > > > >>   * @numa_mem_supported:
> > > > >>   *    true if '--numa node.mem' option is supported and false otherwise
> > > > >>   * @smp_parse:
> > > > >> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> > > > >> index 84c943fcdb2..b069938d881 100644
> > > > >> --- a/accel/kvm/kvm-all.c
> > > > >> +++ b/accel/kvm/kvm-all.c
> > > > >> @@ -2057,6 +2057,12 @@ static int kvm_init(MachineState *ms)
> > > > >>                                                              "kvm-type",
> > > > >>                                                              &error_abort);
> > > > >>          type = mc->kvm_type(ms, kvm_type);
> > > > >> +        if (type < 0) {
> > > > >> +            ret = -EINVAL;
> > > > >> +            fprintf(stderr, "Failed to detect kvm-type for machine '%s'\n",
> > > > >> +                    mc->name);
> > > > >> +            goto err;
> > > > >> +        }
> > > > >>      }
> > > > >>  
> > > > >>      do {    
> > > > > 
> > > > > No objection to this patch; but I'm wondering why some non-pseries
> > > > > machines implement the kvm_type callback, when I see the kvm-type
> > > > > property only for pseries? Am I holding my git grep wrong?    
> > > > 
> > > > Can it be what David commented here?
> > > > https://www.mail-archive.com/qemu-devel@nongnu.org/msg784508.html
> > > >   
> > > 
> > > Ok, I might be confused about the other ppc machines; but I'm wondering
> > > about the kvm_type callback for mips and arm/virt. Maybe I'm just
> > > confused by the whole mechanism?  
> > 
> > For ppc at least, not sure about in general, pseries is the only
> > machine type that can possibly work under more than one KVM flavour
> > (HV or PR).  So, it's the only one where it's actually useful to be
> > able to configure this.  
> 
> Wait... I'm not sure that's true.  At least theoretically, some of the
> Book3E platforms could work with either PR or the Book3E specific
> KVM.  Not sure if KVM PR supports all the BookE instructions it would
> need to in practice.
> 
> Possibly pseries is just the platform where there's been enough people
> interested in setting the KVM flavour so far.

If I'm not utterly confused by the code, it seems the pseries machines
are the only ones where you can actually get to an invocation of
->kvm_type(): You need to have a 'kvm-type' machine property, and
AFAICS only the pseries machine has that.

(Or is something hiding behind some macro magic?)

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 01/11] accel/kvm: Check MachineClass kvm_type() return value

[Philippe Mathieu-Daudé]

On 2/23/21 11:36 AM, Cornelia Huck wrote:
> On Tue, 23 Feb 2021 10:37:01 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
>> On Tue, Feb 23, 2021 at 10:33:55AM +1100, David Gibson wrote:
>>> On Mon, Feb 22, 2021 at 06:50:44PM +0100, Cornelia Huck wrote:  
>>>> On Mon, 22 Feb 2021 18:41:07 +0100
>>>> Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>>>>   
>>>>> On 2/22/21 6:24 PM, Cornelia Huck wrote:  
>>>>>> On Fri, 19 Feb 2021 18:38:37 +0100
>>>>>> Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>>>>>>     
>>>>>>> MachineClass::kvm_type() can return -1 on failure.
>>>>>>> Document it, and add a check in kvm_init().
>>>>>>>
>>>>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>>>>>> ---
>>>>>>>  include/hw/boards.h | 3 ++-
>>>>>>>  accel/kvm/kvm-all.c | 6 ++++++
>>>>>>>  2 files changed, 8 insertions(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/include/hw/boards.h b/include/hw/boards.h
>>>>>>> index a46dfe5d1a6..68d3d10f6b0 100644
>>>>>>> --- a/include/hw/boards.h
>>>>>>> +++ b/include/hw/boards.h
>>>>>>> @@ -127,7 +127,8 @@ typedef struct {
>>>>>>>   *    implement and a stub device is required.
>>>>>>>   * @kvm_type:
>>>>>>>   *    Return the type of KVM corresponding to the kvm-type string option or
>>>>>>> - *    computed based on other criteria such as the host kernel capabilities.
>>>>>>> + *    computed based on other criteria such as the host kernel capabilities
>>>>>>> + *    (which can't be negative), or -1 on error.
>>>>>>>   * @numa_mem_supported:
>>>>>>>   *    true if '--numa node.mem' option is supported and false otherwise
>>>>>>>   * @smp_parse:
>>>>>>> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
>>>>>>> index 84c943fcdb2..b069938d881 100644
>>>>>>> --- a/accel/kvm/kvm-all.c
>>>>>>> +++ b/accel/kvm/kvm-all.c
>>>>>>> @@ -2057,6 +2057,12 @@ static int kvm_init(MachineState *ms)
>>>>>>>                                                              "kvm-type",
>>>>>>>                                                              &error_abort);
>>>>>>>          type = mc->kvm_type(ms, kvm_type);
>>>>>>> +        if (type < 0) {
>>>>>>> +            ret = -EINVAL;
>>>>>>> +            fprintf(stderr, "Failed to detect kvm-type for machine '%s'\n",
>>>>>>> +                    mc->name);
>>>>>>> +            goto err;
>>>>>>> +        }
>>>>>>>      }
>>>>>>>  
>>>>>>>      do {    
>>>>>>
>>>>>> No objection to this patch; but I'm wondering why some non-pseries
>>>>>> machines implement the kvm_type callback, when I see the kvm-type
>>>>>> property only for pseries? Am I holding my git grep wrong?    
>>>>>
>>>>> Can it be what David commented here?
>>>>> https://www.mail-archive.com/qemu-devel@nongnu.org/msg784508.html
>>>>>   
>>>>
>>>> Ok, I might be confused about the other ppc machines; but I'm wondering
>>>> about the kvm_type callback for mips and arm/virt. Maybe I'm just
>>>> confused by the whole mechanism?  
>>>
>>> For ppc at least, not sure about in general, pseries is the only
>>> machine type that can possibly work under more than one KVM flavour
>>> (HV or PR).  So, it's the only one where it's actually useful to be
>>> able to configure this.  
>>
>> Wait... I'm not sure that's true.  At least theoretically, some of the
>> Book3E platforms could work with either PR or the Book3E specific
>> KVM.  Not sure if KVM PR supports all the BookE instructions it would
>> need to in practice.
>>
>> Possibly pseries is just the platform where there's been enough people
>> interested in setting the KVM flavour so far.
> 
> If I'm not utterly confused by the code, it seems the pseries machines
> are the only ones where you can actually get to an invocation of
> ->kvm_type(): You need to have a 'kvm-type' machine property, and
> AFAICS only the pseries machine has that.

OMG you are right... This changed in commit f2ce39b4f06
("vl: make qemu_get_machine_opts static"):

@@ -2069,13 +2068,11 @@ static int kvm_init(MachineState *ms)
     }
     s->as = g_new0(struct KVMAs, s->nr_as);

-    kvm_type = qemu_opt_get(qemu_get_machine_opts(), "kvm-type");
-    if (mc->kvm_type) {
+    if (object_property_find(OBJECT(current_machine), "kvm-type")) {
+        g_autofree char *kvm_type =
object_property_get_str(OBJECT(current_machine),
+                                                            "kvm-type",
+                                                            &error_abort);
         type = mc->kvm_type(ms, kvm_type);
-    } else if (kvm_type) {
-        ret = -EINVAL;
-        fprintf(stderr, "Invalid argument kvm-type=%s\n", kvm_type);
-        goto err;
     }

Paolo, is that expected?

So these callbacks are dead code:
hw/arm/virt.c:2585:    mc->kvm_type = virt_kvm_type;
hw/mips/loongson3_virt.c:625:    mc->kvm_type = mips_kvm_type;
hw/ppc/mac_newworld.c:598:    mc->kvm_type = core99_kvm_type;
hw/ppc/mac_oldworld.c:447:    mc->kvm_type = heathrow_kvm_type;

> 
> (Or is something hiding behind some macro magic?)
> 

[PATCH v2 02/11] hw/boards: Introduce machine_class_valid_for_accelerator()

[Philippe Mathieu-Daudé]

Introduce the valid_accelerators[] field to express the list
of valid accelators a machine can use, and add the
machine_class_valid_for_current_accelerator() and
machine_class_valid_for_accelerator() methods.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/hw/boards.h | 24 ++++++++++++++++++++++++
 hw/core/machine.c   | 26 ++++++++++++++++++++++++++
 2 files changed, 50 insertions(+)

diff --git a/include/hw/boards.h b/include/hw/boards.h
index 68d3d10f6b0..4d08bc12093 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -36,6 +36,24 @@ void machine_set_cpu_numa_node(MachineState *machine,
                                const CpuInstanceProperties *props,
                                Error **errp);
 
+/**
+ * machine_class_valid_for_accelerator:
+ * @mc: the machine class
+ * @acc_name: accelerator name
+ *
+ * Returns %true if the accelerator is valid for the machine, %false
+ * otherwise. See #MachineClass.valid_accelerators.
+ */
+bool machine_class_valid_for_accelerator(MachineClass *mc, const char *acc_name);
+/**
+ * machine_class_valid_for_current_accelerator:
+ * @mc: the machine class
+ *
+ * Returns %true if the accelerator is valid for the current machine,
+ * %false otherwise. See #MachineClass.valid_accelerators.
+ */
+bool machine_class_valid_for_current_accelerator(MachineClass *mc);
+
 void machine_class_allow_dynamic_sysbus_dev(MachineClass *mc, const char *type);
 /*
  * Checks that backend isn't used, preps it for exclusive usage and
@@ -125,6 +143,11 @@ typedef struct {
  *    should instead use "unimplemented-device" for all memory ranges where
  *    the guest will attempt to probe for a device that QEMU doesn't
  *    implement and a stub device is required.
+ * @valid_accelerators:
+ *    If this machine supports a specific set of virtualization accelerators,
+ *    this contains a NULL-terminated list of the accelerators that can be
+ *    used. If this field is not set, any accelerator is valid. The QTest
+ *    accelerator is always valid.
  * @kvm_type:
  *    Return the type of KVM corresponding to the kvm-type string option or
  *    computed based on other criteria such as the host kernel capabilities
@@ -166,6 +189,7 @@ struct MachineClass {
     const char *alias;
     const char *desc;
     const char *deprecation_reason;
+    const char *const *valid_accelerators;
 
     void (*init)(MachineState *state);
     void (*reset)(MachineState *state);
diff --git a/hw/core/machine.c b/hw/core/machine.c
index 970046f4388..c42d8e382b1 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -518,6 +518,32 @@ static void machine_set_nvdimm_persistence(Object *obj, const char *value,
     nvdimms_state->persistence_string = g_strdup(value);
 }
 
+bool machine_class_valid_for_accelerator(MachineClass *mc, const char *acc_name)
+{
+    const char *const *name = mc->valid_accelerators;
+
+    if (!name) {
+        return true;
+    }
+    if (strcmp(acc_name, "qtest") == 0) {
+        return true;
+    }
+
+    for (unsigned i = 0; name[i]; i++) {
+        if (strcasecmp(acc_name, name[i]) == 0) {
+            return true;
+        }
+    }
+    return false;
+}
+
+bool machine_class_valid_for_current_accelerator(MachineClass *mc)
+{
+    AccelClass *ac = ACCEL_GET_CLASS(current_accel());
+
+    return machine_class_valid_for_accelerator(mc, ac->name);
+}
+
 void machine_class_allow_dynamic_sysbus_dev(MachineClass *mc, const char *type)
 {
     QAPI_LIST_PREPEND(mc->allowed_dynamic_sysbus_devices, g_strdup(type));
-- 
2.26.2

Re: [PATCH v2 02/11] hw/boards: Introduce machine_class_valid_for_accelerator()

[Cornelia Huck]

On Fri, 19 Feb 2021 18:38:38 +0100
Philippe Mathieu-Daudé <philmd@redhat.com> wrote:

> Introduce the valid_accelerators[] field to express the list
> of valid accelators a machine can use, and add the
> machine_class_valid_for_current_accelerator() and
> machine_class_valid_for_accelerator() methods.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  include/hw/boards.h | 24 ++++++++++++++++++++++++
>  hw/core/machine.c   | 26 ++++++++++++++++++++++++++
>  2 files changed, 50 insertions(+)
> 
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 68d3d10f6b0..4d08bc12093 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -36,6 +36,24 @@ void machine_set_cpu_numa_node(MachineState *machine,
>                                 const CpuInstanceProperties *props,
>                                 Error **errp);
>  
> +/**
> + * machine_class_valid_for_accelerator:
> + * @mc: the machine class
> + * @acc_name: accelerator name
> + *
> + * Returns %true if the accelerator is valid for the machine, %false
> + * otherwise. See #MachineClass.valid_accelerators.

Naming confusion: is the machine class valid for the accelerator, or
the accelerator valid for the machine class? Or either? :)

> + */
> +bool machine_class_valid_for_accelerator(MachineClass *mc, const char *acc_name);
> +/**
> + * machine_class_valid_for_current_accelerator:
> + * @mc: the machine class
> + *
> + * Returns %true if the accelerator is valid for the current machine,
> + * %false otherwise. See #MachineClass.valid_accelerators.

Same here: current accelerator vs. current machine.

> + */
> +bool machine_class_valid_for_current_accelerator(MachineClass *mc);
> +
>  void machine_class_allow_dynamic_sysbus_dev(MachineClass *mc, const char *type);
>  /*
>   * Checks that backend isn't used, preps it for exclusive usage and
> @@ -125,6 +143,11 @@ typedef struct {
>   *    should instead use "unimplemented-device" for all memory ranges where
>   *    the guest will attempt to probe for a device that QEMU doesn't
>   *    implement and a stub device is required.
> + * @valid_accelerators:
> + *    If this machine supports a specific set of virtualization accelerators,
> + *    this contains a NULL-terminated list of the accelerators that can be
> + *    used. If this field is not set, any accelerator is valid. The QTest
> + *    accelerator is always valid.
>   * @kvm_type:
>   *    Return the type of KVM corresponding to the kvm-type string option or
>   *    computed based on other criteria such as the host kernel capabilities
> @@ -166,6 +189,7 @@ struct MachineClass {
>      const char *alias;
>      const char *desc;
>      const char *deprecation_reason;
> +    const char *const *valid_accelerators;
>  
>      void (*init)(MachineState *state);
>      void (*reset)(MachineState *state);
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 970046f4388..c42d8e382b1 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -518,6 +518,32 @@ static void machine_set_nvdimm_persistence(Object *obj, const char *value,
>      nvdimms_state->persistence_string = g_strdup(value);
>  }
>  
> +bool machine_class_valid_for_accelerator(MachineClass *mc, const char *acc_name)
> +{
> +    const char *const *name = mc->valid_accelerators;
> +
> +    if (!name) {
> +        return true;
> +    }
> +    if (strcmp(acc_name, "qtest") == 0) {
> +        return true;
> +    }
> +
> +    for (unsigned i = 0; name[i]; i++) {
> +        if (strcasecmp(acc_name, name[i]) == 0) {
> +            return true;
> +        }
> +    }
> +    return false;
> +}
> +
> +bool machine_class_valid_for_current_accelerator(MachineClass *mc)
> +{
> +    AccelClass *ac = ACCEL_GET_CLASS(current_accel());
> +
> +    return machine_class_valid_for_accelerator(mc, ac->name);
> +}

The implementation of the function tests for the current accelerator,
so I think you need to tweak the description above?

> +
>  void machine_class_allow_dynamic_sysbus_dev(MachineClass *mc, const char *type)
>  {
>      QAPI_LIST_PREPEND(mc->allowed_dynamic_sysbus_devices, g_strdup(type));

Re: [PATCH v2 02/11] hw/boards: Introduce machine_class_valid_for_accelerator()

[Philippe Mathieu-Daudé]

On 2/22/21 6:34 PM, Cornelia Huck wrote:
> On Fri, 19 Feb 2021 18:38:38 +0100
> Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> 
>> Introduce the valid_accelerators[] field to express the list
>> of valid accelators a machine can use, and add the
>> machine_class_valid_for_current_accelerator() and
>> machine_class_valid_for_accelerator() methods.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>  include/hw/boards.h | 24 ++++++++++++++++++++++++
>>  hw/core/machine.c   | 26 ++++++++++++++++++++++++++
>>  2 files changed, 50 insertions(+)
>>
>> diff --git a/include/hw/boards.h b/include/hw/boards.h
>> index 68d3d10f6b0..4d08bc12093 100644
>> --- a/include/hw/boards.h
>> +++ b/include/hw/boards.h
>> @@ -36,6 +36,24 @@ void machine_set_cpu_numa_node(MachineState *machine,
>>                                 const CpuInstanceProperties *props,
>>                                 Error **errp);
>>  
>> +/**
>> + * machine_class_valid_for_accelerator:
>> + * @mc: the machine class
>> + * @acc_name: accelerator name
>> + *
>> + * Returns %true if the accelerator is valid for the machine, %false
>> + * otherwise. See #MachineClass.valid_accelerators.
> 
> Naming confusion: is the machine class valid for the accelerator, or
> the accelerator valid for the machine class? Or either? :)

"the accelerator valid for the machine class".

Is this clearer?

"Returns %true if the current accelerator is valid for the
 selected machine, %false otherwise.

Or...

"Returns %true if the selected accelerator is valid for the
 current machine, %false otherwise.

How would look "either"?

The machine is already selected, and the accelerator too...

> 
>> + */
>> +bool machine_class_valid_for_accelerator(MachineClass *mc, const char *acc_name);
>> +/**
>> + * machine_class_valid_for_current_accelerator:
>> + * @mc: the machine class
>> + *
>> + * Returns %true if the accelerator is valid for the current machine,
>> + * %false otherwise. See #MachineClass.valid_accelerators.
> 
> Same here: current accelerator vs. current machine.
> 
>> + */
>> +bool machine_class_valid_for_current_accelerator(MachineClass *mc);
>> +
>>  void machine_class_allow_dynamic_sysbus_dev(MachineClass *mc, const char *type);
>>  /*
>>   * Checks that backend isn't used, preps it for exclusive usage and
>> @@ -125,6 +143,11 @@ typedef struct {
>>   *    should instead use "unimplemented-device" for all memory ranges where
>>   *    the guest will attempt to probe for a device that QEMU doesn't
>>   *    implement and a stub device is required.
>> + * @valid_accelerators:
>> + *    If this machine supports a specific set of virtualization accelerators,
>> + *    this contains a NULL-terminated list of the accelerators that can be
>> + *    used. If this field is not set, any accelerator is valid. The QTest
>> + *    accelerator is always valid.
>>   * @kvm_type:
>>   *    Return the type of KVM corresponding to the kvm-type string option or
>>   *    computed based on other criteria such as the host kernel capabilities
>> @@ -166,6 +189,7 @@ struct MachineClass {
>>      const char *alias;
>>      const char *desc;
>>      const char *deprecation_reason;
>> +    const char *const *valid_accelerators;
>>  
>>      void (*init)(MachineState *state);
>>      void (*reset)(MachineState *state);
>> diff --git a/hw/core/machine.c b/hw/core/machine.c
>> index 970046f4388..c42d8e382b1 100644
>> --- a/hw/core/machine.c
>> +++ b/hw/core/machine.c
>> @@ -518,6 +518,32 @@ static void machine_set_nvdimm_persistence(Object *obj, const char *value,
>>      nvdimms_state->persistence_string = g_strdup(value);
>>  }
>>  
>> +bool machine_class_valid_for_accelerator(MachineClass *mc, const char *acc_name)
>> +{
>> +    const char *const *name = mc->valid_accelerators;
>> +
>> +    if (!name) {
>> +        return true;
>> +    }
>> +    if (strcmp(acc_name, "qtest") == 0) {
>> +        return true;
>> +    }
>> +
>> +    for (unsigned i = 0; name[i]; i++) {
>> +        if (strcasecmp(acc_name, name[i]) == 0) {
>> +            return true;
>> +        }
>> +    }
>> +    return false;
>> +}
>> +
>> +bool machine_class_valid_for_current_accelerator(MachineClass *mc)
>> +{
>> +    AccelClass *ac = ACCEL_GET_CLASS(current_accel());
>> +
>> +    return machine_class_valid_for_accelerator(mc, ac->name);
>> +}
> 
> The implementation of the function tests for the current accelerator,
> so I think you need to tweak the description above?
> 
>> +
>>  void machine_class_allow_dynamic_sysbus_dev(MachineClass *mc, const char *type)
>>  {
>>      QAPI_LIST_PREPEND(mc->allowed_dynamic_sysbus_devices, g_strdup(type));
> 

Re: [PATCH v2 02/11] hw/boards: Introduce machine_class_valid_for_accelerator()

[Cornelia Huck]

On Mon, 22 Feb 2021 18:46:15 +0100
Philippe Mathieu-Daudé <philmd@redhat.com> wrote:

> On 2/22/21 6:34 PM, Cornelia Huck wrote:
> > On Fri, 19 Feb 2021 18:38:38 +0100
> > Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> >   
> >> Introduce the valid_accelerators[] field to express the list
> >> of valid accelators a machine can use, and add the
> >> machine_class_valid_for_current_accelerator() and
> >> machine_class_valid_for_accelerator() methods.
> >>
> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> >> ---
> >>  include/hw/boards.h | 24 ++++++++++++++++++++++++
> >>  hw/core/machine.c   | 26 ++++++++++++++++++++++++++
> >>  2 files changed, 50 insertions(+)
> >>
> >> diff --git a/include/hw/boards.h b/include/hw/boards.h
> >> index 68d3d10f6b0..4d08bc12093 100644
> >> --- a/include/hw/boards.h
> >> +++ b/include/hw/boards.h
> >> @@ -36,6 +36,24 @@ void machine_set_cpu_numa_node(MachineState *machine,
> >>                                 const CpuInstanceProperties *props,
> >>                                 Error **errp);
> >>  
> >> +/**
> >> + * machine_class_valid_for_accelerator:
> >> + * @mc: the machine class
> >> + * @acc_name: accelerator name
> >> + *
> >> + * Returns %true if the accelerator is valid for the machine, %false
> >> + * otherwise. See #MachineClass.valid_accelerators.  
> > 
> > Naming confusion: is the machine class valid for the accelerator, or
> > the accelerator valid for the machine class? Or either? :)  
> 
> "the accelerator valid for the machine class".
> 
> Is this clearer?
> 
> "Returns %true if the current accelerator is valid for the
>  selected machine, %false otherwise.
> 
> Or...
> 
> "Returns %true if the selected accelerator is valid for the
>  current machine, %false otherwise.

Maybe that one, given how it ends up being called? Or "specified
machine"?
> 
> How would look "either"?
> 
> The machine is already selected, and the accelerator too...

Yes, so this is basically testing the (machine,accelerator) tuple,
which is what I meant with 'either'.

> 
> >   
> >> + */
> >> +bool machine_class_valid_for_accelerator(MachineClass *mc, const char *acc_name);
> >> +/**
> >> + * machine_class_valid_for_current_accelerator:
> >> + * @mc: the machine class
> >> + *
> >> + * Returns %true if the accelerator is valid for the current machine,
> >> + * %false otherwise. See #MachineClass.valid_accelerators.  
> > 
> > Same here: current accelerator vs. current machine.

So maybe

"Returns %true if the current accelerator is valid for the specified
machine class..." ?

> >   
> >> + */
> >> +bool machine_class_valid_for_current_accelerator(MachineClass *mc);

[PATCH v2 03/11] hw/core: Restrict 'query-machines' to those supported by current accel

[Philippe Mathieu-Daudé]

Do not let 'query-machines' return machines not valid with
the current accelerator.

Suggested-by: Daniel Berrangé <berrange@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/core/machine-qmp-cmds.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/core/machine-qmp-cmds.c b/hw/core/machine-qmp-cmds.c
index 44e979e503b..c8630bc2ddc 100644
--- a/hw/core/machine-qmp-cmds.c
+++ b/hw/core/machine-qmp-cmds.c
@@ -204,6 +204,10 @@ MachineInfoList *qmp_query_machines(Error **errp)
         MachineClass *mc = el->data;
         MachineInfo *info;
 
+        if (!machine_class_valid_for_current_accelerator(mc)) {
+            continue;
+        }
+
         info = g_malloc0(sizeof(*info));
         if (mc->is_default) {
             info->has_is_default = true;
-- 
2.26.2

Re: [PATCH v2 03/11] hw/core: Restrict 'query-machines' to those supported by current accel

[Cornelia Huck]

On Fri, 19 Feb 2021 18:38:39 +0100
Philippe Mathieu-Daudé <philmd@redhat.com> wrote:

> Do not let 'query-machines' return machines not valid with
> the current accelerator.
> 
> Suggested-by: Daniel Berrangé <berrange@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/core/machine-qmp-cmds.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/hw/core/machine-qmp-cmds.c b/hw/core/machine-qmp-cmds.c
> index 44e979e503b..c8630bc2ddc 100644
> --- a/hw/core/machine-qmp-cmds.c
> +++ b/hw/core/machine-qmp-cmds.c
> @@ -204,6 +204,10 @@ MachineInfoList *qmp_query_machines(Error **errp)
>          MachineClass *mc = el->data;
>          MachineInfo *info;
>  
> +        if (!machine_class_valid_for_current_accelerator(mc)) {
> +            continue;
> +        }
> +
>          info = g_malloc0(sizeof(*info));
>          if (mc->is_default) {
>              info->has_is_default = true;

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

[PATCH v2 04/11] hw/arm: Restrit KVM to the virt & versal machines

[Philippe Mathieu-Daudé]

Restrit KVM to the following ARM machines:
- virt
- xlnx-versal-virt

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/arm/virt.c             | 5 +++++
 hw/arm/xlnx-versal-virt.c | 5 +++++
 2 files changed, 10 insertions(+)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 371147f3ae9..8e9861b61a9 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -2527,6 +2527,10 @@ static HotplugHandler *virt_machine_get_hotplug_handler(MachineState *machine,
     return NULL;
 }
 
+static const char *const valid_accels[] = {
+    "tcg", "kvm", "hvf", NULL
+};
+
 /*
  * for arm64 kvm_type [7-0] encodes the requested number of bits
  * in the IPA address space
@@ -2582,6 +2586,7 @@ static void virt_machine_class_init(ObjectClass *oc, void *data)
     mc->cpu_index_to_instance_props = virt_cpu_index_to_props;
     mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-a15");
     mc->get_default_cpu_node_id = virt_get_default_cpu_node_id;
+    mc->valid_accelerators = valid_accels;
     mc->kvm_type = virt_kvm_type;
     assert(!mc->get_hotplug_handler);
     mc->get_hotplug_handler = virt_machine_get_hotplug_handler;
diff --git a/hw/arm/xlnx-versal-virt.c b/hw/arm/xlnx-versal-virt.c
index 8482cd61960..d424813cae1 100644
--- a/hw/arm/xlnx-versal-virt.c
+++ b/hw/arm/xlnx-versal-virt.c
@@ -610,6 +610,10 @@ static void versal_virt_machine_instance_init(Object *obj)
 {
 }
 
+static const char *const valid_accels[] = {
+    "tcg", "kvm", NULL
+};
+
 static void versal_virt_machine_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
@@ -621,6 +625,7 @@ static void versal_virt_machine_class_init(ObjectClass *oc, void *data)
     mc->default_cpus = XLNX_VERSAL_NR_ACPUS;
     mc->no_cdrom = true;
     mc->default_ram_id = "ddr";
+    mc->valid_accelerators = valid_accels;
 }
 
 static const TypeInfo versal_virt_machine_init_typeinfo = {
-- 
2.26.2

Re: [PATCH v2 04/11] hw/arm: Restrit KVM to the virt & versal machines

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 2088 bytes --]

On Fri, 19 Feb 2021, Philippe Mathieu-Daudé wrote:
> Restrit KVM to the following ARM machines:

Typo: "Restrict" (also in patch title).

Regards,
BALATON Zoltan

> - virt
> - xlnx-versal-virt
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> hw/arm/virt.c             | 5 +++++
> hw/arm/xlnx-versal-virt.c | 5 +++++
> 2 files changed, 10 insertions(+)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 371147f3ae9..8e9861b61a9 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -2527,6 +2527,10 @@ static HotplugHandler *virt_machine_get_hotplug_handler(MachineState *machine,
>     return NULL;
> }
>
> +static const char *const valid_accels[] = {
> +    "tcg", "kvm", "hvf", NULL
> +};
> +
> /*
>  * for arm64 kvm_type [7-0] encodes the requested number of bits
>  * in the IPA address space
> @@ -2582,6 +2586,7 @@ static void virt_machine_class_init(ObjectClass *oc, void *data)
>     mc->cpu_index_to_instance_props = virt_cpu_index_to_props;
>     mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-a15");
>     mc->get_default_cpu_node_id = virt_get_default_cpu_node_id;
> +    mc->valid_accelerators = valid_accels;
>     mc->kvm_type = virt_kvm_type;
>     assert(!mc->get_hotplug_handler);
>     mc->get_hotplug_handler = virt_machine_get_hotplug_handler;
> diff --git a/hw/arm/xlnx-versal-virt.c b/hw/arm/xlnx-versal-virt.c
> index 8482cd61960..d424813cae1 100644
> --- a/hw/arm/xlnx-versal-virt.c
> +++ b/hw/arm/xlnx-versal-virt.c
> @@ -610,6 +610,10 @@ static void versal_virt_machine_instance_init(Object *obj)
> {
> }
>
> +static const char *const valid_accels[] = {
> +    "tcg", "kvm", NULL
> +};
> +
> static void versal_virt_machine_class_init(ObjectClass *oc, void *data)
> {
>     MachineClass *mc = MACHINE_CLASS(oc);
> @@ -621,6 +625,7 @@ static void versal_virt_machine_class_init(ObjectClass *oc, void *data)
>     mc->default_cpus = XLNX_VERSAL_NR_ACPUS;
>     mc->no_cdrom = true;
>     mc->default_ram_id = "ddr";
> +    mc->valid_accelerators = valid_accels;
> }
>
> static const TypeInfo versal_virt_machine_init_typeinfo = {
>

[PATCH v2 05/11] hw/mips: Restrict KVM to the malta & virt machines

[Philippe Mathieu-Daudé]

Restrit KVM to the following MIPS machines:
- malta
- loongson3-virt

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/mips/loongson3_virt.c | 5 +++++
 hw/mips/malta.c          | 5 +++++
 2 files changed, 10 insertions(+)

diff --git a/hw/mips/loongson3_virt.c b/hw/mips/loongson3_virt.c
index d4a82fa5367..c3679dff043 100644
--- a/hw/mips/loongson3_virt.c
+++ b/hw/mips/loongson3_virt.c
@@ -612,6 +612,10 @@ static void mips_loongson3_virt_init(MachineState *machine)
     loongson3_virt_devices_init(machine, liointc);
 }
 
+static const char *const valid_accels[] = {
+    "tcg", "kvm", NULL
+};
+
 static void loongson3v_machine_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
@@ -622,6 +626,7 @@ static void loongson3v_machine_class_init(ObjectClass *oc, void *data)
     mc->max_cpus = LOONGSON_MAX_VCPUS;
     mc->default_ram_id = "loongson3.highram";
     mc->default_ram_size = 1600 * MiB;
+    mc->valid_accelerators = valid_accels;
     mc->kvm_type = mips_kvm_type;
     mc->minimum_page_bits = 14;
 }
diff --git a/hw/mips/malta.c b/hw/mips/malta.c
index 9afc0b427bf..0212048dc63 100644
--- a/hw/mips/malta.c
+++ b/hw/mips/malta.c
@@ -1443,6 +1443,10 @@ static const TypeInfo mips_malta_device = {
     .instance_init = mips_malta_instance_init,
 };
 
+static const char *const valid_accels[] = {
+    "tcg", "kvm", NULL
+};
+
 static void mips_malta_machine_init(MachineClass *mc)
 {
     mc->desc = "MIPS Malta Core LV";
@@ -1456,6 +1460,7 @@ static void mips_malta_machine_init(MachineClass *mc)
     mc->default_cpu_type = MIPS_CPU_TYPE_NAME("24Kf");
 #endif
     mc->default_ram_id = "mips_malta.ram";
+    mc->valid_accelerators = valid_accels;
 }
 
 DEFINE_MACHINE("malta", mips_malta_machine_init)
-- 
2.26.2

Re: [PATCH v2 05/11] hw/mips: Restrict KVM to the malta & virt machines

[Huacai Chen]

Reviewed-by: Huacai Chen <chenhuacai@kernel.org>

On Sat, Feb 20, 2021 at 12:56 PM Jiaxun Yang <jiaxun.yang@flygoat.com> wrote:
>
> 在 2021/2/20 上午1:38, Philippe Mathieu-Daudé 写道:
> > Restrit KVM to the following MIPS machines:
> > - malta
> > - loongson3-virt
> >
> > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>
> Reviewed-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
>
> > ---
> >   hw/mips/loongson3_virt.c | 5 +++++
> >   hw/mips/malta.c          | 5 +++++
> >   2 files changed, 10 insertions(+)
> >
> > diff --git a/hw/mips/loongson3_virt.c b/hw/mips/loongson3_virt.c
> > index d4a82fa5367..c3679dff043 100644
> > --- a/hw/mips/loongson3_virt.c
> > +++ b/hw/mips/loongson3_virt.c
> > @@ -612,6 +612,10 @@ static void mips_loongson3_virt_init(MachineState *machine)
> >       loongson3_virt_devices_init(machine, liointc);
> >   }
> >
> > +static const char *const valid_accels[] = {
> > +    "tcg", "kvm", NULL
> > +};
> > +
> >   static void loongson3v_machine_class_init(ObjectClass *oc, void *data)
> >   {
> >       MachineClass *mc = MACHINE_CLASS(oc);
> > @@ -622,6 +626,7 @@ static void loongson3v_machine_class_init(ObjectClass *oc, void *data)
> >       mc->max_cpus = LOONGSON_MAX_VCPUS;
> >       mc->default_ram_id = "loongson3.highram";
> >       mc->default_ram_size = 1600 * MiB;
> > +    mc->valid_accelerators = valid_accels;
> >       mc->kvm_type = mips_kvm_type;
> >       mc->minimum_page_bits = 14;
> >   }
> > diff --git a/hw/mips/malta.c b/hw/mips/malta.c
> > index 9afc0b427bf..0212048dc63 100644
> > --- a/hw/mips/malta.c
> > +++ b/hw/mips/malta.c
> > @@ -1443,6 +1443,10 @@ static const TypeInfo mips_malta_device = {
> >       .instance_init = mips_malta_instance_init,
> >   };
> >
> > +static const char *const valid_accels[] = {
> > +    "tcg", "kvm", NULL
> > +};
> > +
> >   static void mips_malta_machine_init(MachineClass *mc)
> >   {
> >       mc->desc = "MIPS Malta Core LV";
> > @@ -1456,6 +1460,7 @@ static void mips_malta_machine_init(MachineClass *mc)
> >       mc->default_cpu_type = MIPS_CPU_TYPE_NAME("24Kf");
> >   #endif
> >       mc->default_ram_id = "mips_malta.ram";
> > +    mc->valid_accelerators = valid_accels;
> >   }
> >
> >   DEFINE_MACHINE("malta", mips_malta_machine_init)
>

[RFC PATCH v2 06/11] hw/ppc: Restrict KVM to various PPC machines

[Philippe Mathieu-Daudé]

Restrit KVM to the following PPC machines:
- 40p
- bamboo
- g3beige
- mac99
- mpc8544ds
- ppce500
- pseries
- sam460ex
- virtex-ml507

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
RFC: I'm surprise by this list, but this is the result of
     auditing calls to kvm_enabled() checks.

 hw/ppc/e500plat.c      | 5 +++++
 hw/ppc/mac_newworld.c  | 6 ++++++
 hw/ppc/mac_oldworld.c  | 5 +++++
 hw/ppc/mpc8544ds.c     | 5 +++++
 hw/ppc/ppc440_bamboo.c | 5 +++++
 hw/ppc/prep.c          | 5 +++++
 hw/ppc/sam460ex.c      | 5 +++++
 hw/ppc/spapr.c         | 5 +++++
 8 files changed, 41 insertions(+)

diff --git a/hw/ppc/e500plat.c b/hw/ppc/e500plat.c
index bddd5e7c48f..9701dbc2231 100644
--- a/hw/ppc/e500plat.c
+++ b/hw/ppc/e500plat.c
@@ -67,6 +67,10 @@ HotplugHandler *e500plat_machine_get_hotpug_handler(MachineState *machine,
 
 #define TYPE_E500PLAT_MACHINE  MACHINE_TYPE_NAME("ppce500")
 
+static const char *const valid_accels[] = {
+    "tcg", "kvm", NULL
+};
+
 static void e500plat_machine_class_init(ObjectClass *oc, void *data)
 {
     PPCE500MachineClass *pmc = PPCE500_MACHINE_CLASS(oc);
@@ -98,6 +102,7 @@ static void e500plat_machine_class_init(ObjectClass *oc, void *data)
     mc->max_cpus = 32;
     mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("e500v2_v30");
     mc->default_ram_id = "mpc8544ds.ram";
+    mc->valid_accelerators = valid_accels;
     machine_class_allow_dynamic_sysbus_dev(mc, TYPE_ETSEC_COMMON);
  }
 
diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
index e991db4addb..634f5ad19a0 100644
--- a/hw/ppc/mac_newworld.c
+++ b/hw/ppc/mac_newworld.c
@@ -578,6 +578,11 @@ static char *core99_fw_dev_path(FWPathProvider *p, BusState *bus,
 
     return NULL;
 }
+
+static const char *const valid_accels[] = {
+    "tcg", "kvm", NULL
+};
+
 static int core99_kvm_type(MachineState *machine, const char *arg)
 {
     /* Always force PR KVM */
@@ -595,6 +600,7 @@ static void core99_machine_class_init(ObjectClass *oc, void *data)
     mc->max_cpus = MAX_CPUS;
     mc->default_boot_order = "cd";
     mc->default_display = "std";
+    mc->valid_accelerators = valid_accels;
     mc->kvm_type = core99_kvm_type;
 #ifdef TARGET_PPC64
     mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("970fx_v3.1");
diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c
index 44ee99be886..2c58f73b589 100644
--- a/hw/ppc/mac_oldworld.c
+++ b/hw/ppc/mac_oldworld.c
@@ -424,6 +424,10 @@ static char *heathrow_fw_dev_path(FWPathProvider *p, BusState *bus,
     return NULL;
 }
 
+static const char *const valid_accels[] = {
+    "tcg", "kvm", NULL
+};
+
 static int heathrow_kvm_type(MachineState *machine, const char *arg)
 {
     /* Always force PR KVM */
@@ -444,6 +448,7 @@ static void heathrow_class_init(ObjectClass *oc, void *data)
 #endif
     /* TOFIX "cad" when Mac floppy is implemented */
     mc->default_boot_order = "cd";
+    mc->valid_accelerators = valid_accels;
     mc->kvm_type = heathrow_kvm_type;
     mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("750_v3.1");
     mc->default_display = "std";
diff --git a/hw/ppc/mpc8544ds.c b/hw/ppc/mpc8544ds.c
index 81177505f02..92b0e926c1b 100644
--- a/hw/ppc/mpc8544ds.c
+++ b/hw/ppc/mpc8544ds.c
@@ -36,6 +36,10 @@ static void mpc8544ds_init(MachineState *machine)
     ppce500_init(machine);
 }
 
+static const char *const valid_accels[] = {
+    "tcg", "kvm", NULL
+};
+
 static void e500plat_machine_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
@@ -56,6 +60,7 @@ static void e500plat_machine_class_init(ObjectClass *oc, void *data)
     mc->max_cpus = 15;
     mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("e500v2_v30");
     mc->default_ram_id = "mpc8544ds.ram";
+    mc->valid_accelerators = valid_accels;
 }
 
 #define TYPE_MPC8544DS_MACHINE  MACHINE_TYPE_NAME("mpc8544ds")
diff --git a/hw/ppc/ppc440_bamboo.c b/hw/ppc/ppc440_bamboo.c
index b156bcb9990..02501f489e4 100644
--- a/hw/ppc/ppc440_bamboo.c
+++ b/hw/ppc/ppc440_bamboo.c
@@ -298,12 +298,17 @@ static void bamboo_init(MachineState *machine)
     }
 }
 
+static const char *const valid_accels[] = {
+    "tcg", "kvm", NULL
+};
+
 static void bamboo_machine_init(MachineClass *mc)
 {
     mc->desc = "bamboo";
     mc->init = bamboo_init;
     mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("440epb");
     mc->default_ram_id = "ppc4xx.sdram";
+    mc->valid_accelerators = valid_accels;
 }
 
 DEFINE_MACHINE("bamboo", bamboo_machine_init)
diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c
index 7e72f6e4a9b..90d884b0883 100644
--- a/hw/ppc/prep.c
+++ b/hw/ppc/prep.c
@@ -431,6 +431,10 @@ static void ibm_40p_init(MachineState *machine)
     }
 }
 
+static const char *const valid_accels[] = {
+    "tcg", "kvm", NULL
+};
+
 static void ibm_40p_machine_init(MachineClass *mc)
 {
     mc->desc = "IBM RS/6000 7020 (40p)",
@@ -441,6 +445,7 @@ static void ibm_40p_machine_init(MachineClass *mc)
     mc->default_boot_order = "c";
     mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("604");
     mc->default_display = "std";
+    mc->valid_accelerators = valid_accels;
 }
 
 DEFINE_MACHINE("40p", ibm_40p_machine_init)
diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
index e459b43065b..79adb3352f0 100644
--- a/hw/ppc/sam460ex.c
+++ b/hw/ppc/sam460ex.c
@@ -506,6 +506,10 @@ static void sam460ex_init(MachineState *machine)
     boot_info->entry = entry;
 }
 
+static const char *const valid_accels[] = {
+    "tcg", "kvm", NULL
+};
+
 static void sam460ex_machine_init(MachineClass *mc)
 {
     mc->desc = "aCube Sam460ex";
@@ -513,6 +517,7 @@ static void sam460ex_machine_init(MachineClass *mc)
     mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("460exb");
     mc->default_ram_size = 512 * MiB;
     mc->default_ram_id = "ppc4xx.sdram";
+    mc->valid_accelerators = valid_accels;
 }
 
 DEFINE_MACHINE("sam460ex", sam460ex_machine_init)
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 85fe65f8947..c5f985f0187 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -4397,6 +4397,10 @@ static void spapr_cpu_exec_exit(PPCVirtualHypervisor *vhyp, PowerPCCPU *cpu)
     }
 }
 
+static const char *const valid_accels[] = {
+    "tcg", "kvm", NULL
+};
+
 static void spapr_machine_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
@@ -4426,6 +4430,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
     mc->default_ram_size = 512 * MiB;
     mc->default_ram_id = "ppc_spapr.ram";
     mc->default_display = "std";
+    mc->valid_accelerators = valid_accels;
     mc->kvm_type = spapr_kvm_type;
     machine_class_allow_dynamic_sysbus_dev(mc, TYPE_SPAPR_PCI_HOST_BRIDGE);
     mc->pci_allow_0_address = true;
-- 
2.26.2

Re: [RFC PATCH v2 06/11] hw/ppc: Restrict KVM to various PPC machines

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 8299 bytes --]

On Fri, Feb 19, 2021 at 06:38:42PM +0100, Philippe Mathieu-Daudé wrote:
> Restrit KVM to the following PPC machines:
> - 40p
> - bamboo
> - g3beige
> - mac99
> - mpc8544ds
> - ppce500
> - pseries
> - sam460ex
> - virtex-ml507

Hrm.

The reason this list is kind of surprising is because there are 3
different "flavours" of KVM on ppc: KVM HV ("pseries" only), KVM PR
(almost any combination, theoretically, but kind of buggy in
practice), and the Book E specific KVM (Book-E systems with HV
extensions only).

But basically, qemu explicitly managing what accelerators are
available for each machine seems the wrong way around to me.  The
approach we've generally taken is that qemu requests the specific
features it needs of KVM, and KVM tells us whether it can supply those
or not (which may involve selecting between one of the several
flavours).

That way we can extend KVM to cover more situations without needing
corresponding changes in qemu every time.


> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> RFC: I'm surprise by this list, but this is the result of
>      auditing calls to kvm_enabled() checks.
> 
>  hw/ppc/e500plat.c      | 5 +++++
>  hw/ppc/mac_newworld.c  | 6 ++++++
>  hw/ppc/mac_oldworld.c  | 5 +++++
>  hw/ppc/mpc8544ds.c     | 5 +++++
>  hw/ppc/ppc440_bamboo.c | 5 +++++
>  hw/ppc/prep.c          | 5 +++++
>  hw/ppc/sam460ex.c      | 5 +++++
>  hw/ppc/spapr.c         | 5 +++++
>  8 files changed, 41 insertions(+)
> 
> diff --git a/hw/ppc/e500plat.c b/hw/ppc/e500plat.c
> index bddd5e7c48f..9701dbc2231 100644
> --- a/hw/ppc/e500plat.c
> +++ b/hw/ppc/e500plat.c
> @@ -67,6 +67,10 @@ HotplugHandler *e500plat_machine_get_hotpug_handler(MachineState *machine,
>  
>  #define TYPE_E500PLAT_MACHINE  MACHINE_TYPE_NAME("ppce500")
>  
> +static const char *const valid_accels[] = {
> +    "tcg", "kvm", NULL
> +};
> +
>  static void e500plat_machine_class_init(ObjectClass *oc, void *data)
>  {
>      PPCE500MachineClass *pmc = PPCE500_MACHINE_CLASS(oc);
> @@ -98,6 +102,7 @@ static void e500plat_machine_class_init(ObjectClass *oc, void *data)
>      mc->max_cpus = 32;
>      mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("e500v2_v30");
>      mc->default_ram_id = "mpc8544ds.ram";
> +    mc->valid_accelerators = valid_accels;
>      machine_class_allow_dynamic_sysbus_dev(mc, TYPE_ETSEC_COMMON);
>   }
>  
> diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
> index e991db4addb..634f5ad19a0 100644
> --- a/hw/ppc/mac_newworld.c
> +++ b/hw/ppc/mac_newworld.c
> @@ -578,6 +578,11 @@ static char *core99_fw_dev_path(FWPathProvider *p, BusState *bus,
>  
>      return NULL;
>  }
> +
> +static const char *const valid_accels[] = {
> +    "tcg", "kvm", NULL
> +};
> +
>  static int core99_kvm_type(MachineState *machine, const char *arg)
>  {
>      /* Always force PR KVM */
> @@ -595,6 +600,7 @@ static void core99_machine_class_init(ObjectClass *oc, void *data)
>      mc->max_cpus = MAX_CPUS;
>      mc->default_boot_order = "cd";
>      mc->default_display = "std";
> +    mc->valid_accelerators = valid_accels;
>      mc->kvm_type = core99_kvm_type;
>  #ifdef TARGET_PPC64
>      mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("970fx_v3.1");
> diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c
> index 44ee99be886..2c58f73b589 100644
> --- a/hw/ppc/mac_oldworld.c
> +++ b/hw/ppc/mac_oldworld.c
> @@ -424,6 +424,10 @@ static char *heathrow_fw_dev_path(FWPathProvider *p, BusState *bus,
>      return NULL;
>  }
>  
> +static const char *const valid_accels[] = {
> +    "tcg", "kvm", NULL
> +};
> +
>  static int heathrow_kvm_type(MachineState *machine, const char *arg)
>  {
>      /* Always force PR KVM */
> @@ -444,6 +448,7 @@ static void heathrow_class_init(ObjectClass *oc, void *data)
>  #endif
>      /* TOFIX "cad" when Mac floppy is implemented */
>      mc->default_boot_order = "cd";
> +    mc->valid_accelerators = valid_accels;
>      mc->kvm_type = heathrow_kvm_type;
>      mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("750_v3.1");
>      mc->default_display = "std";
> diff --git a/hw/ppc/mpc8544ds.c b/hw/ppc/mpc8544ds.c
> index 81177505f02..92b0e926c1b 100644
> --- a/hw/ppc/mpc8544ds.c
> +++ b/hw/ppc/mpc8544ds.c
> @@ -36,6 +36,10 @@ static void mpc8544ds_init(MachineState *machine)
>      ppce500_init(machine);
>  }
>  
> +static const char *const valid_accels[] = {
> +    "tcg", "kvm", NULL
> +};
> +
>  static void e500plat_machine_class_init(ObjectClass *oc, void *data)
>  {
>      MachineClass *mc = MACHINE_CLASS(oc);
> @@ -56,6 +60,7 @@ static void e500plat_machine_class_init(ObjectClass *oc, void *data)
>      mc->max_cpus = 15;
>      mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("e500v2_v30");
>      mc->default_ram_id = "mpc8544ds.ram";
> +    mc->valid_accelerators = valid_accels;
>  }
>  
>  #define TYPE_MPC8544DS_MACHINE  MACHINE_TYPE_NAME("mpc8544ds")
> diff --git a/hw/ppc/ppc440_bamboo.c b/hw/ppc/ppc440_bamboo.c
> index b156bcb9990..02501f489e4 100644
> --- a/hw/ppc/ppc440_bamboo.c
> +++ b/hw/ppc/ppc440_bamboo.c
> @@ -298,12 +298,17 @@ static void bamboo_init(MachineState *machine)
>      }
>  }
>  
> +static const char *const valid_accels[] = {
> +    "tcg", "kvm", NULL
> +};
> +
>  static void bamboo_machine_init(MachineClass *mc)
>  {
>      mc->desc = "bamboo";
>      mc->init = bamboo_init;
>      mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("440epb");
>      mc->default_ram_id = "ppc4xx.sdram";
> +    mc->valid_accelerators = valid_accels;
>  }
>  
>  DEFINE_MACHINE("bamboo", bamboo_machine_init)
> diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c
> index 7e72f6e4a9b..90d884b0883 100644
> --- a/hw/ppc/prep.c
> +++ b/hw/ppc/prep.c
> @@ -431,6 +431,10 @@ static void ibm_40p_init(MachineState *machine)
>      }
>  }
>  
> +static const char *const valid_accels[] = {
> +    "tcg", "kvm", NULL
> +};
> +
>  static void ibm_40p_machine_init(MachineClass *mc)
>  {
>      mc->desc = "IBM RS/6000 7020 (40p)",
> @@ -441,6 +445,7 @@ static void ibm_40p_machine_init(MachineClass *mc)
>      mc->default_boot_order = "c";
>      mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("604");
>      mc->default_display = "std";
> +    mc->valid_accelerators = valid_accels;
>  }
>  
>  DEFINE_MACHINE("40p", ibm_40p_machine_init)
> diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
> index e459b43065b..79adb3352f0 100644
> --- a/hw/ppc/sam460ex.c
> +++ b/hw/ppc/sam460ex.c
> @@ -506,6 +506,10 @@ static void sam460ex_init(MachineState *machine)
>      boot_info->entry = entry;
>  }
>  
> +static const char *const valid_accels[] = {
> +    "tcg", "kvm", NULL
> +};
> +
>  static void sam460ex_machine_init(MachineClass *mc)
>  {
>      mc->desc = "aCube Sam460ex";
> @@ -513,6 +517,7 @@ static void sam460ex_machine_init(MachineClass *mc)
>      mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("460exb");
>      mc->default_ram_size = 512 * MiB;
>      mc->default_ram_id = "ppc4xx.sdram";
> +    mc->valid_accelerators = valid_accels;
>  }
>  
>  DEFINE_MACHINE("sam460ex", sam460ex_machine_init)
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 85fe65f8947..c5f985f0187 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -4397,6 +4397,10 @@ static void spapr_cpu_exec_exit(PPCVirtualHypervisor *vhyp, PowerPCCPU *cpu)
>      }
>  }
>  
> +static const char *const valid_accels[] = {
> +    "tcg", "kvm", NULL
> +};
> +
>  static void spapr_machine_class_init(ObjectClass *oc, void *data)
>  {
>      MachineClass *mc = MACHINE_CLASS(oc);
> @@ -4426,6 +4430,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
>      mc->default_ram_size = 512 * MiB;
>      mc->default_ram_id = "ppc_spapr.ram";
>      mc->default_display = "std";
> +    mc->valid_accelerators = valid_accels;
>      mc->kvm_type = spapr_kvm_type;
>      machine_class_allow_dynamic_sysbus_dev(mc, TYPE_SPAPR_PCI_HOST_BRIDGE);
>      mc->pci_allow_0_address = true;

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC PATCH v2 06/11] hw/ppc: Restrict KVM to various PPC machines

[Philippe Mathieu-Daudé]

On 2/22/21 6:59 AM, David Gibson wrote:
> On Fri, Feb 19, 2021 at 06:38:42PM +0100, Philippe Mathieu-Daudé wrote:
>> Restrit KVM to the following PPC machines:
>> - 40p
>> - bamboo
>> - g3beige
>> - mac99
>> - mpc8544ds
>> - ppce500
>> - pseries
>> - sam460ex
>> - virtex-ml507
> 
> Hrm.
> 
> The reason this list is kind of surprising is because there are 3
> different "flavours" of KVM on ppc: KVM HV ("pseries" only), KVM PR
> (almost any combination, theoretically, but kind of buggy in
> practice), and the Book E specific KVM (Book-E systems with HV
> extensions only).
> 
> But basically, qemu explicitly managing what accelerators are
> available for each machine seems the wrong way around to me.  The
> approach we've generally taken is that qemu requests the specific
> features it needs of KVM, and KVM tells us whether it can supply those
> or not (which may involve selecting between one of the several
> flavours).
> 
> That way we can extend KVM to cover more situations without needing
> corresponding changes in qemu every time.

OK thanks for the information. I'll wait the other patches
get reviewed (in particular the most important ones, 2 and
10) before respining including this information.

Regards,

Phil.

[PATCH v2 07/11] hw/s390x: Explicit the s390-ccw-virtio machines support TCG and KVM

[Philippe Mathieu-Daudé]

All s390-ccw-virtio machines support TCG and KVM.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/s390x/s390-virtio-ccw.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 2972b607f36..1f168485066 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -586,6 +586,10 @@ static ram_addr_t s390_fixup_ram_size(ram_addr_t sz)
     return newsz;
 }
 
+static const char *const valid_accels[] = {
+    "tcg", "kvm", NULL
+};
+
 static void ccw_machine_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
@@ -612,6 +616,7 @@ static void ccw_machine_class_init(ObjectClass *oc, void *data)
     mc->possible_cpu_arch_ids = s390_possible_cpu_arch_ids;
     /* it is overridden with 'host' cpu *in kvm_arch_init* */
     mc->default_cpu_type = S390_CPU_TYPE_NAME("qemu");
+    mc->valid_accelerators = valid_accels;
     hc->plug = s390_machine_device_plug;
     hc->unplug_request = s390_machine_device_unplug_request;
     nc->nmi_monitor_handler = s390_nmi;
-- 
2.26.2

Re: [PATCH v2 07/11] hw/s390x: Explicit the s390-ccw-virtio machines support TCG and KVM

[Cornelia Huck]

On Fri, 19 Feb 2021 18:38:43 +0100
Philippe Mathieu-Daudé <philmd@redhat.com> wrote:

I'd lose the 'Explicit' in $SUBJECT.


> All s390-ccw-virtio machines support TCG and KVM.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/s390x/s390-virtio-ccw.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 2972b607f36..1f168485066 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -586,6 +586,10 @@ static ram_addr_t s390_fixup_ram_size(ram_addr_t sz)
>      return newsz;
>  }
>  
> +static const char *const valid_accels[] = {
> +    "tcg", "kvm", NULL
> +};
> +
>  static void ccw_machine_class_init(ObjectClass *oc, void *data)
>  {
>      MachineClass *mc = MACHINE_CLASS(oc);
> @@ -612,6 +616,7 @@ static void ccw_machine_class_init(ObjectClass *oc, void *data)
>      mc->possible_cpu_arch_ids = s390_possible_cpu_arch_ids;
>      /* it is overridden with 'host' cpu *in kvm_arch_init* */
>      mc->default_cpu_type = S390_CPU_TYPE_NAME("qemu");
> +    mc->valid_accelerators = valid_accels;
>      hc->plug = s390_machine_device_plug;
>      hc->unplug_request = s390_machine_device_unplug_request;
>      nc->nmi_monitor_handler = s390_nmi;

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

[RFC PATCH v2 08/11] hw/i386: Explicit x86 machines support all current accelerators

[Philippe Mathieu-Daudé]

x86 machines currently support all accelerators.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
RFC: not sure about this, x86 is not my cup of tea

 hw/i386/x86.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/i386/x86.c b/hw/i386/x86.c
index 6329f90ef90..2dc10e7d386 100644
--- a/hw/i386/x86.c
+++ b/hw/i386/x86.c
@@ -1209,6 +1209,10 @@ static void x86_machine_initfn(Object *obj)
     x86ms->pci_irq_mask = ACPI_BUILD_PCI_IRQS;
 }
 
+static const char *const valid_accels[] = {
+    "tcg", "kvm", "xen", "hax", "hvf", "whpx", NULL
+};
+
 static void x86_machine_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
@@ -1218,6 +1222,7 @@ static void x86_machine_class_init(ObjectClass *oc, void *data)
     mc->cpu_index_to_instance_props = x86_cpu_index_to_props;
     mc->get_default_cpu_node_id = x86_get_default_cpu_node_id;
     mc->possible_cpu_arch_ids = x86_possible_cpu_arch_ids;
+    mc->valid_accelerators = valid_accels;
     x86mc->compat_apic_id_mode = false;
     x86mc->save_tsc_khz = true;
     nc->nmi_monitor_handler = x86_nmi;
-- 
2.26.2

[PATCH v2 09/11] hw/xenpv: Restrict Xen Para-virtualized machine to Xen accelerator

[Philippe Mathieu-Daudé]

When started with other accelerator than Xen, the XenPV machine
fails with a criptic message:

  $ qemu-system-x86_64 -M xenpv,accel=kvm
  xen be core: can't connect to xenstored
  qemu-system-x86_64: xen_init_pv: xen backend core setup failed

By restricting it to Xen, we display a clearer error message:

  $ qemu-system-x86_64 -M xenpv,accel=kvm
  qemu-system-x86_64: invalid accelerator 'kvm' for machine xenpv

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/xenpv/xen_machine_pv.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/xenpv/xen_machine_pv.c b/hw/xenpv/xen_machine_pv.c
index 8df575a457c..d7747bcec98 100644
--- a/hw/xenpv/xen_machine_pv.c
+++ b/hw/xenpv/xen_machine_pv.c
@@ -86,12 +86,17 @@ static void xen_init_pv(MachineState *machine)
     atexit(xen_config_cleanup);
 }
 
+static const char *valid_accels[] = {
+    "xen", NULL
+};
+
 static void xenpv_machine_init(MachineClass *mc)
 {
     mc->desc = "Xen Para-virtualized PC";
     mc->init = xen_init_pv;
     mc->max_cpus = 1;
     mc->default_machine_opts = "accel=xen";
+    mc->valid_accelerators = valid_accels;
 }
 
 DEFINE_MACHINE("xenpv", xenpv_machine_init)
-- 
2.26.2

Re: [PATCH v2 09/11] hw/xenpv: Restrict Xen Para-virtualized machine to Xen accelerator

[Paul Durrant]

On 19/02/2021 17:38, Philippe Mathieu-Daudé wrote:
> When started with other accelerator than Xen, the XenPV machine
> fails with a criptic message:
> 
>    $ qemu-system-x86_64 -M xenpv,accel=kvm
>    xen be core: can't connect to xenstored
>    qemu-system-x86_64: xen_init_pv: xen backend core setup failed
> 
> By restricting it to Xen, we display a clearer error message:
> 
>    $ qemu-system-x86_64 -M xenpv,accel=kvm
>    qemu-system-x86_64: invalid accelerator 'kvm' for machine xenpv
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Acked-by: Paul Durrant <paul@xen.org>

[PATCH v2 10/11] hw/board: Only allow TCG accelerator by default

[Philippe Mathieu-Daudé]

By default machines can only use the TCG and QTest accelerators.

If a machine can use another accelerator, it has to explicitly
list it in its MachineClass valid_accelerators[].

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/hw/boards.h | 4 ++--
 hw/core/machine.c   | 8 ++++----
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/include/hw/boards.h b/include/hw/boards.h
index 4d08bc12093..b93d290b348 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -146,8 +146,8 @@ typedef struct {
  * @valid_accelerators:
  *    If this machine supports a specific set of virtualization accelerators,
  *    this contains a NULL-terminated list of the accelerators that can be
- *    used. If this field is not set, any accelerator is valid. The QTest
- *    accelerator is always valid.
+ *    used. If this field is not set, a default list containing only the TCG
+ *    accelerator is used. The QTest accelerator is always valid.
  * @kvm_type:
  *    Return the type of KVM corresponding to the kvm-type string option or
  *    computed based on other criteria such as the host kernel capabilities
diff --git a/hw/core/machine.c b/hw/core/machine.c
index c42d8e382b1..ca7c9ee2a0c 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -520,11 +520,11 @@ static void machine_set_nvdimm_persistence(Object *obj, const char *value,
 
 bool machine_class_valid_for_accelerator(MachineClass *mc, const char *acc_name)
 {
-    const char *const *name = mc->valid_accelerators;
+    static const char *const default_accels[] = {
+        "tcg", NULL
+    };
+    const char *const *name = mc->valid_accelerators ? : default_accels;
 
-    if (!name) {
-        return true;
-    }
     if (strcmp(acc_name, "qtest") == 0) {
         return true;
     }
-- 
2.26.2

[PATCH v2 11/11] softmmu/vl: Exit gracefully when accelerator is not supported

[Philippe Mathieu-Daudé]

Before configuring an accelerator, check it is valid for
the current machine. Doing so we can return a simple error
message instead of criptic one.

Before:

  $ qemu-system-arm -M raspi2b -enable-kvm
  qemu-system-arm: /build/qemu-ETIdrs/qemu-4.2/exec.c:865: cpu_address_space_init: Assertion `asidx == 0 || !kvm_enabled()' failed.
  Aborted

  $ qemu-system-aarch64 -M xlnx-zcu102 -enable-kvm -smp 6
  qemu-system-aarch64: kvm_init_vcpu: kvm_arch_init_vcpu failed (0): Invalid argument

After:

  $ qemu-system-arm -M raspi2b -enable-kvm
  qemu-system-aarch64: invalid accelerator 'kvm' for machine raspi2b

  $ qemu-system-aarch64 -M xlnx-zcu102 -enable-kvm -smp 6
  qemu-system-aarch64: -accel kvm: invalid accelerator 'kvm' for machine xlnx-zcu102

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 softmmu/vl.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/softmmu/vl.c b/softmmu/vl.c
index b219ce1f357..f2c4074310b 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -2133,6 +2133,7 @@ static int do_configure_accelerator(void *opaque, QemuOpts *opts, Error **errp)
     const char *acc = qemu_opt_get(opts, "accel");
     AccelClass *ac = accel_find(acc);
     AccelState *accel;
+    MachineClass *mc;
     int ret;
     bool qtest_with_kvm;
 
@@ -2145,6 +2146,12 @@ static int do_configure_accelerator(void *opaque, QemuOpts *opts, Error **errp)
         }
         return 0;
     }
+    mc = MACHINE_GET_CLASS(current_machine);
+    if (!qtest_chrdev && !machine_class_valid_for_accelerator(mc, ac->name)) {
+        *p_init_failed = true;
+        error_report("invalid accelerator '%s' for machine %s", acc, mc->name);
+        return 0;
+    }
     accel = ACCEL(object_new_with_class(OBJECT_CLASS(ac)));
     object_apply_compat_props(OBJECT(accel));
     qemu_opt_foreach(opts, accelerator_set_property,
-- 
2.26.2

Re: [PATCH v2 11/11] softmmu/vl: Exit gracefully when accelerator is not supported

[Cornelia Huck]

On Fri, 19 Feb 2021 18:38:47 +0100
Philippe Mathieu-Daudé <philmd@redhat.com> wrote:

> Before configuring an accelerator, check it is valid for
> the current machine. Doing so we can return a simple error
> message instead of criptic one.

s/criptic/cryptic/

> 
> Before:
> 
>   $ qemu-system-arm -M raspi2b -enable-kvm
>   qemu-system-arm: /build/qemu-ETIdrs/qemu-4.2/exec.c:865: cpu_address_space_init: Assertion `asidx == 0 || !kvm_enabled()' failed.
>   Aborted
> 
>   $ qemu-system-aarch64 -M xlnx-zcu102 -enable-kvm -smp 6
>   qemu-system-aarch64: kvm_init_vcpu: kvm_arch_init_vcpu failed (0): Invalid argument
> 
> After:
> 
>   $ qemu-system-arm -M raspi2b -enable-kvm
>   qemu-system-aarch64: invalid accelerator 'kvm' for machine raspi2b
> 
>   $ qemu-system-aarch64 -M xlnx-zcu102 -enable-kvm -smp 6
>   qemu-system-aarch64: -accel kvm: invalid accelerator 'kvm' for machine xlnx-zcu102
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  softmmu/vl.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index b219ce1f357..f2c4074310b 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -2133,6 +2133,7 @@ static int do_configure_accelerator(void *opaque, QemuOpts *opts, Error **errp)
>      const char *acc = qemu_opt_get(opts, "accel");
>      AccelClass *ac = accel_find(acc);
>      AccelState *accel;
> +    MachineClass *mc;
>      int ret;
>      bool qtest_with_kvm;
>  
> @@ -2145,6 +2146,12 @@ static int do_configure_accelerator(void *opaque, QemuOpts *opts, Error **errp)
>          }
>          return 0;
>      }
> +    mc = MACHINE_GET_CLASS(current_machine);
> +    if (!qtest_chrdev && !machine_class_valid_for_accelerator(mc, ac->name)) {

Shouldn't qtest be already allowed in any case in the checking function?

> +        *p_init_failed = true;
> +        error_report("invalid accelerator '%s' for machine %s", acc, mc->name);
> +        return 0;
> +    }
>      accel = ACCEL(object_new_with_class(OBJECT_CLASS(ac)));
>      object_apply_compat_props(OBJECT(accel));
>      qemu_opt_foreach(opts, accelerator_set_property,