Message ID | 20230622135700.105383-17-dbarboza@ventanamicro.com |
---|---|
State | New |
Headers | show |
Series | target/riscv, KVM: fixes and enhancements | expand |
On Thu, Jun 22, 2023 at 10:56:57AM -0300, Daniel Henrique Barboza wrote: > KVM-specific properties are being created inside target/riscv/kvm.c. But > at this moment we're gathering all the remaining properties from TCG and > adding them as is when running KVM. This creates a situation where > non-KVM properties are setting flags to 'true' due to its default > settings (e.g. Zawrs). Users can also freely enable them via command > line. > > This doesn't impact runtime per se because KVM doesn't care about these > flags, but code such as riscv_isa_string_ext() take those flags into > account. The result is that, for a KVM guest, setting non-KVM properties > will make them appear in the riscv,isa DT. > > We want to keep the same API for both TCG and KVM and at the same time, > when running KVM, forbid non-KVM extensions to be enabled internally. We > accomplish both by changing riscv_cpu_add_user_properties() to add a > mock/no-op boolean property for every non-KVM extension in > riscv_cpu_extensions[]. Then, when running KVM, users are still free to > set extensions at will, we'll treat non-KVM extensions as a no-op, and > riscv_isa_string_ext() will not report bogus extensions in the DT. > > Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> > --- > target/riscv/cpu.c | 36 +++++++++++++++++++++++++++++++++--- > 1 file changed, 33 insertions(+), 3 deletions(-) > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c > index b65db165cc..f5209f0789 100644 > --- a/target/riscv/cpu.c > +++ b/target/riscv/cpu.c > @@ -1720,6 +1720,18 @@ static Property riscv_cpu_extensions[] = { > DEFINE_PROP_END_OF_LIST(), > }; > > + > +static void cpu_set_cfg_noop(Object *obj, Visitor *v, > + const char *name, > + void *opaque, Error **errp) > +{ > + bool value; > + > + if (!visit_type_bool(v, name, &value, errp)) { > + return; > + } > +} > + > /* > * Add CPU properties with user-facing flags. > * > @@ -1738,9 +1750,27 @@ static void riscv_cpu_add_user_properties(Object *obj) > riscv_cpu_add_misa_properties(obj); > > for (prop = riscv_cpu_extensions; prop && prop->name; prop++) { > - /* Check if KVM didn't create the property already */ > - if (object_property_find(obj, prop->name)) { > - continue; > + if (riscv_running_kvm()) { > + /* Check if KVM didn't create the property already */ > + if (object_property_find(obj, prop->name)) { > + continue; > + } > + > + /* > + * Set every multi-letter extension that KVM doesn't > + * know as a no-op. This will allow users to set values > + * to them while keeping their internal state to 'false'. > + * > + * We're giving a pass for non-bool properties since they're > + * not related to the availability of extensions and can be > + * safely ignored as is. > + */ > + if (prop->info == &qdev_prop_bool) { > + object_property_add(obj, prop->name, "bool", > + NULL, cpu_set_cfg_noop, > + NULL, NULL); > + continue; > + } > } > > qdev_property_add_static(dev, prop); > -- > 2.41.0 > I think we should actually fail with an error when the user tries to enable an extension KVM doesn't support. Otherwise a user may be confused as to why their Zawrs=on didn't provide them a machine with Zawrs. And, when KVM learns how to provide that support to guests (Zawrs is actually on my TODO...), then migrating the same VM to later KVM/QEMU will actually enable the feature, possibly confusing the guest. So, we should probably just not add any extension properties to KVM guests which can't be enabled. Then, as we add support to KVM, we'll add the properties too. Thanks, drew
On 6/23/23 06:58, Andrew Jones wrote: > On Thu, Jun 22, 2023 at 10:56:57AM -0300, Daniel Henrique Barboza wrote: >> KVM-specific properties are being created inside target/riscv/kvm.c. But >> at this moment we're gathering all the remaining properties from TCG and >> adding them as is when running KVM. This creates a situation where >> non-KVM properties are setting flags to 'true' due to its default >> settings (e.g. Zawrs). Users can also freely enable them via command >> line. >> >> This doesn't impact runtime per se because KVM doesn't care about these >> flags, but code such as riscv_isa_string_ext() take those flags into >> account. The result is that, for a KVM guest, setting non-KVM properties >> will make them appear in the riscv,isa DT. >> >> We want to keep the same API for both TCG and KVM and at the same time, >> when running KVM, forbid non-KVM extensions to be enabled internally. We >> accomplish both by changing riscv_cpu_add_user_properties() to add a >> mock/no-op boolean property for every non-KVM extension in >> riscv_cpu_extensions[]. Then, when running KVM, users are still free to >> set extensions at will, we'll treat non-KVM extensions as a no-op, and >> riscv_isa_string_ext() will not report bogus extensions in the DT. >> >> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> >> --- >> target/riscv/cpu.c | 36 +++++++++++++++++++++++++++++++++--- >> 1 file changed, 33 insertions(+), 3 deletions(-) >> >> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c >> index b65db165cc..f5209f0789 100644 >> --- a/target/riscv/cpu.c >> +++ b/target/riscv/cpu.c >> @@ -1720,6 +1720,18 @@ static Property riscv_cpu_extensions[] = { >> DEFINE_PROP_END_OF_LIST(), >> }; >> >> + >> +static void cpu_set_cfg_noop(Object *obj, Visitor *v, >> + const char *name, >> + void *opaque, Error **errp) >> +{ >> + bool value; >> + >> + if (!visit_type_bool(v, name, &value, errp)) { >> + return; >> + } >> +} >> + >> /* >> * Add CPU properties with user-facing flags. >> * >> @@ -1738,9 +1750,27 @@ static void riscv_cpu_add_user_properties(Object *obj) >> riscv_cpu_add_misa_properties(obj); >> >> for (prop = riscv_cpu_extensions; prop && prop->name; prop++) { >> - /* Check if KVM didn't create the property already */ >> - if (object_property_find(obj, prop->name)) { >> - continue; >> + if (riscv_running_kvm()) { >> + /* Check if KVM didn't create the property already */ >> + if (object_property_find(obj, prop->name)) { >> + continue; >> + } >> + >> + /* >> + * Set every multi-letter extension that KVM doesn't >> + * know as a no-op. This will allow users to set values >> + * to them while keeping their internal state to 'false'. >> + * >> + * We're giving a pass for non-bool properties since they're >> + * not related to the availability of extensions and can be >> + * safely ignored as is. >> + */ >> + if (prop->info == &qdev_prop_bool) { >> + object_property_add(obj, prop->name, "bool", >> + NULL, cpu_set_cfg_noop, >> + NULL, NULL); >> + continue; >> + } >> } >> >> qdev_property_add_static(dev, prop); >> -- >> 2.41.0 >> > > I think we should actually fail with an error when the user tries to > enable an extension KVM doesn't support. Otherwise a user may be > confused as to why their Zawrs=on didn't provide them a machine with > Zawrs. And, when KVM learns how to provide that support to guests > (Zawrs is actually on my TODO...), then migrating the same VM to > later KVM/QEMU will actually enable the feature, possibly confusing > the guest. > > So, we should probably just not add any extension properties to KVM > guests which can't be enabled. Then, as we add support to KVM, we'll > add the properties too. By 'extension properties' do you mean just the flags that enable/disable them, like '-cpu, rawrs=<bool>', or also the other properties related to extensions that KVM might not support, like 'vlen' and 'elen' from RVV? I'd say that it's ok to leave things such as 'vlen' because the user won't be able to enable RVV in KVM anyways. And what error do we want to throw? With this patch it's easy to just add an Extension zawrs is not available using KVM" error message. Otherwise we can not add the property at all and then QEMU will fail with a "property cpu.X not found" type of error. Both will error out, question is whether we want to be more informative about it. Thanks, Daniel > > Thanks, > drew
On Fri, Jun 23, 2023 at 11:28:03AM -0300, Daniel Henrique Barboza wrote: ... > > I think we should actually fail with an error when the user tries to > > enable an extension KVM doesn't support. Otherwise a user may be > > confused as to why their Zawrs=on didn't provide them a machine with > > Zawrs. And, when KVM learns how to provide that support to guests > > (Zawrs is actually on my TODO...), then migrating the same VM to > > later KVM/QEMU will actually enable the feature, possibly confusing > > the guest. > > > > So, we should probably just not add any extension properties to KVM > > guests which can't be enabled. Then, as we add support to KVM, we'll > > add the properties too. > > By 'extension properties' do you mean just the flags that enable/disable them, > like '-cpu, rawrs=<bool>', or also the other properties related to extensions > that KVM might not support, like 'vlen' and 'elen' from RVV? I'd say that it's > ok to leave things such as 'vlen' because the user won't be able to enable RVV > in KVM anyways. Properties like 'vlen', which have a dependency on an extension, should probably have their own error checking at cpu finalize features time. I.e. if 'vlen' is present, but not V, then QEMU should complain. I see we don't currently do that, though. > > And what error do we want to throw? With this patch it's easy to just add an > Extension zawrs is not available using KVM" error message. Otherwise we can > not add the property at all and then QEMU will fail with a "property cpu.X not > found" type of error. Both will error out, question is whether we want to be > more informative about it. It's probably best to do the "not available with KVM" error by changing this patch from adding a no-op setter to an error-out setter. That way, our story that a riscv machine is the same for both KVM and TCG still remains, i.e. all properties are still present, but we add the honesty to the story that not everything works with KVM. Thanks, drew
On 6/24/23 04:41, Andrew Jones wrote: > On Fri, Jun 23, 2023 at 11:28:03AM -0300, Daniel Henrique Barboza wrote: > ... >>> I think we should actually fail with an error when the user tries to >>> enable an extension KVM doesn't support. Otherwise a user may be >>> confused as to why their Zawrs=on didn't provide them a machine with >>> Zawrs. And, when KVM learns how to provide that support to guests >>> (Zawrs is actually on my TODO...), then migrating the same VM to >>> later KVM/QEMU will actually enable the feature, possibly confusing >>> the guest. >>> >>> So, we should probably just not add any extension properties to KVM >>> guests which can't be enabled. Then, as we add support to KVM, we'll >>> add the properties too. >> >> By 'extension properties' do you mean just the flags that enable/disable them, >> like '-cpu, rawrs=<bool>', or also the other properties related to extensions >> that KVM might not support, like 'vlen' and 'elen' from RVV? I'd say that it's >> ok to leave things such as 'vlen' because the user won't be able to enable RVV >> in KVM anyways. > > Properties like 'vlen', which have a dependency on an extension, should > probably have their own error checking at cpu finalize features time. > I.e. if 'vlen' is present, but not V, then QEMU should complain. I see > we don't currently do that, though. We do some checks during realize() in riscv_validate_set_extensions(), via riscv_cpu_validate_v(). However, 'is the user set vlen but not V we should error out' is not possible because we don't have a 'user_set' flag for this option, and we can't say if vlen = 128 is the default, untouched value, or if the user set vlen = 128 in the command line. I think we're getting closer to a point where we can "export" the KVM config options model to TCG. Let's see if we can pull something off for the next release. > >> >> And what error do we want to throw? With this patch it's easy to just add an >> Extension zawrs is not available using KVM" error message. Otherwise we can >> not add the property at all and then QEMU will fail with a "property cpu.X not >> found" type of error. Both will error out, question is whether we want to be >> more informative about it. > > It's probably best to do the "not available with KVM" error by changing > this patch from adding a no-op setter to an error-out setter. That way, > our story that a riscv machine is the same for both KVM and TCG still > remains, i.e. all properties are still present, but we add the honesty > to the story that not everything works with KVM. Got it. I'll add an error message in v4. Thanks, Daniel > > Thanks, > drew
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c index b65db165cc..f5209f0789 100644 --- a/target/riscv/cpu.c +++ b/target/riscv/cpu.c @@ -1720,6 +1720,18 @@ static Property riscv_cpu_extensions[] = { DEFINE_PROP_END_OF_LIST(), }; + +static void cpu_set_cfg_noop(Object *obj, Visitor *v, + const char *name, + void *opaque, Error **errp) +{ + bool value; + + if (!visit_type_bool(v, name, &value, errp)) { + return; + } +} + /* * Add CPU properties with user-facing flags. * @@ -1738,9 +1750,27 @@ static void riscv_cpu_add_user_properties(Object *obj) riscv_cpu_add_misa_properties(obj); for (prop = riscv_cpu_extensions; prop && prop->name; prop++) { - /* Check if KVM didn't create the property already */ - if (object_property_find(obj, prop->name)) { - continue; + if (riscv_running_kvm()) { + /* Check if KVM didn't create the property already */ + if (object_property_find(obj, prop->name)) { + continue; + } + + /* + * Set every multi-letter extension that KVM doesn't + * know as a no-op. This will allow users to set values + * to them while keeping their internal state to 'false'. + * + * We're giving a pass for non-bool properties since they're + * not related to the availability of extensions and can be + * safely ignored as is. + */ + if (prop->info == &qdev_prop_bool) { + object_property_add(obj, prop->name, "bool", + NULL, cpu_set_cfg_noop, + NULL, NULL); + continue; + } } qdev_property_add_static(dev, prop);
KVM-specific properties are being created inside target/riscv/kvm.c. But at this moment we're gathering all the remaining properties from TCG and adding them as is when running KVM. This creates a situation where non-KVM properties are setting flags to 'true' due to its default settings (e.g. Zawrs). Users can also freely enable them via command line. This doesn't impact runtime per se because KVM doesn't care about these flags, but code such as riscv_isa_string_ext() take those flags into account. The result is that, for a KVM guest, setting non-KVM properties will make them appear in the riscv,isa DT. We want to keep the same API for both TCG and KVM and at the same time, when running KVM, forbid non-KVM extensions to be enabled internally. We accomplish both by changing riscv_cpu_add_user_properties() to add a mock/no-op boolean property for every non-KVM extension in riscv_cpu_extensions[]. Then, when running KVM, users are still free to set extensions at will, we'll treat non-KVM extensions as a no-op, and riscv_isa_string_ext() will not report bogus extensions in the DT. Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> --- target/riscv/cpu.c | 36 +++++++++++++++++++++++++++++++++--- 1 file changed, 33 insertions(+), 3 deletions(-)