On 2/20/25 3:55 AM, Igor Mammedov wrote:
On Fri, 14 Feb 2025 14:16:35 +1000
Gavin Shan <gs...@redhat.com> wrote:
The error -1 is returned if the previously reported CPER error
hasn't been claimed. The virtual machine is terminated due to
abort(). It's conflicting to the ideal behaviour that the affected
vCPU retries pushing the CPER error in this case since the vCPU
can't proceed its execution.
Move the chunk of code to push CPER error to a separate helper
report_memory_errors() and retry the request when the return
value from acpi_ghes_memory_errors() is greater than zero.
Signed-off-by: Gavin Shan <gs...@redhat.com>
---
target/arm/kvm.c | 31 +++++++++++++++++++++++++------
1 file changed, 25 insertions(+), 6 deletions(-)
diff --git a/target/arm/kvm.c b/target/arm/kvm.c
index 5c0bf99aec..9f063f6053 100644
--- a/target/arm/kvm.c
+++ b/target/arm/kvm.c
@@ -2362,6 +2362,30 @@ int kvm_arch_get_registers(CPUState *cs, Error **errp)
return ret;
}
+static void report_memory_error(CPUState *c, hwaddr paddr)
+{
+ int ret;
+
+ while (true) {
+ /* Retry if the previously report error hasn't been claimed */
+ ret = acpi_ghes_memory_errors(ACPI_HEST_SRC_ID_SEA, paddr, true);
+ if (ret <= 0) {
+ break;
+ }
+
+ bql_unlock();
+ g_usleep(1000);
Igor, thanks for the detailed comments. Sorry for a bit delay of the reply, I
was checking the code to understand it better :)
even with bql released it's not safe to loop in here.
consider,
a guest with 2 vcpus
* vcpu 1 gets SIGBUS due to error
* vcpu 2 trips over the same error and gets into this loop
* on guest side vcpu 1 continues to run to handle SEA but
might need to acquire a lock that vcpu 2 holds
Agreed.
GHESv2 error source we support, can report several errors,
currently QEMU supports only 1 'error status block' which
can hold several error records (CPER) (though storage size is limited)
1:
We can potentially add support for more GHESv2 error sources
with their own Read ACK registers (let's say =max_cpus)
(that is under assumption that no other error will be
triggered while guest VCPUs handle their own SEA (upto clearing Read ACK))
2:
Another way could be for QEMU to allocate more error status _blocks_
for the only one error source it has now and try to find
empty status block to inject new error(s).
* it can be saturated with high rate of errors (so what do we do in case it
happens?)
* subject to race between clearing/setting Read ACK
(maybe it can dealt with that on side by keeping internal read_ack counter)
3:
And alternatively, queue incoming errors until read ack is cleared
and then inject pending errors in one go.
(problem with that is that at the moment QEMU doesn't monitor
read ack register memory so it won't notice guest clearing that)
Given spec has provision for multiple error status blocks/error data entries
it seems that #2 is an expected way to deal with the problem.
I would say #1 is the ideal model because the read_ack_register is the
bottleneck
and it should be scaled up to max_cpus. In that way, the bottleneck can be
avoided
from the bottom. Another benefit with #1 is the error can be delivered
immediately
to the vCPU where the error was raised. This matches with the syntax of SEA to
me.
#2 still has the risk to saturate the multiple error status blocks if there are
high rate of errors as you said. Besides, the vCPU where read_ack_register is
acknoledged
can be different from the vCPU where the error is raised, violating the syntax
of
SEA.
#3's drawback is to violate the syntax of SEA, similar to #2.
However, #2/#3 wouldn't be that complicated to #1. I didn't expect big surgery
to
GHES module, but it seems there isn't perfect solution without a big surgery.
I would vote for #1 to resolve the issue from the ground. What do you think,
Igor?
I'm also hoping Jonathan and Mauro can provide their preference.
PS:
I'd prefer Mauro's series being merged 1st (once it's resplit),
for it refactors a bunch of original code and hopefully makes
code easier to follow/extend.
Sure. I won't start the coding until the solution is confirmed. All the followup
work will base on Mauro's series.
+ bql_lock();
+ }
+
+ if (ret == 0) {
+ kvm_inject_arm_sea(c);
+ } else {
+ error_report("Error %d to report memory error", ret);
+ abort();
+ }
+}
+
void kvm_arch_on_sigbus_vcpu(CPUState *c, int code, void *addr)
{
ram_addr_t ram_addr;
@@ -2387,12 +2411,7 @@ void kvm_arch_on_sigbus_vcpu(CPUState *c, int code, void
*addr)
*/
if (code == BUS_MCEERR_AR) {
kvm_cpu_synchronize_state(c);
- if (!acpi_ghes_memory_errors(ACPI_HEST_SRC_ID_SEA, paddr,
false)) {
- kvm_inject_arm_sea(c);
- } else {
- error_report("failed to record the error");
- abort();
- }
+ report_memory_error(c, paddr);
}
return;
}
Thanks,
Gavin
