* Re: [PATCH v2 1/1] target/riscv/kvm: tolerate KVM disable ext errors
2024-04-22 17:14 ` [PATCH v2 1/1] " Daniel Henrique Barboza
@ 2024-04-25 15:58 ` Andrew Jones
2024-04-29 2:57 ` Alistair Francis
1 sibling, 0 replies; 4+ messages in thread
From: Andrew Jones @ 2024-04-25 15:58 UTC (permalink / raw)
To: Daniel Henrique Barboza
Cc: qemu-devel, qemu-riscv, alistair.francis, bmeng, liwei1518,
zhiwei_liu, palmer
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, ®);
> 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>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH v2 1/1] target/riscv/kvm: tolerate KVM disable ext errors
2024-04-22 17:14 ` [PATCH v2 1/1] " Daniel Henrique Barboza
2024-04-25 15:58 ` Andrew Jones
@ 2024-04-29 2:57 ` Alistair Francis
1 sibling, 0 replies; 4+ messages in thread
From: Alistair Francis @ 2024-04-29 2:57 UTC (permalink / raw)
To: Daniel Henrique Barboza
Cc: qemu-devel, qemu-riscv, alistair.francis, bmeng, liwei1518,
zhiwei_liu, palmer, ajones
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, ®);
> 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
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread