| Message ID | 20190923121712.22971-1-thuth@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Disallow colons in the parameter of "-accel" | expand |
On Mon, 23 Sep 2019 at 13:21, Thomas Huth <thuth@redhat.com> wrote: > > Everybody who used something like "-machine accel=kvm:tcg" in the past > might be tempted to specify a similar list with the -accel parameter, > too, for example "-accel kvm:tcg". However, this is not how this > options is thought to be used, since each "-accel" should only take care > of one specific accelerator. > > In the long run, we really should rework the "-accel" code completely, > so that it does not set "-machine accel=..." anymore internally, but > is completely independent from "-machine". For the short run, let's > make sure that users cannot use "-accel xyz:tcg", so that we avoid > that we have to deal with such cases in the wild later. > > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- > vl.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/vl.c b/vl.c > index 630f5c5e9c..68f47a9c25 100644 > --- a/vl.c > +++ b/vl.c > @@ -3554,6 +3554,11 @@ int main(int argc, char **argv, char **envp) > g_slist_free(accel_list); > exit(0); > } > + if (optarg && strchr(optarg, ':')) { > + error_report("Don't use ':' with -accel, " > + "use -M accel=... in this case instead"); > + exit(1); > + } This seems pretty ugly. If -accel is the way we're recommending users configure the accelerator then it should support syntax for specifying everything we could do with the old -machine... option. If it can't do that yet, we shouldn't switch over to it until it can. thanks -- PMM
On 23/09/2019 14.23, Peter Maydell wrote: > On Mon, 23 Sep 2019 at 13:21, Thomas Huth <thuth@redhat.com> wrote: >> >> Everybody who used something like "-machine accel=kvm:tcg" in the past >> might be tempted to specify a similar list with the -accel parameter, >> too, for example "-accel kvm:tcg". However, this is not how this >> options is thought to be used, since each "-accel" should only take care >> of one specific accelerator. >> >> In the long run, we really should rework the "-accel" code completely, >> so that it does not set "-machine accel=..." anymore internally, but >> is completely independent from "-machine". For the short run, let's >> make sure that users cannot use "-accel xyz:tcg", so that we avoid >> that we have to deal with such cases in the wild later. >> >> Signed-off-by: Thomas Huth <thuth@redhat.com> >> --- >> vl.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/vl.c b/vl.c >> index 630f5c5e9c..68f47a9c25 100644 >> --- a/vl.c >> +++ b/vl.c >> @@ -3554,6 +3554,11 @@ int main(int argc, char **argv, char **envp) >> g_slist_free(accel_list); >> exit(0); >> } >> + if (optarg && strchr(optarg, ':')) { >> + error_report("Don't use ':' with -accel, " >> + "use -M accel=... in this case instead"); >> + exit(1); >> + } > > This seems pretty ugly. Yes. The whole "-accel" option is currently ugly. My patch is just a temporary work-around to prevent that we later have to deal with the fact that users started to use this colon here in the wild and we would then have to fight to get rid of it again. > If -accel is the way we're recommending > users configure the accelerator then it should support syntax > for specifying everything we could do with the old -machine... > option. No, we certainly don't want to have the colon in here. The idea is rather that you could specify multiply "-accel" options one day, e.g.: -accel tcg,tb-size=2048 -accel kvm,kernel_irqchip=on ... and then the accelators are used with the right parameters in the order of availability. Per-accelerator parameters just don't work here if you allow the colon. (IMHO we should not have let the -accel code enter the repository in this shape, but rather insist on a proper implementation right from the start - but now that it's there, we have to deal with it and should make sure that it does not get worse) Thomas
On Mon, 23 Sep 2019 at 13:42, Thomas Huth <thuth@redhat.com> wrote: > > On 23/09/2019 14.23, Peter Maydell wrote: > > This seems pretty ugly. > > Yes. The whole "-accel" option is currently ugly. My patch is just a > temporary work-around to prevent that we later have to deal with the > fact that users started to use this colon here in the wild and we would > then have to fight to get rid of it again. > > > If -accel is the way we're recommending > > users configure the accelerator then it should support syntax > > for specifying everything we could do with the old -machine... > > option. > > No, we certainly don't want to have the colon in here. The idea is > rather that you could specify multiply "-accel" options one day, e.g.: > > -accel tcg,tb-size=2048 -accel kvm,kernel_irqchip=on > > ... and then the accelators are used with the right parameters in the > order of availability. > > Per-accelerator parameters just don't work here if you allow the colon. > > (IMHO we should not have let the -accel code enter the repository in > this shape, but rather insist on a proper implementation right from the > start - but now that it's there, we have to deal with it and should make > sure that it does not get worse) Ah, I hadn't realised that -accel was a (relatively) long-standing option; I'd just noticed some patches going past recently suggesting we were starting to recommend it over -machine accel=. Thanks for trying to clean it up a bit. -- PMM
On 23/09/19 14:17, Thomas Huth wrote: > Everybody who used something like "-machine accel=kvm:tcg" in the past > might be tempted to specify a similar list with the -accel parameter, > too, for example "-accel kvm:tcg". However, this is not how this > options is thought to be used, since each "-accel" should only take care > of one specific accelerator. > > In the long run, we really should rework the "-accel" code completely, > so that it does not set "-machine accel=..." anymore internally, but > is completely independent from "-machine". For the short run, let's > make sure that users cannot use "-accel xyz:tcg", so that we avoid > that we have to deal with such cases in the wild later. > > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- > vl.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/vl.c b/vl.c > index 630f5c5e9c..68f47a9c25 100644 > --- a/vl.c > +++ b/vl.c > @@ -3554,6 +3554,11 @@ int main(int argc, char **argv, char **envp) > g_slist_free(accel_list); > exit(0); > } > + if (optarg && strchr(optarg, ':')) { > + error_report("Don't use ':' with -accel, " > + "use -M accel=... in this case instead"); s/in this case/for now/ or something like that? Thanks, Paolo > + exit(1); > + } > opts = qemu_opts_create(qemu_find_opts("machine"), NULL, > false, &error_abort); > qemu_opt_set(opts, "accel", optarg, &error_abort); >
diff --git a/vl.c b/vl.c index 630f5c5e9c..68f47a9c25 100644 --- a/vl.c +++ b/vl.c @@ -3554,6 +3554,11 @@ int main(int argc, char **argv, char **envp) g_slist_free(accel_list); exit(0); } + if (optarg && strchr(optarg, ':')) { + error_report("Don't use ':' with -accel, " + "use -M accel=... in this case instead"); + exit(1); + } opts = qemu_opts_create(qemu_find_opts("machine"), NULL, false, &error_abort); qemu_opt_set(opts, "accel", optarg, &error_abort);
Everybody who used something like "-machine accel=kvm:tcg" in the past might be tempted to specify a similar list with the -accel parameter, too, for example "-accel kvm:tcg". However, this is not how this options is thought to be used, since each "-accel" should only take care of one specific accelerator. In the long run, we really should rework the "-accel" code completely, so that it does not set "-machine accel=..." anymore internally, but is completely independent from "-machine". For the short run, let's make sure that users cannot use "-accel xyz:tcg", so that we avoid that we have to deal with such cases in the wild later. Signed-off-by: Thomas Huth <thuth@redhat.com> --- vl.c | 5 +++++ 1 file changed, 5 insertions(+)