1. Patchew
  2. QEMU
  3. View series

[PATCH] accel/kvm/kvm-all: Handle register access errors

Akihiko Odaki posted 1 patch 1 year, 5 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20221201102728.69751-1-akihiko.odaki@daynix.com
Maintainers: Paolo Bonzini <pbonzini@redhat.com>
accel/kvm/kvm-all.c | 32 ++++++++++++++++++++++++++++----
1 file changed, 28 insertions(+), 4 deletions(-)
[PATCH] accel/kvm/kvm-all: Handle register access errors
Posted by Akihiko Odaki 1 year, 5 months ago 
A register access error typically means something seriously wrong
happened so that anything bad can happen after that and recovery is
impossible.
Even failing one register access is catastorophic as
architecture-specific code are not written so that it torelates such
failures.

Make sure the VM stop and nothing worse happens if such an error occurs.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 accel/kvm/kvm-all.c | 32 ++++++++++++++++++++++++++++----
 1 file changed, 28 insertions(+), 4 deletions(-)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index f99b0becd8..9e848f750e 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -2709,7 +2709,13 @@ bool kvm_cpu_check_are_resettable(void)
 static void do_kvm_cpu_synchronize_state(CPUState *cpu, run_on_cpu_data arg)
 {
     if (!cpu->vcpu_dirty) {
-        kvm_arch_get_registers(cpu);
+        int ret = kvm_arch_get_registers(cpu);
+        if (ret) {
+            error_report("Failed to get registers: %s", strerror(-ret));
+            cpu_dump_state(cpu, stderr, CPU_DUMP_CODE);
+            vm_stop(RUN_STATE_INTERNAL_ERROR);
+        }
+
         cpu->vcpu_dirty = true;
     }
 }
@@ -2723,7 +2729,13 @@ void kvm_cpu_synchronize_state(CPUState *cpu)
 
 static void do_kvm_cpu_synchronize_post_reset(CPUState *cpu, run_on_cpu_data arg)
 {
-    kvm_arch_put_registers(cpu, KVM_PUT_RESET_STATE);
+    int ret = kvm_arch_put_registers(cpu, KVM_PUT_RESET_STATE);
+    if (ret) {
+        error_report("Failed to put registers after reset: %s", strerror(-ret));
+        cpu_dump_state(cpu, stderr, CPU_DUMP_CODE);
+        vm_stop(RUN_STATE_INTERNAL_ERROR);
+    }
+
     cpu->vcpu_dirty = false;
 }
 
@@ -2734,7 +2746,12 @@ void kvm_cpu_synchronize_post_reset(CPUState *cpu)
 
 static void do_kvm_cpu_synchronize_post_init(CPUState *cpu, run_on_cpu_data arg)
 {
-    kvm_arch_put_registers(cpu, KVM_PUT_FULL_STATE);
+    int ret = kvm_arch_put_registers(cpu, KVM_PUT_FULL_STATE);
+    if (ret) {
+        error_report("Failed to put registers after init: %s", strerror(-ret));
+        exit(1);
+    }
+
     cpu->vcpu_dirty = false;
 }
 
@@ -2827,7 +2844,14 @@ int kvm_cpu_exec(CPUState *cpu)
         MemTxAttrs attrs;
 
         if (cpu->vcpu_dirty) {
-            kvm_arch_put_registers(cpu, KVM_PUT_RUNTIME_STATE);
+            ret = kvm_arch_put_registers(cpu, KVM_PUT_RUNTIME_STATE);
+            if (ret) {
+                error_report("Failed to put registers after init: %s",
+                             strerror(-ret));
+                ret = -1;
+                break;
+            }
+
             cpu->vcpu_dirty = false;
         }
 
-- 
2.38.1
Re: [PATCH] accel/kvm/kvm-all: Handle register access errors
Posted by Paolo Bonzini 7 months, 1 week ago 
Queued, thanks.

Paolo
Re: [PATCH] accel/kvm/kvm-all: Handle register access errors
Posted by Peter Maydell 1 year, 5 months ago 
On Thu, 1 Dec 2022 at 10:27, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> A register access error typically means something seriously wrong
> happened so that anything bad can happen after that and recovery is
> impossible.
> Even failing one register access is catastorophic as
> architecture-specific code are not written so that it torelates such
> failures.
>
> Make sure the VM stop and nothing worse happens if such an error occurs.
>
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>

In a similar vein there was also
https://lore.kernel.org/all/20220617144857.34189-1-peterx@redhat.com/
back in June, which on the one hand was less comprehensive but on
the other does the plumbing to pass the error upwards rather than
reporting it immediately at point of failure.

I'm in principle in favour but suspect we'll run into some corner
cases where we were happily ignoring not-very-important failures
(eg if you're running Linux as the host OS on a Mac M1 and your
host kernel doesn't have this fix:
https://lore.kernel.org/all/YnHz6Cw5ONR2e+KA@google.com/T/
then QEMU will go from "works by sheer luck" to "consistently
hits this error check"). So we should aim to land this extra
error checking early in the release cycle so we have plenty of
time to deal with any bug reports we get about it.

thanks
-- PMM
Re: [PATCH] accel/kvm/kvm-all: Handle register access errors
Posted by Akihiko Odaki 1 year, 5 months ago 
On 2022/12/01 19:40, Peter Maydell wrote:
> On Thu, 1 Dec 2022 at 10:27, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>
>> A register access error typically means something seriously wrong
>> happened so that anything bad can happen after that and recovery is
>> impossible.
>> Even failing one register access is catastorophic as
>> architecture-specific code are not written so that it torelates such
>> failures.
>>
>> Make sure the VM stop and nothing worse happens if such an error occurs.
>>
>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> 
> In a similar vein there was also
> https://lore.kernel.org/all/20220617144857.34189-1-peterx@redhat.com/
> back in June, which on the one hand was less comprehensive but on
> the other does the plumbing to pass the error upwards rather than
> reporting it immediately at point of failure.
> 
> I'm in principle in favour but suspect we'll run into some corner
> cases where we were happily ignoring not-very-important failures
> (eg if you're running Linux as the host OS on a Mac M1 and your
> host kernel doesn't have this fix:
> https://lore.kernel.org/all/YnHz6Cw5ONR2e+KA@google.com/T/
> then QEMU will go from "works by sheer luck" to "consistently
> hits this error check"). So we should aim to land this extra
> error checking early in the release cycle so we have plenty of
> time to deal with any bug reports we get about it.
> 
> thanks
> -- PMM

Actually I found this problem when I tried to run QEMU with KVM on M2 
MacBook Air and encountered a failure described and fixed at:
https://lore.kernel.org/all/20221201104914.28944-2-akihiko.odaki@daynix.com/

Although the affected register was not really important, QEMU couldn't 
run the guest well enough because kvm_arch_put_registers for ARM64 is 
written in a way that it fails early. I guess the situation is not so 
different for other architectures as well.

I still agree that this should be postponed until a new release cycle 
starts as register saving/restoring is too important to fail.

Regards,
Akihiko Odaki
Re: [PATCH] accel/kvm/kvm-all: Handle register access errors
Posted by Akihiko Odaki 11 months ago 
On 2022/12/01 20:00, Akihiko Odaki wrote:
> On 2022/12/01 19:40, Peter Maydell wrote:
>> On Thu, 1 Dec 2022 at 10:27, Akihiko Odaki <akihiko.odaki@daynix.com> 
>> wrote:
>>>
>>> A register access error typically means something seriously wrong
>>> happened so that anything bad can happen after that and recovery is
>>> impossible.
>>> Even failing one register access is catastorophic as
>>> architecture-specific code are not written so that it torelates such
>>> failures.
>>>
>>> Make sure the VM stop and nothing worse happens if such an error occurs.
>>>
>>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>>
>> In a similar vein there was also
>> https://lore.kernel.org/all/20220617144857.34189-1-peterx@redhat.com/
>> back in June, which on the one hand was less comprehensive but on
>> the other does the plumbing to pass the error upwards rather than
>> reporting it immediately at point of failure.
>>
>> I'm in principle in favour but suspect we'll run into some corner
>> cases where we were happily ignoring not-very-important failures
>> (eg if you're running Linux as the host OS on a Mac M1 and your
>> host kernel doesn't have this fix:
>> https://lore.kernel.org/all/YnHz6Cw5ONR2e+KA@google.com/T/
>> then QEMU will go from "works by sheer luck" to "consistently
>> hits this error check"). So we should aim to land this extra
>> error checking early in the release cycle so we have plenty of
>> time to deal with any bug reports we get about it.
>>
>> thanks
>> -- PMM
> 
> Actually I found this problem when I tried to run QEMU with KVM on M2 
> MacBook Air and encountered a failure described and fixed at:
> https://lore.kernel.org/all/20221201104914.28944-2-akihiko.odaki@daynix.com/
> 
> Although the affected register was not really important, QEMU couldn't 
> run the guest well enough because kvm_arch_put_registers for ARM64 is 
> written in a way that it fails early. I guess the situation is not so 
> different for other architectures as well.
> 
> I still agree that this should be postponed until a new release cycle 
> starts as register saving/restoring is too important to fail.
> 
> Regards,
> Akihiko Odaki

Hi,

QEMU 8.0 is already released so I think it's time to revisit this.

Regards,
Akihiko Odaki
Re: [PATCH] accel/kvm/kvm-all: Handle register access errors
Posted by Peter Maydell 10 months, 3 weeks ago 
On Sat, 10 Jun 2023 at 04:51, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> On 2022/12/01 20:00, Akihiko Odaki wrote:
> > On 2022/12/01 19:40, Peter Maydell wrote:
> >> On Thu, 1 Dec 2022 at 10:27, Akihiko Odaki <akihiko.odaki@daynix.com>
> >> wrote:
> >>>
> >>> A register access error typically means something seriously wrong
> >>> happened so that anything bad can happen after that and recovery is
> >>> impossible.
> >>> Even failing one register access is catastorophic as
> >>> architecture-specific code are not written so that it torelates such
> >>> failures.
> >>>
> >>> Make sure the VM stop and nothing worse happens if such an error occurs.
> >>>
> >>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> >>
> >> In a similar vein there was also
> >> https://lore.kernel.org/all/20220617144857.34189-1-peterx@redhat.com/
> >> back in June, which on the one hand was less comprehensive but on
> >> the other does the plumbing to pass the error upwards rather than
> >> reporting it immediately at point of failure.
> >>
> >> I'm in principle in favour but suspect we'll run into some corner
> >> cases where we were happily ignoring not-very-important failures
> >> (eg if you're running Linux as the host OS on a Mac M1 and your
> >> host kernel doesn't have this fix:
> >> https://lore.kernel.org/all/YnHz6Cw5ONR2e+KA@google.com/T/
> >> then QEMU will go from "works by sheer luck" to "consistently
> >> hits this error check"). So we should aim to land this extra
> >> error checking early in the release cycle so we have plenty of
> >> time to deal with any bug reports we get about it.

> > Actually I found this problem when I tried to run QEMU with KVM on M2
> > MacBook Air and encountered a failure described and fixed at:
> > https://lore.kernel.org/all/20221201104914.28944-2-akihiko.odaki@daynix.com/
> >
> > Although the affected register was not really important, QEMU couldn't
> > run the guest well enough because kvm_arch_put_registers for ARM64 is
> > written in a way that it fails early. I guess the situation is not so
> > different for other architectures as well.
> >
> > I still agree that this should be postponed until a new release cycle
> > starts as register saving/restoring is too important to fail.

> Hi,
>
> QEMU 8.0 is already released so I think it's time to revisit this.

Two months ago would have been a better time :-) We're heading up
towards softfreeze for 8.1 in about three weeks from now.

thanks
-- PMM
Re: [PATCH] accel/kvm/kvm-all: Handle register access errors
Posted by Akihiko Odaki 7 months, 2 weeks ago 
On 2023/06/19 21:19, Peter Maydell wrote:
> On Sat, 10 Jun 2023 at 04:51, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>
>> On 2022/12/01 20:00, Akihiko Odaki wrote:
>>> On 2022/12/01 19:40, Peter Maydell wrote:
>>>> On Thu, 1 Dec 2022 at 10:27, Akihiko Odaki <akihiko.odaki@daynix.com>
>>>> wrote:
>>>>>
>>>>> A register access error typically means something seriously wrong
>>>>> happened so that anything bad can happen after that and recovery is
>>>>> impossible.
>>>>> Even failing one register access is catastorophic as
>>>>> architecture-specific code are not written so that it torelates such
>>>>> failures.
>>>>>
>>>>> Make sure the VM stop and nothing worse happens if such an error occurs.
>>>>>
>>>>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>>>>
>>>> In a similar vein there was also
>>>> https://lore.kernel.org/all/20220617144857.34189-1-peterx@redhat.com/
>>>> back in June, which on the one hand was less comprehensive but on
>>>> the other does the plumbing to pass the error upwards rather than
>>>> reporting it immediately at point of failure.
>>>>
>>>> I'm in principle in favour but suspect we'll run into some corner
>>>> cases where we were happily ignoring not-very-important failures
>>>> (eg if you're running Linux as the host OS on a Mac M1 and your
>>>> host kernel doesn't have this fix:
>>>> https://lore.kernel.org/all/YnHz6Cw5ONR2e+KA@google.com/T/
>>>> then QEMU will go from "works by sheer luck" to "consistently
>>>> hits this error check"). So we should aim to land this extra
>>>> error checking early in the release cycle so we have plenty of
>>>> time to deal with any bug reports we get about it.
> 
>>> Actually I found this problem when I tried to run QEMU with KVM on M2
>>> MacBook Air and encountered a failure described and fixed at:
>>> https://lore.kernel.org/all/20221201104914.28944-2-akihiko.odaki@daynix.com/
>>>
>>> Although the affected register was not really important, QEMU couldn't
>>> run the guest well enough because kvm_arch_put_registers for ARM64 is
>>> written in a way that it fails early. I guess the situation is not so
>>> different for other architectures as well.
>>>
>>> I still agree that this should be postponed until a new release cycle
>>> starts as register saving/restoring is too important to fail.
> 
>> Hi,
>>
>> QEMU 8.0 is already released so I think it's time to revisit this.
> 
> Two months ago would have been a better time :-) We're heading up
> towards softfreeze for 8.1 in about three weeks from now.
> 
> thanks
> -- PMM

Hi Peter,

Please apply this.

Regards,
Akihiko Odaki
Re: [PATCH] accel/kvm/kvm-all: Handle register access errors
Posted by Peter Maydell 7 months, 2 weeks ago 
On Thu, 21 Sept 2023 at 08:25, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
> On 2023/06/19 21:19, Peter Maydell wrote:
> > On Sat, 10 Jun 2023 at 04:51, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
> >> On 2022/12/01 20:00, Akihiko Odaki wrote:
> >>> On 2022/12/01 19:40, Peter Maydell wrote:
> >>>> On Thu, 1 Dec 2022 at 10:27, Akihiko Odaki <akihiko.odaki@daynix.com>
> >>>> wrote:
> >>>>> A register access error typically means something seriously wrong
> >>>>> happened so that anything bad can happen after that and recovery is
> >>>>> impossible.
> >>>>> Even failing one register access is catastorophic as
> >>>>> architecture-specific code are not written so that it torelates such
> >>>>> failures.
> >>>>>
> >>>>> Make sure the VM stop and nothing worse happens if such an error occurs.
> >>>>>
> >>>>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>

> >> QEMU 8.0 is already released so I think it's time to revisit this.
> >
> > Two months ago would have been a better time :-) We're heading up
> > towards softfreeze for 8.1 in about three weeks from now.

> Hi Peter,
>
> Please apply this.

Looking again at the patch I see it hasn't been reviewed by
anybody on the KVM side of things. Paolo, does this seem like
the right way to handle errors from kvm_arch_get_registers()
and kvm_arch_put_registers() ?

The original patch is at:
https://patchew.org/QEMU/20221201102728.69751-1-akihiko.odaki@daynix.com/

thanks
-- PMM
Re: [PATCH] accel/kvm/kvm-all: Handle register access errors
Posted by Peter Maydell 1 year, 5 months ago 
On Thu, 1 Dec 2022 at 11:00, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> On 2022/12/01 19:40, Peter Maydell wrote:
> > On Thu, 1 Dec 2022 at 10:27, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
> >>
> >> A register access error typically means something seriously wrong
> >> happened so that anything bad can happen after that and recovery is
> >> impossible.
> >> Even failing one register access is catastorophic as
> >> architecture-specific code are not written so that it torelates such
> >> failures.
> >>
> >> Make sure the VM stop and nothing worse happens if such an error occurs.
> >>
> >> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> >
> > In a similar vein there was also
> > https://lore.kernel.org/all/20220617144857.34189-1-peterx@redhat.com/
> > back in June, which on the one hand was less comprehensive but on
> > the other does the plumbing to pass the error upwards rather than
> > reporting it immediately at point of failure.
> >
> > I'm in principle in favour but suspect we'll run into some corner
> > cases where we were happily ignoring not-very-important failures
> > (eg if you're running Linux as the host OS on a Mac M1 and your
> > host kernel doesn't have this fix:
> > https://lore.kernel.org/all/YnHz6Cw5ONR2e+KA@google.com/T/
> > then QEMU will go from "works by sheer luck" to "consistently
> > hits this error check"). So we should aim to land this extra
> > error checking early in the release cycle so we have plenty of
> > time to deal with any bug reports we get about it.

> Actually I found this problem when I tried to run QEMU with KVM on M2
> MacBook Air and encountered a failure described and fixed at:
> https://lore.kernel.org/all/20221201104914.28944-2-akihiko.odaki@daynix.com/

Ah, yeah, you're trying to run QEMU+KVM on a heterogenous cluster.
You need to force all the vCPUs to run on only a single host
CPU type. It's a shame the error-reporting for this situation
is not very good, but there's not really any way to tell in
advance, the best you get is an error at the point where a vCPU
happens to migrate over to a different host CPU.

> Although the affected register was not really important, QEMU couldn't
> run the guest well enough because kvm_arch_put_registers for ARM64 is
> written in a way that it fails early. I guess the situation is not so
> different for other architectures as well.

I think Arm is the only one that does this kind of "leave the
handling of the system registers up to the host kernel and treat
them as mostly black-box values to be passed around on migration"
approach. Most other architectures have QEMU know about specific system
registers in the vCPU and only ask the kernel about those, I think.

-- PMM

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.