Re: [PATCH v2 01/11] accel/kvm: Check MachineClass kvm_type() return value
On 2/23/21 11:36 AM, Cornelia Huck wrote:
> On Tue, 23 Feb 2021 10:37:01 +1100
> David Gibson 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é wrote:
> On 2/22/21 6:24 PM, Cornelia Huck wrote:
>> On Fri, 19 Feb 2021 18:38:37 +0100
>> Philippe Mathieu-Daudé wrote:
>>
>>> MachineClass::kvm_type() can return -1 on failure.
>>> Document it, and add a check in kvm_init().
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé
>>> ---
>>> 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",
>>>
>>> _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",
+_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?)
>
Re: [PATCH v2 01/11] accel/kvm: Check MachineClass kvm_type() return value
On Tue, 23 Feb 2021 10:37:01 +1100
David Gibson 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é wrote:
> > >
> > > > On 2/22/21 6:24 PM, Cornelia Huck wrote:
> > > > > On Fri, 19 Feb 2021 18:38:37 +0100
> > > > > Philippe Mathieu-Daudé wrote:
> > > > >
> > > > >> MachineClass::kvm_type() can return -1 on failure.
> > > > >> Document it, and add a check in kvm_init().
> > > > >>
> > > > >> Signed-off-by: Philippe Mathieu-Daudé
> > > > >> ---
> > > > >> 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",
> > > > >>
> > > > >> _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?)
pgpBB6MqDKqZs.pgp
Description: OpenPGP digital signature
Re: [PATCH v2 01/11] accel/kvm: Check MachineClass kvm_type() return value
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é wrote:
>
> > On 2/22/21 6:24 PM, Cornelia Huck wrote:
> > > On Fri, 19 Feb 2021 18:38:37 +0100
> > > Philippe Mathieu-Daudé wrote:
> > >
> > >> MachineClass::kvm_type() can return -1 on failure.
> > >> Document it, and add a check in kvm_init().
> > >>
> > >> Signed-off-by: Philippe Mathieu-Daudé
> > >> ---
> > >> 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",
> > >>
> > >> _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
signature.asc
Description: PGP signature
Re: [PATCH v2 01/11] accel/kvm: Check MachineClass kvm_type() return value
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é wrote:
> >
> > > On 2/22/21 6:24 PM, Cornelia Huck wrote:
> > > > On Fri, 19 Feb 2021 18:38:37 +0100
> > > > Philippe Mathieu-Daudé wrote:
> > > >
> > > >> MachineClass::kvm_type() can return -1 on failure.
> > > >> Document it, and add a check in kvm_init().
> > > >>
> > > >> Signed-off-by: Philippe Mathieu-Daudé
> > > >> ---
> > > >> 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",
> > > >>
> > > >> _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
signature.asc
Description: PGP signature
Re: [PATCH v2 01/11] accel/kvm: Check MachineClass kvm_type() return value
On 2/22/21 6:50 PM, Cornelia Huck wrote:
> On Mon, 22 Feb 2021 18:41:07 +0100
> Philippe Mathieu-Daudé wrote:
>
>> On 2/22/21 6:24 PM, Cornelia Huck wrote:
>>> On Fri, 19 Feb 2021 18:38:37 +0100
>>> Philippe Mathieu-Daudé wrote:
>>>
MachineClass::kvm_type() can return -1 on failure.
Document it, and add a check in kvm_init().
Signed-off-by: Philippe Mathieu-Daudé
---
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",
_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-cc768fc1b...@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
On Mon, 22 Feb 2021 18:41:07 +0100
Philippe Mathieu-Daudé wrote:
> On 2/22/21 6:24 PM, Cornelia Huck wrote:
> > On Fri, 19 Feb 2021 18:38:37 +0100
> > Philippe Mathieu-Daudé wrote:
> >
> >> MachineClass::kvm_type() can return -1 on failure.
> >> Document it, and add a check in kvm_init().
> >>
> >> Signed-off-by: Philippe Mathieu-Daudé
> >> ---
> >> 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",
> >> _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
On 2/22/21 6:24 PM, Cornelia Huck wrote:
> On Fri, 19 Feb 2021 18:38:37 +0100
> Philippe Mathieu-Daudé wrote:
>
>> MachineClass::kvm_type() can return -1 on failure.
>> Document it, and add a check in kvm_init().
>>
>> Signed-off-by: Philippe Mathieu-Daudé
>> ---
>> 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",
>> _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
On Fri, 19 Feb 2021 18:38:37 +0100
Philippe Mathieu-Daudé wrote:
> MachineClass::kvm_type() can return -1 on failure.
> Document it, and add a check in kvm_init().
>
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> 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",
> _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?
[PATCH v2 01/11] accel/kvm: Check MachineClass kvm_type() return value
MachineClass::kvm_type() can return -1 on failure.
Document it, and add a check in kvm_init().
Signed-off-by: Philippe Mathieu-Daudé
---
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",
_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
