[PATCH RFC] arm/kvm: report registers we failed to set

Cornelia Huck posted 1 patch 6 months, 3 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20250721161932.548668-1-cohuck@redhat.com
Maintainers: Peter Maydell <peter.maydell@linaro.org>, Paolo Bonzini <pbonzini@redhat.com>
There is a newer version of this series
target/arm/kvm.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 53 insertions(+)
[PATCH RFC] arm/kvm: report registers we failed to set
Posted by Cornelia Huck 6 months, 3 weeks ago
If we fail migration because of a mismatch of some registers between
source and destination, the error message is not very informative:

qemu-system-aarch64: error while loading state for instance 0x0 ofdevice 'cpu'
qemu-system-aarch64: Failed to put registers after init: Invalid argument

At least try to give the user a hint which registers had a problem,
even if they cannot really do anything about it right now.

Sample output:

Could not set register op0:3 op1:0 crn:0 crm:0 op2:0 to c00fac31 (is 413fd0c1)

We could be even more helpful once we support writable ID registers,
at which point the user might actually be able to configure something
that is migratable.

Suggested-by: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Cornelia Huck <cohuck@redhat.com>
---

Notes:
- This currently prints the list of failing registers for every call to
  write_list_to_kvmstate(), in particular for every cpu -- we might want
  to reduce that.
- If the macros aren't too ugly (or we manage to improve them), there
  might be other places where they could be useful.

---
 target/arm/kvm.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/target/arm/kvm.c b/target/arm/kvm.c
index 667234485547..ac6502e0c78f 100644
--- a/target/arm/kvm.c
+++ b/target/arm/kvm.c
@@ -900,6 +900,24 @@ bool write_kvmstate_to_list(ARMCPU *cpu)
     return ok;
 }
 
+/* pretty-print a KVM register */
+#define CP_REG_ARM64_SYSREG_OP(_reg, _op)                       \
+    ((uint8_t)((_reg & CP_REG_ARM64_SYSREG_ ## _op ## _MASK) >> \
+               CP_REG_ARM64_SYSREG_ ## _op ## _SHIFT))
+
+#define PRI_CP_REG_ARM64_SYSREG(_reg)                    \
+    ({                                                   \
+        char _out[32];                                   \
+        snprintf(_out, sizeof(_out),                     \
+                 "op0:%d op1:%d crn:%d crm:%d op2:%d",   \
+                 CP_REG_ARM64_SYSREG_OP(_reg, OP0),      \
+                 CP_REG_ARM64_SYSREG_OP(_reg, OP1),      \
+                 CP_REG_ARM64_SYSREG_OP(_reg, CRN),      \
+                 CP_REG_ARM64_SYSREG_OP(_reg, CRM),      \
+                 CP_REG_ARM64_SYSREG_OP(_reg, OP2));     \
+        _out;                                            \
+    })
+
 bool write_list_to_kvmstate(ARMCPU *cpu, int level)
 {
     CPUState *cs = CPU(cpu);
@@ -932,6 +950,41 @@ bool write_list_to_kvmstate(ARMCPU *cpu, int level)
              * a different value from what it actually contains".
              */
             ok = false;
+            switch (ret) {
+            case -ENOENT:
+                error_report("Could not set register %s: unknown to KVM",
+                             PRI_CP_REG_ARM64_SYSREG(regidx));
+                break;
+            case -EINVAL:
+                if ((regidx & KVM_REG_SIZE_MASK) == KVM_REG_SIZE_U32) {
+                    if (!kvm_get_one_reg(cs, regidx, &v32)) {
+                        error_report("Could not set register %s to %x (is %x)",
+                                     PRI_CP_REG_ARM64_SYSREG(regidx),
+                                     (uint32_t)cpu->cpreg_values[i], v32);
+                    } else {
+                        error_report("Could not set register %s to %x",
+                                     PRI_CP_REG_ARM64_SYSREG(regidx),
+                                     (uint32_t)cpu->cpreg_values[i]);
+                    }
+                } else /* U64 */ {
+                    uint64_t v64;
+
+                    if (!kvm_get_one_reg(cs, regidx, &v64)) {
+                        error_report("Could not set register %s to %lx (is %lx)",
+                                     PRI_CP_REG_ARM64_SYSREG(regidx),
+                                     cpu->cpreg_values[i], v64);
+                    } else {
+                        error_report("Could not set register %s to %lx",
+                                     PRI_CP_REG_ARM64_SYSREG(regidx),
+                                     cpu->cpreg_values[i]);
+                    }
+                }
+                break;
+            default:
+                error_report("Could not set register %s: %s",
+                             PRI_CP_REG_ARM64_SYSREG(regidx),
+                             strerror(-ret));
+            }
         }
     }
     return ok;
-- 
2.50.0
Re: [PATCH RFC] arm/kvm: report registers we failed to set
Posted by Eric Auger 5 months, 4 weeks ago
Hi Connie,

On 7/21/25 6:19 PM, Cornelia Huck wrote:
> If we fail migration because of a mismatch of some registers between
> source and destination, the error message is not very informative:
>
> qemu-system-aarch64: error while loading state for instance 0x0 ofdevice 'cpu'
> qemu-system-aarch64: Failed to put registers after init: Invalid argument
>
> At least try to give the user a hint which registers had a problem,
> even if they cannot really do anything about it right now.
>
> Sample output:
>
> Could not set register op0:3 op1:0 crn:0 crm:0 op2:0 to c00fac31 (is 413fd0c1)
>
> We could be even more helpful once we support writable ID registers,
> at which point the user might actually be able to configure something
> that is migratable.
>
> Suggested-by: Eric Auger <eric.auger@redhat.com>
> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> ---
>
> Notes:
> - This currently prints the list of failing registers for every call to
>   write_list_to_kvmstate(), in particular for every cpu -- we might want
>   to reduce that.
> - If the macros aren't too ugly (or we manage to improve them), there
>   might be other places where they could be useful.
>
> ---
>  target/arm/kvm.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 53 insertions(+)
>
> diff --git a/target/arm/kvm.c b/target/arm/kvm.c
> index 667234485547..ac6502e0c78f 100644
> --- a/target/arm/kvm.c
> +++ b/target/arm/kvm.c
> @@ -900,6 +900,24 @@ bool write_kvmstate_to_list(ARMCPU *cpu)
>      return ok;
>  }
>  
> +/* pretty-print a KVM register */
> +#define CP_REG_ARM64_SYSREG_OP(_reg, _op)                       \
> +    ((uint8_t)((_reg & CP_REG_ARM64_SYSREG_ ## _op ## _MASK) >> \
> +               CP_REG_ARM64_SYSREG_ ## _op ## _SHIFT))
> +
> +#define PRI_CP_REG_ARM64_SYSREG(_reg)                    \
> +    ({                                                   \
> +        char _out[32];                                   \
> +        snprintf(_out, sizeof(_out),                     \
> +                 "op0:%d op1:%d crn:%d crm:%d op2:%d",   \
> +                 CP_REG_ARM64_SYSREG_OP(_reg, OP0),      \
> +                 CP_REG_ARM64_SYSREG_OP(_reg, OP1),      \
> +                 CP_REG_ARM64_SYSREG_OP(_reg, CRN),      \
> +                 CP_REG_ARM64_SYSREG_OP(_reg, CRM),      \
> +                 CP_REG_ARM64_SYSREG_OP(_reg, OP2));     \
> +        _out;                                            \
> +    })
> +
I am afraid this is too simplistic.
Refering to linux/Documentation/virt/kvm/api.rst 4.68 KVM_SET_ONE_REG
ARM registers section
there are different groups of registers (upper 16b) and not all regs are
further identified by op0-2, crn, crm.
I think it would be valuable to output the group type and then the
formatted lower 16b, depending on the group type.

For instance 64b ARM FW pseudo reg is formatted as
0x6030 0000 0014 <regno:16>

a diff on reg 0 results in
qemu-system-aarch64: Could not set register op0:0 op1:0 crn:0 crm:0
op2:0 to 10003 (is 10001)
qemu-system-aarch64: error while loading state for instance 0x0 of
device 'cpu'
qemu-system-aarch64: Could not set register op0:0 op1:0 crn:0 crm:0
op2:0 to 10003 (is 10001)
qemu-system-aarch64: Failed to put registers after init: Invalid argument

Thanks

Eric
>  bool write_list_to_kvmstate(ARMCPU *cpu, int level)
>  {
>      CPUState *cs = CPU(cpu);
> @@ -932,6 +950,41 @@ bool write_list_to_kvmstate(ARMCPU *cpu, int level)
>               * a different value from what it actually contains".
>               */
>              ok = false;
> +            switch (ret) {
> +            case -ENOENT:
> +                error_report("Could not set register %s: unknown to KVM",
> +                             PRI_CP_REG_ARM64_SYSREG(regidx));
> +                break;
> +            case -EINVAL:
> +                if ((regidx & KVM_REG_SIZE_MASK) == KVM_REG_SIZE_U32) {
> +                    if (!kvm_get_one_reg(cs, regidx, &v32)) {
> +                        error_report("Could not set register %s to %x (is %x)",
> +                                     PRI_CP_REG_ARM64_SYSREG(regidx),
> +                                     (uint32_t)cpu->cpreg_values[i], v32);
> +                    } else {
> +                        error_report("Could not set register %s to %x",
> +                                     PRI_CP_REG_ARM64_SYSREG(regidx),
> +                                     (uint32_t)cpu->cpreg_values[i]);
> +                    }
> +                } else /* U64 */ {
> +                    uint64_t v64;
> +
> +                    if (!kvm_get_one_reg(cs, regidx, &v64)) {
> +                        error_report("Could not set register %s to %lx (is %lx)",
> +                                     PRI_CP_REG_ARM64_SYSREG(regidx),
> +                                     cpu->cpreg_values[i], v64);
> +                    } else {
> +                        error_report("Could not set register %s to %lx",
> +                                     PRI_CP_REG_ARM64_SYSREG(regidx),
> +                                     cpu->cpreg_values[i]);
> +                    }
> +                }
> +                break;
> +            default:
> +                error_report("Could not set register %s: %s",
> +                             PRI_CP_REG_ARM64_SYSREG(regidx),
> +                             strerror(-ret));
> +            }
>          }
>      }
>      return ok;
Re: [PATCH RFC] arm/kvm: report registers we failed to set
Posted by Cornelia Huck 5 months, 4 weeks ago
On Tue, Aug 12 2025, Eric Auger <eric.auger@redhat.com> wrote:

> Hi Connie,
>
> On 7/21/25 6:19 PM, Cornelia Huck wrote:
>> If we fail migration because of a mismatch of some registers between
>> source and destination, the error message is not very informative:
>>
>> qemu-system-aarch64: error while loading state for instance 0x0 ofdevice 'cpu'
>> qemu-system-aarch64: Failed to put registers after init: Invalid argument
>>
>> At least try to give the user a hint which registers had a problem,
>> even if they cannot really do anything about it right now.
>>
>> Sample output:
>>
>> Could not set register op0:3 op1:0 crn:0 crm:0 op2:0 to c00fac31 (is 413fd0c1)
>>
>> We could be even more helpful once we support writable ID registers,
>> at which point the user might actually be able to configure something
>> that is migratable.
>>
>> Suggested-by: Eric Auger <eric.auger@redhat.com>
>> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
>> ---
>>
>> Notes:
>> - This currently prints the list of failing registers for every call to
>>   write_list_to_kvmstate(), in particular for every cpu -- we might want
>>   to reduce that.
>> - If the macros aren't too ugly (or we manage to improve them), there
>>   might be other places where they could be useful.
>>
>> ---
>>  target/arm/kvm.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 53 insertions(+)
>>
>> diff --git a/target/arm/kvm.c b/target/arm/kvm.c
>> index 667234485547..ac6502e0c78f 100644
>> --- a/target/arm/kvm.c
>> +++ b/target/arm/kvm.c
>> @@ -900,6 +900,24 @@ bool write_kvmstate_to_list(ARMCPU *cpu)
>>      return ok;
>>  }
>>  
>> +/* pretty-print a KVM register */
>> +#define CP_REG_ARM64_SYSREG_OP(_reg, _op)                       \
>> +    ((uint8_t)((_reg & CP_REG_ARM64_SYSREG_ ## _op ## _MASK) >> \
>> +               CP_REG_ARM64_SYSREG_ ## _op ## _SHIFT))
>> +
>> +#define PRI_CP_REG_ARM64_SYSREG(_reg)                    \
>> +    ({                                                   \
>> +        char _out[32];                                   \
>> +        snprintf(_out, sizeof(_out),                     \
>> +                 "op0:%d op1:%d crn:%d crm:%d op2:%d",   \
>> +                 CP_REG_ARM64_SYSREG_OP(_reg, OP0),      \
>> +                 CP_REG_ARM64_SYSREG_OP(_reg, OP1),      \
>> +                 CP_REG_ARM64_SYSREG_OP(_reg, CRN),      \
>> +                 CP_REG_ARM64_SYSREG_OP(_reg, CRM),      \
>> +                 CP_REG_ARM64_SYSREG_OP(_reg, OP2));     \
>> +        _out;                                            \
>> +    })
>> +
> I am afraid this is too simplistic.
> Refering to linux/Documentation/virt/kvm/api.rst 4.68 KVM_SET_ONE_REG
> ARM registers section
> there are different groups of registers (upper 16b) and not all regs are
> further identified by op0-2, crn, crm.
> I think it would be valuable to output the group type and then the
> formatted lower 16b, depending on the group type.
>
> For instance 64b ARM FW pseudo reg is formatted as
> 0x6030 0000 0014 <regno:16>
>
> a diff on reg 0 results in
> qemu-system-aarch64: Could not set register op0:0 op1:0 crn:0 crm:0
> op2:0 to 10003 (is 10001)
> qemu-system-aarch64: error while loading state for instance 0x0 of
> device 'cpu'
> qemu-system-aarch64: Could not set register op0:0 op1:0 crn:0 crm:0
> op2:0 to 10003 (is 10001)
> qemu-system-aarch64: Failed to put registers after init: Invalid argument

You mean smth like

Could not set FW pseudo register <regno> to <val> (is <val>)
Could not set ID register <decoding> to <val> (is <val>)

?
Re: [PATCH RFC] arm/kvm: report registers we failed to set
Posted by Eric Auger 5 months, 4 weeks ago

On 8/13/25 12:01 PM, Cornelia Huck wrote:
> On Tue, Aug 12 2025, Eric Auger <eric.auger@redhat.com> wrote:
>
>> Hi Connie,
>>
>> On 7/21/25 6:19 PM, Cornelia Huck wrote:
>>> If we fail migration because of a mismatch of some registers between
>>> source and destination, the error message is not very informative:
>>>
>>> qemu-system-aarch64: error while loading state for instance 0x0 ofdevice 'cpu'
>>> qemu-system-aarch64: Failed to put registers after init: Invalid argument
>>>
>>> At least try to give the user a hint which registers had a problem,
>>> even if they cannot really do anything about it right now.
>>>
>>> Sample output:
>>>
>>> Could not set register op0:3 op1:0 crn:0 crm:0 op2:0 to c00fac31 (is 413fd0c1)
>>>
>>> We could be even more helpful once we support writable ID registers,
>>> at which point the user might actually be able to configure something
>>> that is migratable.
>>>
>>> Suggested-by: Eric Auger <eric.auger@redhat.com>
>>> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
>>> ---
>>>
>>> Notes:
>>> - This currently prints the list of failing registers for every call to
>>>   write_list_to_kvmstate(), in particular for every cpu -- we might want
>>>   to reduce that.
>>> - If the macros aren't too ugly (or we manage to improve them), there
>>>   might be other places where they could be useful.
>>>
>>> ---
>>>  target/arm/kvm.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 53 insertions(+)
>>>
>>> diff --git a/target/arm/kvm.c b/target/arm/kvm.c
>>> index 667234485547..ac6502e0c78f 100644
>>> --- a/target/arm/kvm.c
>>> +++ b/target/arm/kvm.c
>>> @@ -900,6 +900,24 @@ bool write_kvmstate_to_list(ARMCPU *cpu)
>>>      return ok;
>>>  }
>>>  
>>> +/* pretty-print a KVM register */
>>> +#define CP_REG_ARM64_SYSREG_OP(_reg, _op)                       \
>>> +    ((uint8_t)((_reg & CP_REG_ARM64_SYSREG_ ## _op ## _MASK) >> \
>>> +               CP_REG_ARM64_SYSREG_ ## _op ## _SHIFT))
>>> +
>>> +#define PRI_CP_REG_ARM64_SYSREG(_reg)                    \
>>> +    ({                                                   \
>>> +        char _out[32];                                   \
>>> +        snprintf(_out, sizeof(_out),                     \
>>> +                 "op0:%d op1:%d crn:%d crm:%d op2:%d",   \
>>> +                 CP_REG_ARM64_SYSREG_OP(_reg, OP0),      \
>>> +                 CP_REG_ARM64_SYSREG_OP(_reg, OP1),      \
>>> +                 CP_REG_ARM64_SYSREG_OP(_reg, CRN),      \
>>> +                 CP_REG_ARM64_SYSREG_OP(_reg, CRM),      \
>>> +                 CP_REG_ARM64_SYSREG_OP(_reg, OP2));     \
>>> +        _out;                                            \
>>> +    })
>>> +
>> I am afraid this is too simplistic.
>> Refering to linux/Documentation/virt/kvm/api.rst 4.68 KVM_SET_ONE_REG
>> ARM registers section
>> there are different groups of registers (upper 16b) and not all regs are
>> further identified by op0-2, crn, crm.
>> I think it would be valuable to output the group type and then the
>> formatted lower 16b, depending on the group type.
>>
>> For instance 64b ARM FW pseudo reg is formatted as
>> 0x6030 0000 0014 <regno:16>
>>
>> a diff on reg 0 results in
>> qemu-system-aarch64: Could not set register op0:0 op1:0 crn:0 crm:0
>> op2:0 to 10003 (is 10001)
>> qemu-system-aarch64: error while loading state for instance 0x0 of
>> device 'cpu'
>> qemu-system-aarch64: Could not set register op0:0 op1:0 crn:0 crm:0
>> op2:0 to 10003 (is 10001)
>> qemu-system-aarch64: Failed to put registers after init: Invalid argument
> You mean smth like
>
> Could not set FW pseudo register <regno> to <val> (is <val>)
> Could not set ID register <decoding> to <val> (is <val>)
yes

Eric
>
> ?
>
Re: [PATCH RFC] arm/kvm: report registers we failed to set
Posted by Sebastian Ott 6 months, 2 weeks ago
On Mon, 21 Jul 2025, Cornelia Huck wrote:
> If we fail migration because of a mismatch of some registers between
> source and destination, the error message is not very informative:
>
> qemu-system-aarch64: error while loading state for instance 0x0 ofdevice 'cpu'
> qemu-system-aarch64: Failed to put registers after init: Invalid argument
>
> At least try to give the user a hint which registers had a problem,
> even if they cannot really do anything about it right now.
>
> Sample output:
>
> Could not set register op0:3 op1:0 crn:0 crm:0 op2:0 to c00fac31 (is 413fd0c1)
>
> We could be even more helpful once we support writable ID registers,
> at which point the user might actually be able to configure something
> that is migratable.

That message will be even more helpful once we have the stuff in place
to actually print a reg name here (and be able to change the values for
the guest) - but I agree this would already be a great improvement over
the current message!

>
> Suggested-by: Eric Auger <eric.auger@redhat.com>
> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> ---
>
> Notes:
> - This currently prints the list of failing registers for every call to
>  write_list_to_kvmstate(), in particular for every cpu -- we might want
>  to reduce that.
> - If the macros aren't too ugly (or we manage to improve them), there
>  might be other places where they could be useful.
>
> ---
> target/arm/kvm.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 53 insertions(+)
>
> diff --git a/target/arm/kvm.c b/target/arm/kvm.c
> index 667234485547..ac6502e0c78f 100644
> --- a/target/arm/kvm.c
> +++ b/target/arm/kvm.c
> @@ -900,6 +900,24 @@ bool write_kvmstate_to_list(ARMCPU *cpu)
>     return ok;
> }
>
> +/* pretty-print a KVM register */
> +#define CP_REG_ARM64_SYSREG_OP(_reg, _op)                       \
> +    ((uint8_t)((_reg & CP_REG_ARM64_SYSREG_ ## _op ## _MASK) >> \
> +               CP_REG_ARM64_SYSREG_ ## _op ## _SHIFT))
> +
> +#define PRI_CP_REG_ARM64_SYSREG(_reg)                    \
> +    ({                                                   \
> +        char _out[32];                                   \
> +        snprintf(_out, sizeof(_out),                     \
> +                 "op0:%d op1:%d crn:%d crm:%d op2:%d",   \
> +                 CP_REG_ARM64_SYSREG_OP(_reg, OP0),      \
> +                 CP_REG_ARM64_SYSREG_OP(_reg, OP1),      \
> +                 CP_REG_ARM64_SYSREG_OP(_reg, CRN),      \
> +                 CP_REG_ARM64_SYSREG_OP(_reg, CRM),      \
> +                 CP_REG_ARM64_SYSREG_OP(_reg, OP2));     \
> +        _out;                                            \
> +    })

Gcc correctly complains about a dangling pointer here.

Sebastian
Re: [PATCH RFC] arm/kvm: report registers we failed to set
Posted by Cornelia Huck 6 months, 2 weeks ago
On Tue, Jul 22 2025, Sebastian Ott <sebott@redhat.com> wrote:

> On Mon, 21 Jul 2025, Cornelia Huck wrote:
>> +/* pretty-print a KVM register */
>> +#define CP_REG_ARM64_SYSREG_OP(_reg, _op)                       \
>> +    ((uint8_t)((_reg & CP_REG_ARM64_SYSREG_ ## _op ## _MASK) >> \
>> +               CP_REG_ARM64_SYSREG_ ## _op ## _SHIFT))
>> +
>> +#define PRI_CP_REG_ARM64_SYSREG(_reg)                    \
>> +    ({                                                   \
>> +        char _out[32];                                   \
>> +        snprintf(_out, sizeof(_out),                     \
>> +                 "op0:%d op1:%d crn:%d crm:%d op2:%d",   \
>> +                 CP_REG_ARM64_SYSREG_OP(_reg, OP0),      \
>> +                 CP_REG_ARM64_SYSREG_OP(_reg, OP1),      \
>> +                 CP_REG_ARM64_SYSREG_OP(_reg, CRN),      \
>> +                 CP_REG_ARM64_SYSREG_OP(_reg, CRM),      \
>> +                 CP_REG_ARM64_SYSREG_OP(_reg, OP2));     \
>> +        _out;                                            \
>> +    })
>
> Gcc correctly complains about a dangling pointer here.

Hum, I maybe should switch to a system with a less ancient
compiler... thanks for testing.