Re: [PATCH v2 13/18] target/riscv/kvm.c: update KVM MISA bits

[Andrew Jones]

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 <dbarboza@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, &reg);
> +        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 <ajones@ventanamicro.com>

Thanks,
drew