On Tue, Jun 13, 2023 at 05:58:52PM -0300, Daniel Henrique Barboza wrote:
> Our design philosophy with KVM properties can be resumed in two main
> decisions based on KVM interface availability and what the user wants to
> do:
>
> - if the user disables an extension that the host KVM module doesn't
> know about (i.e. it doesn't implement the kvm_get_one_reg() interface),
> keep booting the CPU. This will avoid users having to deal with issues
> with older KVM versions while disabling features they don't care;
>
> - for any other case we're going to error out immediately. If the user
> wants to enable a feature that KVM doesn't know about this a problem that
> is worth aborting - the user must know that the feature wasn't enabled
> in the hart. Likewise, if KVM knows about the extension, the user wants
> to enable/disable it, and we fail to do it so, that's also a problem we
> can't shrug it off.
>
> For MISA bits we're going to be a little more conservative: we won't
> even try enabling bits that aren't already available in the host. The
I don't think any extensions should try to enable anything KVM doesn't
advertise. Even if it somehow works, the lack of advertisement is a
KVM bug and QEMU not trying to enable it without the advertisement would
help flush that out. IOW, MISA bits shouldn't be "more conservative",
all extensions should be fully conservative.
> ioctl() is so likely to fail that's not worth trying. This check is
> already done in the previous patch, in kvm_cpu_set_misa_ext_cfg(), thus
> we don't need to worry about it now.
>
> In kvm_riscv_update_cpu_misa_ext() we'll go through every potential user
> option and do as follows:
>
> - if the user didn't set the property or set to the same value of the
> host, do nothing;
>
> - Disable the given extension in KVM. Error out if anything goes wrong.
>
> Signed-off-by: Daniel Henrique Barboza <dbarb...@ventanamicro.com>
> ---
> target/riscv/kvm.c | 40 ++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 40 insertions(+)
>
> diff --git a/target/riscv/kvm.c b/target/riscv/kvm.c
> index 53042a0e86..ea38f91b92 100644
> --- a/target/riscv/kvm.c
> +++ b/target/riscv/kvm.c
> @@ -164,6 +164,41 @@ static void kvm_cpu_set_misa_ext_cfg(Object *obj,
> Visitor *v,
> "enabled in the host", misa_ext_cfg->name);
> }
>
> +static void kvm_riscv_update_cpu_misa_ext(RISCVCPU *cpu, CPUState *cs)
> +{
> + CPURISCVState *env = &cpu->env;
> + uint64_t id, reg;
> + int i, ret;
> +
> + for (i = 0; i < ARRAY_SIZE(kvm_misa_ext_cfgs); i++) {
> + KVMCPUConfig *misa_cfg = &kvm_misa_ext_cfgs[i];
> + target_ulong misa_bit = misa_cfg->offset;
> +
> + if (!misa_cfg->user_set) {
> + continue;
> + }
> +
> + /* If we're here we're going to disable the MISA bit */
> + reg = 0;
> + id = kvm_riscv_reg_id(env, KVM_REG_RISCV_ISA_EXT,
> + misa_cfg->kvm_reg_id);
> + ret = kvm_set_one_reg(cs, id, ®);
> + if (ret != 0) {
> + /*
> + * We're not checking for -EINVAL because if the bit is
> + * about to be disabled means that it was already enabled
^, it
> + * by KVM, something that we determined by fetching the
> + * 'isa' register during init() time. Any error at this
> + * point is worth aborting.
> + */
> + error_report("Unable to set KVM reg %s, error %d",
> + misa_cfg->name, ret);
> + exit(EXIT_FAILURE);
> + }
> + env->misa_ext &= ~misa_bit;
> + }
> +}
> +
> static void kvm_riscv_add_cpu_user_properties(Object *cpu_obj)
> {
> int i;
> @@ -630,8 +665,13 @@ int kvm_arch_init_vcpu(CPUState *cs)
>
> if (!object_dynamic_cast(OBJECT(cpu), TYPE_RISCV_CPU_HOST)) {
> ret = kvm_vcpu_set_machine_ids(cpu, cs);
> + if (ret != 0) {
> + return ret;
> + }
> }
>
> + kvm_riscv_update_cpu_misa_ext(cpu, cs);
> +
> return ret;
> }
>
> --
> 2.40.1
>
Besides the commit message clarification and the code comment typo,
Reviewed-by: Andrew Jones <ajo...@ventanamicro.com>
Thanks,
drew
