| Message ID | 20230620074802.86898-1-philmd@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | hw/ppc/spapr: Test whether TCG is enabled with tcg_enabled() | expand |
On 6/20/23 09:48, Philippe Mathieu-Daudé wrote: > Although the PPC target only supports the TCG and KVM > accelerators, QEMU supports more. We can no assume that > '!kvm == tcg', so test for the correct accelerator. This > also eases code review, because here we don't care about > KVM, we really want to test for TCG. > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> I don't remember anymore, but what about qtest ? It is usually the forgotten case in these kind of tests... so much complexity :-) Ciao, Claudio > --- > hw/ppc/spapr.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index dcb7f1c70a..c4b666587b 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -2524,7 +2524,7 @@ static void spapr_set_vsmt_mode(SpaprMachineState *spapr, Error **errp) > int ret; > unsigned int smp_threads = ms->smp.threads; > > - if (!kvm_enabled() && (smp_threads > 1)) { > + if (tcg_enabled() && (smp_threads > 1)) { > error_setg(errp, "TCG cannot support more than 1 thread/core " > "on a pseries machine"); > return;
On 6/20/23 13:18, Philippe Mathieu-Daudé wrote: > Although the PPC target only supports the TCG and KVM > accelerators, QEMU supports more. We can no assume that > '!kvm == tcg', so test for the correct accelerator. This > also eases code review, because here we don't care about > KVM, we really want to test for TCG. > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > --- > hw/ppc/spapr.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index dcb7f1c70a..c4b666587b 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -2524,7 +2524,7 @@ static void spapr_set_vsmt_mode(SpaprMachineState *spapr, Error **errp) > int ret; > unsigned int smp_threads = ms->smp.threads; > > - if (!kvm_enabled() && (smp_threads > 1)) { > + if (tcg_enabled() && (smp_threads > 1)) { > error_setg(errp, "TCG cannot support more than 1 thread/core " > "on a pseries machine"); Reviewed-by: Harsh Prateek Bora <harshpb@linux.ibm.com> > return;
On 6/20/23 09:48, Philippe Mathieu-Daudé wrote: > Although the PPC target only supports the TCG and KVM > accelerators, QEMU supports more. We can no assume that > '!kvm == tcg', so test for the correct accelerator. This > also eases code review, because here we don't care about > KVM, we really want to test for TCG. > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> Reviewed-by: Cédric Le Goater <clg@kaod.org> > --- > hw/ppc/spapr.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index dcb7f1c70a..c4b666587b 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -2524,7 +2524,7 @@ static void spapr_set_vsmt_mode(SpaprMachineState *spapr, Error **errp) > int ret; > unsigned int smp_threads = ms->smp.threads; > > - if (!kvm_enabled() && (smp_threads > 1)) { > + if (tcg_enabled() && (smp_threads > 1)) { > error_setg(errp, "TCG cannot support more than 1 thread/core " > "on a pseries machine"); > return; Nick, You might want to include this patch in the series adding SMT support to pseries/spapr : https://patchew.org/QEMU/20230605112323.179259-1-npiggin@gmail.com/ C.
On Tue, Jun 20, 2023 at 09:48:02AM +0200, Philippe Mathieu-Daudé wrote: > Although the PPC target only supports the TCG and KVM > accelerators, QEMU supports more. We can no assume that > '!kvm == tcg', so test for the correct accelerator. This > also eases code review, because here we don't care about > KVM, we really want to test for TCG. > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> Reviewed-by: David Gibson <david@gibson.dropbear.id.au> > --- > hw/ppc/spapr.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index dcb7f1c70a..c4b666587b 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -2524,7 +2524,7 @@ static void spapr_set_vsmt_mode(SpaprMachineState *spapr, Error **errp) > int ret; > unsigned int smp_threads = ms->smp.threads; > > - if (!kvm_enabled() && (smp_threads > 1)) { > + if (tcg_enabled() && (smp_threads > 1)) { > error_setg(errp, "TCG cannot support more than 1 thread/core " > "on a pseries machine"); > return;
On Tue, 20 Jun 2023, Philippe Mathieu-Daudé wrote: > Although the PPC target only supports the TCG and KVM > accelerators, QEMU supports more. We can no assume that Typo: -> can not assume Regards, BALATON Zoltan > '!kvm == tcg', so test for the correct accelerator. This > also eases code review, because here we don't care about > KVM, we really want to test for TCG. > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > --- > hw/ppc/spapr.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index dcb7f1c70a..c4b666587b 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -2524,7 +2524,7 @@ static void spapr_set_vsmt_mode(SpaprMachineState *spapr, Error **errp) > int ret; > unsigned int smp_threads = ms->smp.threads; > > - if (!kvm_enabled() && (smp_threads > 1)) { > + if (tcg_enabled() && (smp_threads > 1)) { > error_setg(errp, "TCG cannot support more than 1 thread/core " > "on a pseries machine"); > return; >
On Tue, 20 Jun 2023 09:55:49 +0200 Claudio Fontana <cfontana@suse.de> wrote: > On 6/20/23 09:48, Philippe Mathieu-Daudé wrote: > > Although the PPC target only supports the TCG and KVM > > accelerators, QEMU supports more. We can no assume that > > '!kvm == tcg', so test for the correct accelerator. This > > also eases code review, because here we don't care about > > KVM, we really want to test for TCG. > > > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > > I don't remember anymore, but what about qtest ? It is usually the forgotten case in these kind of tests... so much complexity :-) > This check was added with TCG in mind because it is a known limitation. I don't see any reason to prevent qtest from being used with the rest of this function though. > Ciao, > > Claudio > > > > --- > > hw/ppc/spapr.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > > index dcb7f1c70a..c4b666587b 100644 > > --- a/hw/ppc/spapr.c > > +++ b/hw/ppc/spapr.c > > @@ -2524,7 +2524,7 @@ static void spapr_set_vsmt_mode(SpaprMachineState *spapr, Error **errp) > > int ret; > > unsigned int smp_threads = ms->smp.threads; > > > > - if (!kvm_enabled() && (smp_threads > 1)) { > > + if (tcg_enabled() && (smp_threads > 1)) { Bonjour Philippe, Please drop the unneeded parens in the second check. With this fixed, Reviewed-by: Greg Kurz <groug@kaod.org> Cheers, -- Greg > > error_setg(errp, "TCG cannot support more than 1 thread/core " > > "on a pseries machine"); > > return; >
On 20/6/23 16:48, Greg Kurz wrote: > On Tue, 20 Jun 2023 09:55:49 +0200 > Claudio Fontana <cfontana@suse.de> wrote: > >> On 6/20/23 09:48, Philippe Mathieu-Daudé wrote: >>> Although the PPC target only supports the TCG and KVM >>> accelerators, QEMU supports more. We can no assume that >>> '!kvm == tcg', so test for the correct accelerator. This >>> also eases code review, because here we don't care about >>> KVM, we really want to test for TCG. >>> >>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >> >> I don't remember anymore, but what about qtest ? It is usually the forgotten case in these kind of tests... so much complexity :-) Good to think about this, since such changes indeed usually break, qtests :) > This check was added with TCG in mind because it is a known limitation. > I don't see any reason to prevent qtest from being used with the rest > of this function though. We don't have any CPU core when using qtest accelerator, so this check is irrelevant IMHO (not reachable with '-accel qtest'). >>> --- >>> hw/ppc/spapr.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c >>> index dcb7f1c70a..c4b666587b 100644 >>> --- a/hw/ppc/spapr.c >>> +++ b/hw/ppc/spapr.c >>> @@ -2524,7 +2524,7 @@ static void spapr_set_vsmt_mode(SpaprMachineState *spapr, Error **errp) >>> int ret; >>> unsigned int smp_threads = ms->smp.threads; >>> >>> - if (!kvm_enabled() && (smp_threads > 1)) { >>> + if (tcg_enabled() && (smp_threads > 1)) { > > Bonjour Philippe, > > Please drop the unneeded parens in the second check. I wanted to do that but then thought someone would ask me to do one change at once ;) > With this fixed, > > Reviewed-by: Greg Kurz <groug@kaod.org> Thanks!
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index dcb7f1c70a..c4b666587b 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -2524,7 +2524,7 @@ static void spapr_set_vsmt_mode(SpaprMachineState *spapr, Error **errp) int ret; unsigned int smp_threads = ms->smp.threads; - if (!kvm_enabled() && (smp_threads > 1)) { + if (tcg_enabled() && (smp_threads > 1)) { error_setg(errp, "TCG cannot support more than 1 thread/core " "on a pseries machine"); return;
Although the PPC target only supports the TCG and KVM accelerators, QEMU supports more. We can no assume that '!kvm == tcg', so test for the correct accelerator. This also eases code review, because here we don't care about KVM, we really want to test for TCG. Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- hw/ppc/spapr.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)