[PATCH v2 0/1] target/riscv/kvm: tolerate KVM disable ext errors

[PATCH v2 0/1] target/riscv/kvm: tolerate KVM disable ext errors

[Daniel Henrique Barboza]

Hi,

In this new version we changed the commit message a bit and we're now
only handling the case for EINVAL. Both were suggested by Drew in v1.

Changes from v1:
- added an extra paragraph explaining why we're throwing an warning
- changed the warning string
- warning is now being thrown only if EINVAL is returned. We'll keep
  erroring out otherwise.
- v1 link: https://lore.kernel.org/qemu-riscv/20240422131253.313869-1-dbarboza@ventanamicro.com/

Daniel Henrique Barboza (1):
  target/riscv/kvm: tolerate KVM disable ext errors

 target/riscv/kvm/kvm-cpu.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

-- 
2.44.0

[PATCH v2 1/1] target/riscv/kvm: tolerate KVM disable ext errors

[Daniel Henrique Barboza]

Running a KVM guest using a 6.9-rc3 kernel, in a 6.8 host that has zkr
enabled, will fail with a kernel oops SIGILL right at the start. The
reason is that we can't expose zkr without implementing the SEED CSR.
Disabling zkr in the guest would be a workaround, but if the KVM doesn't
allow it we'll error out and never boot.

In hindsight this is too strict. If we keep proceeding, despite not
disabling the extension in the KVM vcpu, we'll not add the extension in
the riscv,isa. The guest kernel will be unaware of the extension, i.e.
it doesn't matter if the KVM vcpu has it enabled underneath or not. So
it's ok to keep booting in this case.

Change our current logic to not error out if we fail to disable an
extension in kvm_set_one_reg(), but show a warning and keep booting. It
is important to throw a warning because we must make the user aware that
the extension is still available in the vcpu, meaning that an
ill-behaved guest can ignore the riscv,isa settings and  use the
extension.

The case we're handling happens with an EINVAL error code. If we fail to
disable the extension in KVM for any other reason, error out.

We'll also keep erroring out when we fail to enable an extension in KVM,
since adding the extension in riscv,isa at this point will cause a guest
malfunction because the extension isn't enabled in the vcpu.

Suggested-by: Andrew Jones <ajones@ventanamicro.com>
Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 target/riscv/kvm/kvm-cpu.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
index 6a6c6cae80..03e3fee607 100644
--- a/target/riscv/kvm/kvm-cpu.c
+++ b/target/riscv/kvm/kvm-cpu.c
@@ -427,10 +427,14 @@ static void kvm_riscv_update_cpu_cfg_isa_ext(RISCVCPU *cpu, CPUState *cs)
         reg = kvm_cpu_cfg_get(cpu, multi_ext_cfg);
         ret = kvm_set_one_reg(cs, id, &reg);
         if (ret != 0) {
-            error_report("Unable to %s extension %s in KVM, error %d",
-                         reg ? "enable" : "disable",
-                         multi_ext_cfg->name, ret);
-            exit(EXIT_FAILURE);
+            if (!reg && ret == -EINVAL) {
+                warn_report("KVM cannot disable extension %s",
+                            multi_ext_cfg->name);
+            } else {
+                error_report("Unable to enable extension %s in KVM, error %d",
+                             multi_ext_cfg->name, ret);
+                exit(EXIT_FAILURE);
+            }
         }
     }
 }
-- 
2.44.0

Re: [PATCH v2 1/1] target/riscv/kvm: tolerate KVM disable ext errors

[Andrew Jones]

On Mon, Apr 22, 2024 at 02:14:25PM GMT, Daniel Henrique Barboza wrote:
> Running a KVM guest using a 6.9-rc3 kernel, in a 6.8 host that has zkr
> enabled, will fail with a kernel oops SIGILL right at the start. The
> reason is that we can't expose zkr without implementing the SEED CSR.
> Disabling zkr in the guest would be a workaround, but if the KVM doesn't
> allow it we'll error out and never boot.
> 
> In hindsight this is too strict. If we keep proceeding, despite not
> disabling the extension in the KVM vcpu, we'll not add the extension in
> the riscv,isa. The guest kernel will be unaware of the extension, i.e.
> it doesn't matter if the KVM vcpu has it enabled underneath or not. So
> it's ok to keep booting in this case.
> 
> Change our current logic to not error out if we fail to disable an
> extension in kvm_set_one_reg(), but show a warning and keep booting. It
> is important to throw a warning because we must make the user aware that
> the extension is still available in the vcpu, meaning that an
> ill-behaved guest can ignore the riscv,isa settings and  use the
> extension.
> 
> The case we're handling happens with an EINVAL error code. If we fail to
> disable the extension in KVM for any other reason, error out.
> 
> We'll also keep erroring out when we fail to enable an extension in KVM,
> since adding the extension in riscv,isa at this point will cause a guest
> malfunction because the extension isn't enabled in the vcpu.
> 
> Suggested-by: Andrew Jones <ajones@ventanamicro.com>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>  target/riscv/kvm/kvm-cpu.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
> index 6a6c6cae80..03e3fee607 100644
> --- a/target/riscv/kvm/kvm-cpu.c
> +++ b/target/riscv/kvm/kvm-cpu.c
> @@ -427,10 +427,14 @@ static void kvm_riscv_update_cpu_cfg_isa_ext(RISCVCPU *cpu, CPUState *cs)
>          reg = kvm_cpu_cfg_get(cpu, multi_ext_cfg);
>          ret = kvm_set_one_reg(cs, id, &reg);
>          if (ret != 0) {
> -            error_report("Unable to %s extension %s in KVM, error %d",
> -                         reg ? "enable" : "disable",
> -                         multi_ext_cfg->name, ret);
> -            exit(EXIT_FAILURE);
> +            if (!reg && ret == -EINVAL) {
> +                warn_report("KVM cannot disable extension %s",
> +                            multi_ext_cfg->name);
> +            } else {
> +                error_report("Unable to enable extension %s in KVM, error %d",
> +                             multi_ext_cfg->name, ret);
> +                exit(EXIT_FAILURE);
> +            }
>          }
>      }
>  }
> -- 
> 2.44.0
>

Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

Re: [PATCH v2 1/1] target/riscv/kvm: tolerate KVM disable ext errors

[Alistair Francis]

On Tue, Apr 23, 2024 at 3:15 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> Running a KVM guest using a 6.9-rc3 kernel, in a 6.8 host that has zkr
> enabled, will fail with a kernel oops SIGILL right at the start. The
> reason is that we can't expose zkr without implementing the SEED CSR.
> Disabling zkr in the guest would be a workaround, but if the KVM doesn't
> allow it we'll error out and never boot.
>
> In hindsight this is too strict. If we keep proceeding, despite not
> disabling the extension in the KVM vcpu, we'll not add the extension in
> the riscv,isa. The guest kernel will be unaware of the extension, i.e.
> it doesn't matter if the KVM vcpu has it enabled underneath or not. So
> it's ok to keep booting in this case.
>
> Change our current logic to not error out if we fail to disable an
> extension in kvm_set_one_reg(), but show a warning and keep booting. It
> is important to throw a warning because we must make the user aware that
> the extension is still available in the vcpu, meaning that an
> ill-behaved guest can ignore the riscv,isa settings and  use the
> extension.
>
> The case we're handling happens with an EINVAL error code. If we fail to
> disable the extension in KVM for any other reason, error out.
>
> We'll also keep erroring out when we fail to enable an extension in KVM,
> since adding the extension in riscv,isa at this point will cause a guest
> malfunction because the extension isn't enabled in the vcpu.
>
> Suggested-by: Andrew Jones <ajones@ventanamicro.com>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

Thanks!

Applied to riscv-to-apply.next

Alistair

> ---
>  target/riscv/kvm/kvm-cpu.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
> index 6a6c6cae80..03e3fee607 100644
> --- a/target/riscv/kvm/kvm-cpu.c
> +++ b/target/riscv/kvm/kvm-cpu.c
> @@ -427,10 +427,14 @@ static void kvm_riscv_update_cpu_cfg_isa_ext(RISCVCPU *cpu, CPUState *cs)
>          reg = kvm_cpu_cfg_get(cpu, multi_ext_cfg);
>          ret = kvm_set_one_reg(cs, id, &reg);
>          if (ret != 0) {
> -            error_report("Unable to %s extension %s in KVM, error %d",
> -                         reg ? "enable" : "disable",
> -                         multi_ext_cfg->name, ret);
> -            exit(EXIT_FAILURE);
> +            if (!reg && ret == -EINVAL) {
> +                warn_report("KVM cannot disable extension %s",
> +                            multi_ext_cfg->name);
> +            } else {
> +                error_report("Unable to enable extension %s in KVM, error %d",
> +                             multi_ext_cfg->name, ret);
> +                exit(EXIT_FAILURE);
> +            }
>          }
>      }
>  }
> --
> 2.44.0
>
>