* [PATCH v3 0/1] spapr.c: set a 'kvm-type' default value instead of relying on NULL
@ 2020-12-10 14:55 Daniel Henrique Barboza
2020-12-10 14:55 ` [PATCH v3 1/1] " Daniel Henrique Barboza
2020-12-10 16:51 ` [PATCH v3 0/1] " Paolo Bonzini
0 siblings, 2 replies; 6+ messages in thread
From: Daniel Henrique Barboza @ 2020-12-10 14:55 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, Daniel Henrique Barboza, qemu-ppc, groug, david
changes from v2, all proposed by Greg:
* Handle 'NULL' value as default mode fallback in spapr_kvm_type()
* Do not allow for 'AUTO' to be a valid mode in spapr_kvm_type()
* Initialize 'spapr->kvm_type' in spapr_instance_init() like Paolo
proposed. This will spare us from changing spapr_get_kvm_type()
altogether.
v2 link: https://lists.gnu.org/archive/html/qemu-devel/2020-12/msg02623.html
This patch addresses an issue that happens with the pseries machine after
testing Paolo's patch [1]:
$ sudo ./ppc64-softmmu/qemu-system-ppc64 -nographic -nodefaults -machine pseries --enable-kvm
qemu-system-ppc64: Unknown kvm-type specified ''
The reason lies on how qemu_opt_get() and object_property_get_str() works
when there is no 'kvm-type' specified. We were conting on receiving NULL
for kvm-type, but the latter will use a blank string "". Instead on relying
on NULL, let's expose the already existing 'auto' kvm-type mode to the users
and use that as default.
[1] https://lists.gnu.org/archive/html/qemu-devel/2020-12/msg00471.html
Daniel Henrique Barboza (1):
spapr.c: set a 'kvm-type' default value instead of relying on NULL
hw/ppc/spapr.c | 21 +++++++++++++++++----
1 file changed, 17 insertions(+), 4 deletions(-)
--
2.26.2
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v3 1/1] spapr.c: set a 'kvm-type' default value instead of relying on NULL
2020-12-10 14:55 [PATCH v3 0/1] spapr.c: set a 'kvm-type' default value instead of relying on NULL Daniel Henrique Barboza
@ 2020-12-10 14:55 ` Daniel Henrique Barboza
2020-12-10 15:13 ` Greg Kurz
2020-12-10 16:51 ` [PATCH v3 0/1] " Paolo Bonzini
1 sibling, 1 reply; 6+ messages in thread
From: Daniel Henrique Barboza @ 2020-12-10 14:55 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, Daniel Henrique Barboza, qemu-ppc, groug, david
spapr_kvm_type() is considering 'vm_type=NULL' as a valid input, where
the function returns 0. This is relying on the current QEMU machine
options handling logic, where the absence of the 'kvm-type' option
will be reflected as 'vm_type=NULL' in this function.
This is not robust, and will break if QEMU options code decides to propagate
something else in the case mentioned above (e.g. an empty string instead
of NULL).
Let's avoid this entirely by setting a non-NULL default value in case of
no user input for 'kvm-type'. spapr_kvm_type() was changed to handle 3 fixed
values of kvm-type: "auto", "hv", and "pr", with "auto" being the default
if no kvm-type was set by the user. This allows us to always be predictable
regardless of any enhancements/changes made in QEMU options mechanics.
While we're at it, let's also document in 'kvm-type' description the
already existing default mode, now named 'auto'. The information provided
about it is based on how the pseries kernel handles the KVM_CREATE_VM
ioctl(), where the default value '0' makes the kernel choose an available
KVM module to use, giving precedence to kvm_hv. This logic is described in
the kernel source file arch/powerpc/kvm/powerpc.c, function kvm_arch_init_vm().
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
hw/ppc/spapr.c | 21 +++++++++++++++++----
1 file changed, 17 insertions(+), 4 deletions(-)
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index b7e0894019..877bd264ce 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3021,17 +3021,25 @@ static void spapr_machine_init(MachineState *machine)
qemu_cond_init(&spapr->fwnmi_machine_check_interlock_cond);
}
+#define DEFAULT_KVM_TYPE "auto"
static int spapr_kvm_type(MachineState *machine, const char *vm_type)
{
- if (!vm_type) {
+ /*
+ * The use of g_ascii_strcasecmp() for 'hv' and 'pr' is to
+ * accomodate the 'HV' and 'PV' formats that exists in the
+ * wild. The 'auto' mode is being introduced already as
+ * lower-case, thus we don't need to bother checking for
+ * "AUTO".
+ */
+ if (!vm_type || !strcmp(vm_type, DEFAULT_KVM_TYPE)) {
return 0;
}
- if (!strcmp(vm_type, "HV")) {
+ if (!g_ascii_strcasecmp(vm_type, "hv")) {
return 1;
}
- if (!strcmp(vm_type, "PR")) {
+ if (!g_ascii_strcasecmp(vm_type, "pr")) {
return 2;
}
@@ -3270,10 +3278,15 @@ static void spapr_instance_init(Object *obj)
spapr->htab_fd = -1;
spapr->use_hotplug_event_source = true;
+ spapr->kvm_type = g_strdup(DEFAULT_KVM_TYPE);
object_property_add_str(obj, "kvm-type",
spapr_get_kvm_type, spapr_set_kvm_type);
object_property_set_description(obj, "kvm-type",
- "Specifies the KVM virtualization mode (HV, PR)");
+ "Specifies the KVM virtualization mode (auto,"
+ " hv, pr). Defaults to 'auto'. This mode will use"
+ " any available KVM module loaded in the host,"
+ " where kvm_hv takes precedence if both kvm_hv and"
+ " kvm_pr are loaded.");
object_property_add_bool(obj, "modern-hotplug-events",
spapr_get_modern_hotplug_events,
spapr_set_modern_hotplug_events);
--
2.26.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v3 1/1] spapr.c: set a 'kvm-type' default value instead of relying on NULL
2020-12-10 14:55 ` [PATCH v3 1/1] " Daniel Henrique Barboza
@ 2020-12-10 15:13 ` Greg Kurz
0 siblings, 0 replies; 6+ messages in thread
From: Greg Kurz @ 2020-12-10 15:13 UTC (permalink / raw)
To: Daniel Henrique Barboza; +Cc: pbonzini, qemu-ppc, qemu-devel, david
On Thu, 10 Dec 2020 11:55:17 -0300
Daniel Henrique Barboza <danielhb413@gmail.com> wrote:
> spapr_kvm_type() is considering 'vm_type=NULL' as a valid input, where
> the function returns 0. This is relying on the current QEMU machine
> options handling logic, where the absence of the 'kvm-type' option
> will be reflected as 'vm_type=NULL' in this function.
>
> This is not robust, and will break if QEMU options code decides to propagate
> something else in the case mentioned above (e.g. an empty string instead
> of NULL).
>
> Let's avoid this entirely by setting a non-NULL default value in case of
> no user input for 'kvm-type'. spapr_kvm_type() was changed to handle 3 fixed
> values of kvm-type: "auto", "hv", and "pr", with "auto" being the default
> if no kvm-type was set by the user. This allows us to always be predictable
> regardless of any enhancements/changes made in QEMU options mechanics.
>
> While we're at it, let's also document in 'kvm-type' description the
> already existing default mode, now named 'auto'. The information provided
> about it is based on how the pseries kernel handles the KVM_CREATE_VM
> ioctl(), where the default value '0' makes the kernel choose an available
> KVM module to use, giving precedence to kvm_hv. This logic is described in
> the kernel source file arch/powerpc/kvm/powerpc.c, function kvm_arch_init_vm().
>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
Reviewed-by: Greg Kurz <groug@kaod.org>
> hw/ppc/spapr.c | 21 +++++++++++++++++----
> 1 file changed, 17 insertions(+), 4 deletions(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index b7e0894019..877bd264ce 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3021,17 +3021,25 @@ static void spapr_machine_init(MachineState *machine)
> qemu_cond_init(&spapr->fwnmi_machine_check_interlock_cond);
> }
>
> +#define DEFAULT_KVM_TYPE "auto"
> static int spapr_kvm_type(MachineState *machine, const char *vm_type)
> {
> - if (!vm_type) {
> + /*
> + * The use of g_ascii_strcasecmp() for 'hv' and 'pr' is to
> + * accomodate the 'HV' and 'PV' formats that exists in the
> + * wild. The 'auto' mode is being introduced already as
> + * lower-case, thus we don't need to bother checking for
> + * "AUTO".
> + */
> + if (!vm_type || !strcmp(vm_type, DEFAULT_KVM_TYPE)) {
> return 0;
> }
>
> - if (!strcmp(vm_type, "HV")) {
> + if (!g_ascii_strcasecmp(vm_type, "hv")) {
> return 1;
> }
>
> - if (!strcmp(vm_type, "PR")) {
> + if (!g_ascii_strcasecmp(vm_type, "pr")) {
> return 2;
> }
>
> @@ -3270,10 +3278,15 @@ static void spapr_instance_init(Object *obj)
>
> spapr->htab_fd = -1;
> spapr->use_hotplug_event_source = true;
> + spapr->kvm_type = g_strdup(DEFAULT_KVM_TYPE);
> object_property_add_str(obj, "kvm-type",
> spapr_get_kvm_type, spapr_set_kvm_type);
> object_property_set_description(obj, "kvm-type",
> - "Specifies the KVM virtualization mode (HV, PR)");
> + "Specifies the KVM virtualization mode (auto,"
> + " hv, pr). Defaults to 'auto'. This mode will use"
> + " any available KVM module loaded in the host,"
> + " where kvm_hv takes precedence if both kvm_hv and"
> + " kvm_pr are loaded.");
> object_property_add_bool(obj, "modern-hotplug-events",
> spapr_get_modern_hotplug_events,
> spapr_set_modern_hotplug_events);
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3 0/1] spapr.c: set a 'kvm-type' default value instead of relying on NULL
2020-12-10 14:55 [PATCH v3 0/1] spapr.c: set a 'kvm-type' default value instead of relying on NULL Daniel Henrique Barboza
2020-12-10 14:55 ` [PATCH v3 1/1] " Daniel Henrique Barboza
@ 2020-12-10 16:51 ` Paolo Bonzini
2020-12-11 0:21 ` David Gibson
1 sibling, 1 reply; 6+ messages in thread
From: Paolo Bonzini @ 2020-12-10 16:51 UTC (permalink / raw)
To: Daniel Henrique Barboza, qemu-devel; +Cc: qemu-ppc, groug, david
On 10/12/20 15:55, Daniel Henrique Barboza wrote:
> changes from v2, all proposed by Greg:
> * Handle 'NULL' value as default mode fallback in spapr_kvm_type()
> * Do not allow for 'AUTO' to be a valid mode in spapr_kvm_type()
> * Initialize 'spapr->kvm_type' in spapr_instance_init() like Paolo
> proposed. This will spare us from changing spapr_get_kvm_type()
> altogether.
> v2 link: https://lists.gnu.org/archive/html/qemu-devel/2020-12/msg02623.html
>
>
> This patch addresses an issue that happens with the pseries machine after
> testing Paolo's patch [1]:
>
> $ sudo ./ppc64-softmmu/qemu-system-ppc64 -nographic -nodefaults -machine pseries --enable-kvm
> qemu-system-ppc64: Unknown kvm-type specified ''
>
> The reason lies on how qemu_opt_get() and object_property_get_str() works
> when there is no 'kvm-type' specified. We were conting on receiving NULL
> for kvm-type, but the latter will use a blank string "". Instead on relying
> on NULL, let's expose the already existing 'auto' kvm-type mode to the users
> and use that as default.
>
> [1] https://lists.gnu.org/archive/html/qemu-devel/2020-12/msg00471.html
>
> Daniel Henrique Barboza (1):
> spapr.c: set a 'kvm-type' default value instead of relying on NULL
>
> hw/ppc/spapr.c | 21 +++++++++++++++++----
> 1 file changed, 17 insertions(+), 4 deletions(-)
>
Will queue, thanks!
Paolo
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3 0/1] spapr.c: set a 'kvm-type' default value instead of relying on NULL
2020-12-10 16:51 ` [PATCH v3 0/1] " Paolo Bonzini
@ 2020-12-11 0:21 ` David Gibson
2020-12-11 15:46 ` Paolo Bonzini
0 siblings, 1 reply; 6+ messages in thread
From: David Gibson @ 2020-12-11 0:21 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: Daniel Henrique Barboza, qemu-ppc, qemu-devel, groug
[-- Attachment #1: Type: text/plain, Size: 1904 bytes --]
On Thu, Dec 10, 2020 at 05:51:41PM +0100, Paolo Bonzini wrote:
> On 10/12/20 15:55, Daniel Henrique Barboza wrote:
> > changes from v2, all proposed by Greg:
> > * Handle 'NULL' value as default mode fallback in spapr_kvm_type()
> > * Do not allow for 'AUTO' to be a valid mode in spapr_kvm_type()
> > * Initialize 'spapr->kvm_type' in spapr_instance_init() like Paolo
> > proposed. This will spare us from changing spapr_get_kvm_type()
> > altogether.
> > v2 link: https://lists.gnu.org/archive/html/qemu-devel/2020-12/msg02623.html
> >
> >
> > This patch addresses an issue that happens with the pseries machine after
> > testing Paolo's patch [1]:
> >
> > $ sudo ./ppc64-softmmu/qemu-system-ppc64 -nographic -nodefaults -machine pseries --enable-kvm
> > qemu-system-ppc64: Unknown kvm-type specified ''
> >
> > The reason lies on how qemu_opt_get() and object_property_get_str() works
> > when there is no 'kvm-type' specified. We were conting on receiving NULL
> > for kvm-type, but the latter will use a blank string "". Instead on relying
> > on NULL, let's expose the already existing 'auto' kvm-type mode to the users
> > and use that as default.
> >
> > [1] https://lists.gnu.org/archive/html/qemu-devel/2020-12/msg00471.html
> >
> > Daniel Henrique Barboza (1):
> > spapr.c: set a 'kvm-type' default value instead of relying on NULL
> >
> > hw/ppc/spapr.c | 21 +++++++++++++++++----
> > 1 file changed, 17 insertions(+), 4 deletions(-)
> >
>
> Will queue, thanks!
I've also put it into my ppc-for-6.0 tree, which I plan to send a PR
for shortly. I guess it should be an easy conflict to resolve, so I
don't think that will be a problem.
--
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 --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3 0/1] spapr.c: set a 'kvm-type' default value instead of relying on NULL
2020-12-11 0:21 ` David Gibson
@ 2020-12-11 15:46 ` Paolo Bonzini
0 siblings, 0 replies; 6+ messages in thread
From: Paolo Bonzini @ 2020-12-11 15:46 UTC (permalink / raw)
To: David Gibson; +Cc: Daniel Henrique Barboza, qemu-ppc, qemu-devel, groug
On 11/12/20 01:21, David Gibson wrote:
> On Thu, Dec 10, 2020 at 05:51:41PM +0100, Paolo Bonzini wrote:
>> On 10/12/20 15:55, Daniel Henrique Barboza wrote:
>>> changes from v2, all proposed by Greg:
>>> * Handle 'NULL' value as default mode fallback in spapr_kvm_type()
>>> * Do not allow for 'AUTO' to be a valid mode in spapr_kvm_type()
>>> * Initialize 'spapr->kvm_type' in spapr_instance_init() like Paolo
>>> proposed. This will spare us from changing spapr_get_kvm_type()
>>> altogether.
>>> v2 link: https://lists.gnu.org/archive/html/qemu-devel/2020-12/msg02623.html
>>>
>>>
>>> This patch addresses an issue that happens with the pseries machine after
>>> testing Paolo's patch [1]:
>>>
>>> $ sudo ./ppc64-softmmu/qemu-system-ppc64 -nographic -nodefaults -machine pseries --enable-kvm
>>> qemu-system-ppc64: Unknown kvm-type specified ''
>>>
>>> The reason lies on how qemu_opt_get() and object_property_get_str() works
>>> when there is no 'kvm-type' specified. We were conting on receiving NULL
>>> for kvm-type, but the latter will use a blank string "". Instead on relying
>>> on NULL, let's expose the already existing 'auto' kvm-type mode to the users
>>> and use that as default.
>>>
>>> [1] https://lists.gnu.org/archive/html/qemu-devel/2020-12/msg00471.html
>>>
>>> Daniel Henrique Barboza (1):
>>> spapr.c: set a 'kvm-type' default value instead of relying on NULL
>>>
>>> hw/ppc/spapr.c | 21 +++++++++++++++++----
>>> 1 file changed, 17 insertions(+), 4 deletions(-)
>>>
>>
>> Will queue, thanks!
>
> I've also put it into my ppc-for-6.0 tree, which I plan to send a PR
> for shortly. I guess it should be an easy conflict to resolve, so I
> don't think that will be a problem.
Yup, my own queue is still a week away, so go ahead. Thanks!
Paolo
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-12-11 15:53 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-10 14:55 [PATCH v3 0/1] spapr.c: set a 'kvm-type' default value instead of relying on NULL Daniel Henrique Barboza
2020-12-10 14:55 ` [PATCH v3 1/1] " Daniel Henrique Barboza
2020-12-10 15:13 ` Greg Kurz
2020-12-10 16:51 ` [PATCH v3 0/1] " Paolo Bonzini
2020-12-11 0:21 ` David Gibson
2020-12-11 15:46 ` Paolo Bonzini
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).