Re: [PATCH v3 16/19] target/riscv/cpu.c: create KVM mock properties

[Daniel Henrique Barboza]

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