Question about SEA handling process happened in user space

I'm a little confused about the handling process of SEA happened in user space.

Following the description of FnV bit of register ESR_ELx in ARMv8.4 SPEC,FAR is valid only for synchronous External abort on a translation table walk.

But for this FAR valid scenario(attached code from line 684 to 687),
we send signal SIGKILL to kill process. For some other scenario, such as line 680, FAR is not valid, but we send SIGBUS and transfer error address to process to try to do some recovery.

should it be an problem ?

680         { do_sea,               SIGBUS,  BUS_OBJERR,    "synchronous external abort"    },
684         { do_sea,               SIGKILL, SI_KERNEL,     "level 0 (translation table walk)"      },
685         { do_sea,               SIGKILL, SI_KERNEL,     "level 1 (translation table walk)"      },
686         { do_sea,               SIGKILL, SI_KERNEL,     "level 2 (translation table walk)"      },
687         { do_sea,               SIGKILL, SI_KERNEL,     "level 3 (translation table walk)"      },
688         { do_sea,               SIGBUS,  BUS_OBJERR,    "synchronous parity or ECC error" },    // Reserved when RAS is implemented
692         { do_sea,               SIGKILL, SI_KERNEL,     "level 0 synchronous parity error (translation table walk)"     },      // Reserved when RAS is implemented
693         { do_sea,               SIGKILL, SI_KERNEL,     "level 1 synchronous parity error (translation table walk)"     },      // Reserved when RAS is implemented
694         { do_sea,               SIGKILL, SI_KERNEL,     "level 2 synchronous parity error (translation table walk)"     },      // Reserved when RAS is implemented
695         { do_sea,               SIGKILL, SI_KERNEL,     "level 3 synchronous parity error (translation table walk)"     },      // Reserved when RAS is implemented
696         { do_bad,               SIGKILL, SI_KERNEL,     "unknown 32"                    },

Hi Xiaofei,

Quoted 8 lines · Xiaofei Tan

I'm a little confused about the handling process of SEA happened in user space.

Following the description of FnV bit of register ESR_ELx in ARMv8.4 SPEC,FAR is valid only for synchronous External abort on a translation table walk.

But for this FAR valid scenario(attached code from line 684 to 687),
we send signal SIGKILL to kill process. For some other scenario, such as line 680, FAR is not valid, but we send SIGBUS and transfer error address to process to try to do some recovery.

'FAR is not valid': its optional. The ESR_EL1.FnV bit can be set for the 'catch all' external abort fault-status-code. This lets the CPU tell us that it doesn't know what the faulting virtual address is.

do_sea() checks for this:

|	if (esr & ESR_ELx_FnV)
|		siaddr = NULL;
|	else
|		siaddr  = (void __user *)addr;

If we can't know the address, there isn't much we can do.
(This check is really making sure we don't pass junk to user-space when FnV is set)

Quoted 1 line

should it be an problem ?

I'm not quite sure what your question is.

If the CPU doesn't tell us the address, we can't tell user-space what it is. The alternative is to upgrade to SIGKILL in that case.

If you see this instead of the address provided via firmware-first, there is a series to improve that here:
https://lore.kernel.org/linux-acpi/[email protected]/

(We skip this signal code of APEI promises it did all the work. This lets you take the signal from memory_failure() instead, which may have better information.)

If its the SIGKILL entries: these are for the translation table walk. There is no point telling user-space about corruption in its page tables as it can't do anything about it. The kernel's handling of this is to kill the process. (page tables make up a very small amount of memory, so this should be rarer than the regular 'external abort' case)

Thanks,

James

Quoted 11 lines

680         { do_sea,               SIGBUS,  BUS_OBJERR,    "synchronous external abort"    },
684         { do_sea,               SIGKILL, SI_KERNEL,     "level 0 (translation table walk)"      },
685         { do_sea,               SIGKILL, SI_KERNEL,     "level 1 (translation table walk)"      },
686         { do_sea,               SIGKILL, SI_KERNEL,     "level 2 (translation table walk)"      },
687         { do_sea,               SIGKILL, SI_KERNEL,     "level 3 (translation table walk)"      },
688         { do_sea,               SIGBUS,  BUS_OBJERR,    "synchronous parity or ECC error" },    // Reserved when RAS is implemented
692         { do_sea,               SIGKILL, SI_KERNEL,     "level 0 synchronous parity error (translation table walk)"     },      // Reserved when RAS is implemented
693         { do_sea,               SIGKILL, SI_KERNEL,     "level 1 synchronous parity error (translation table walk)"     },      // Reserved when RAS is implemented
694         { do_sea,               SIGKILL, SI_KERNEL,     "level 2 synchronous parity error (translation table walk)"     },      // Reserved when RAS is implemented
695         { do_sea,               SIGKILL, SI_KERNEL,     "level 3 synchronous parity error (translation table walk)"     },      // Reserved when RAS is implemented
696         { do_bad,               SIGKILL, SI_KERNEL,     "unknown 32"                    },

.

Quoted 13 lines · James Morse

Quoted 8 lines · Xiaofei Tan

I'm a little confused about the handling process of SEA happened in user space.

Following the description of FnV bit of register ESR_ELx in ARMv8.4 SPEC,FAR is valid only for synchronous External abort on a translation table walk.

But for this FAR valid scenario(attached code from line 684 to 687),
we send signal SIGKILL to kill process. For some other scenario, such as line 680, FAR is not valid, but we send SIGBUS and transfer error address to process to try to do some recovery.

'FAR is not valid': its optional. The ESR_EL1.FnV bit can be set for the 'catch all' external abort fault-status-code. This lets the CPU tell us that it doesn't know what the faulting virtual address is.

Quoted 4 lines

I'm not quite sure what your question is.

If the CPU doesn't tell us the address, we can't tell user-space what it is. The alternative is to upgrade to SIGKILL in that case.

Got it. May be the description of FnV bit of register ESR_ELx is not quite exactly. Because following the code, CPU may still have an chance to tell the address for SEA, not on translation table walk.

Quoted 4 lines

If you see this instead of the address provided via firmware-first, there is a series to improve that here:
https://lore.kernel.org/linux-acpi/[email protected]/

Quoted 2 lines

(We skip this signal code of APEI promises it did all the work. This lets you take the signal from memory_failure() instead, which may have better information.)

This should be an great direction.
I have two concerns.
1.memory_failure() is only called for "memory error section" record. Then should we use this memory record for ghes sea report? Our platform is using "ARM processor error section".

2.Should we define an error source structure for each cpu core in HEST table? If not, there may be conflict if more than one cpu core fall into SEA.

Quoted 5 lines

If its the SIGKILL entries: these are for the translation table walk. There is no point telling user-space about corruption in its page tables as it can't do anything about it. The kernel's handling of this is to kill the process. (page tables make up a very small amount of memory, so this should be rarer than the regular 'external abort' case)

Hmm, then it is useless that CPU record address for this entries.

Quoted 15 lines · Xiaofei Tan

Quoted 13 lines · James Morse

Quoted 8 lines · Xiaofei Tan

I'm a little confused about the handling process of SEA happened in user space.

Following the description of FnV bit of register ESR_ELx in ARMv8.4 SPEC,FAR is valid only for synchronous External abort on a translation table walk.

But for this FAR valid scenario(attached code from line 684 to 687),
we send signal SIGKILL to kill process. For some other scenario, such as line 680, FAR is not valid, but we send SIGBUS and transfer error address to process to try to do some recovery.

'FAR is not valid': its optional. The ESR_EL1.FnV bit can be set for the 'catch all' external abort fault-status-code. This lets the CPU tell us that it doesn't know what the faulting virtual address is.

Quoted 7 lines

Quoted 4 lines

I'm not quite sure what your question is.

If the CPU doesn't tell us the address, we can't tell user-space what it is. The alternative is to upgrade to SIGKILL in that case.

Got it. May be the description of FnV bit of register ESR_ELx is not quite exactly. Because following the code, CPU may still have an chance to tell the address for SEA, not on translation table walk.

Its up to the CPU. If it has a VA for this fault, it can store it in FAR_EL1. If it doesn't, it can set ESR_EL1.FnV to say the value in FAR_EL1 is UNKNOWN.

(these are some made up examples, I don't know how any particular CPU does this...) For example, the address translation may be the last thing the CPU does. When it gets an error, it still has the VA address on hand, and can report it in FAR_EL1. Another CPU may do all the address translation early, when it gets an error, all it has is the physical address, which it can't put in FAR_EL1.

For the translation table walks, the CPU obviously has to have the VA on hand to do the walk, so its expected to report it.

Quoted 7 lines

Quoted 7 lines

If you see this instead of the address provided via firmware-first, there is a series to improve that here:
https://lore.kernel.org/linux-acpi/[email protected]/

(We skip this signal code of APEI promises it did all the work. This lets you take the signal from memory_failure() instead, which may have better information.)

Quoted 5 lines

This should be an great direction.
I have two concerns.
1.memory_failure() is only called for "memory error section" record. Then should we use this memory record for ghes sea report? Our platform is using "ARM processor error section".

For what classes of error?

If memory has become corrupted, you should tell the OS about the memory error.

Quoted 1 line

From (my) memory: linux will just print out 'processor errors', and panic() if

they are marked as fatal. I don't think you can use these to convey a memory error...

Quoted 3 lines

2.Should we define an error source structure for each cpu core in HEST table? If not, there may be conflict if more than one cpu core fall into SEA.

This is a question for the people who wrote your firmware.
For firmware first, you must have set SCR_EL3.EA. What does your firmware do if two CPUs take an external abort at the same time?

Each CPU having its own area to read/write CPER would mean you need one NOTIFY_SEA entry in the HEST for each area ... but how does the OS know which CPU is which?

I think its better for there to be one area for CPER. If a second CPU takes an external abort while the first is processing it, it has to be held in firmware until the GHESv2 ACK says the area is no longer in use.
This way firmware guarantees the CPU taking the emulated external abort, will always find its records in the CPER area.

Quoted 8 lines

Quoted 6 lines

If its the SIGKILL entries: these are for the translation table walk. There is no point telling user-space about corruption in its page tables as it can't do anything about it. The kernel's handling of this is to kill the process. (page tables make up a very small amount of memory, so this should be rarer than the regular 'external abort' case)

Hmm, then it is useless that CPU record address for this entries.

An OS that is better than linux may use FAR_EL1 to handle these errors!

Linux doesn't because user-space memory can be re-mapped by another CPU. We need to know the affected physical-address in order to handle the error, but can't know what that was if a remote CPU remapped the page between us taking the external abort, and do_sea() starting to walk the page-tables with FAR_EL1.

Firmware-first's memory-error description gives us the physical address if firmware can learn it by imp-def means. v8.2 RAS extensions gives us an ERRxADDR register that holds the physical-address.

Thanks,

James

.

Quoted 6 lines · James Morse

Quoted 3 lines · Xiaofei Tan

1.memory_failure() is only called for "memory error section" record. Then should we use this memory record for ghes sea report? Our platform is using "ARM processor error section".

For what classes of error?

Both processor cache ecc error and memory error (marked by poison) can lead to SEA.

Quoted 6 lines

If memory has become corrupted, you should tell the OS about the memory error.

Quoted 1 line

From (my) memory: linux will just print out 'processor errors', and panic() if

they are marked as fatal. I don't think you can use these to convey a memory error...

OK. Then firmware should detect error source. If it is processor cache error,
we use "ARM processor error section", else if it is memory error, we use "memory error section".

Normally, we report memory error only from RAS node of DDRC or HHA module. For SEA,

It is a little strange to report as memory error when collect errors from processor RAS node.

Quoted 6 lines

Quoted 2 lines

2.Should we define an error source structure for each cpu core in HEST table? If not, there may be conflict if more than one cpu core fall into SEA.

This is a question for the people who wrote your firmware.
For firmware first, you must have set SCR_EL3.EA. What does your firmware do if two CPUs take an external abort at the same time?

Will block the second one until first SEA finished and error source of HEST table free.

Quoted 3 lines

Each CPU having its own area to read/write CPER would mean you need one NOTIFY_SEA entry in the HEST for each area ... but how does the OS know which CPU is which?

Yes, OS don't know this.
So, it is ok to share the only one area for all CPUs.

Quoted 5 lines · Xiaofei Tan

Quoted 4 lines · James Morse

Quoted 3 lines · Xiaofei Tan

1.memory_failure() is only called for "memory error section" record. Then should we use this memory record for ghes sea report? Our platform is using "ARM processor error section".

Quoted 3 lines

Quoted 1 line

For what classes of error?

Both processor cache ecc error and memory error (marked by poison) can lead to SEA.

These are the errors that can cause the hardware to notify software via external abort. For which classes of error does your firmware then use a 'processor error'?

It sounds like you assume everything reported in the CPU records must be a processor error, and everything reported by the memory error must be a memory error.

(digression: this isn't really true!
The CPU could report that it read poison from memory. Is this a memory error, or a CPU error? Equally the memory controller could report that a PCIe device wrote to a not-present DIMM. Is this a memory error?)

Quoted 10 lines

Quoted 7 lines

If memory has become corrupted, you should tell the OS about the memory error.

Quoted 1 line

From (my) memory: linux will just print out 'processor errors', and panic() if

they are marked as fatal. I don't think you can use these to convey a memory error...

OK. Then firmware should detect error source. If it is processor cache error,
we use "ARM processor error section", else if it is memory error, we use "memory error section".

Quoted 1 line

Normally, we report memory error only from RAS node of DDRC or HHA module. For SEA,

Do you have patches to get linux to do something useful with the processor error nodes?

We'd need it to handle uncorrected cache errors with a physical address, as if they were memory errors...

A virtual address is no-use as the memory may have been re-mapped in the meantime.

Quoted 3 lines

It is a little strange to report as memory error when collect errors from processor RAS node.

Its pragmatic: today linux ignores the processor errors.
If you suffer a cache error, the memory that backed that cached location is now also corrupt, as you've lost the writes that made the cache-line dirty.

If you can describe this memory corruption, without treating it as 'the error' then an OS that doesn't know about the process error sections will still do the right thing. (i.e. leave out the device/row/rank stuff to avoid it being attributed to a DIMM)

The downside is you have fake memory errors when nothing bad happened to the DIMM. These should be uniform, and smaller than the errors actually occurring at the DIMM.

I've no idea if patches adding support for the processor error nodes would be considered for stable.

Quoted 9 lines

Quoted 7 lines

Quoted 3 lines

2.Should we define an error source structure for each cpu core in HEST table? If not, there may be conflict if more than one cpu core fall into SEA.

This is a question for the people who wrote your firmware.
For firmware first, you must have set SCR_EL3.EA. What does your firmware do if two CPUs take an external abort at the same time?

Will block the second one until first SEA finished and error source of HEST table free.

Okay, so one 'SEA' entry in the HEST describes the single region that CPER will be written to.

Quoted 6 lines

Quoted 3 lines

Each CPU having its own area to read/write CPER would mean you need one NOTIFY_SEA entry in the HEST for each area ... but how does the OS know which CPU is which?

Yes, OS don't know this.
So, it is ok to share the only one area for all CPUs.

Yes, as there is no way to pair the memory with the CPUs.

If there is more than one region, then each CPU taking an external abort will walk the list, checking each one.

Its up to firmware to ensure this is serialised. Sounds like you've got this sorted.

Thanks,

James

.

Quoted 4 lines · James Morse

Do you have patches to get linux to do something useful with the processor error nodes?

We'd need it to handle uncorrected cache errors with a physical address, as if they were memory errors...

Yes, we have some patches to do this thing inside. Then memory_failure() will be called for arm processor error section when physical address is available.

I think this part is worth improving.

BTW, should ARM processor record physical address when consumed an memory poison error for SEA? It is helpful to do error recovery. Is this mandatory for arm spec?

Quoted 3 lines · Xiaofei Tan

Quoted 2 lines · James Morse

Do you have patches to get linux to do something useful with the processor error nodes?

Quoted 5 lines

Quoted 2 lines

We'd need it to handle uncorrected cache errors with a physical address, as if they were memory errors...

Yes, we have some patches to do this thing inside. Then memory_failure() will be called for arm processor error section when physical address is available.

I look forward to reading them!

[...]

Quoted 4 lines

I think this part is worth improving.

BTW, should ARM processor record physical address when consumed an memory poison error for SEA? It is helpful to do error recovery. Is this mandatory for arm spec?

ERR<n>ADDR? Its not mandatory to be filled for any error. It can be some imp-def bus address or a virtual address.

It can also be left out if ERR<b>STATUS.AV says its not valid.

This is really a question for your hardware people.

Does your implementation always give a physical-address for a synchronous external abort?

Thanks,

James

.

Quoted 7 lines · James Morse

Quoted 5 lines · Xiaofei Tan

Quoted 4 lines · James Morse

Do you have patches to get linux to do something useful with the processor error nodes?

We'd need it to handle uncorrected cache errors with a physical address, as if they were memory errors...

Quoted 4 lines

Quoted 2 lines

Yes, we have some patches to do this thing inside. Then memory_failure() will be called for arm processor error section when physical address is available.

I look forward to reading them!

https://lkml.org/lkml/2018/1/26/197

Our guy tried to upstream it, but not accepted. :(

Quoted 9 lines

[...]

Quoted 4 lines

I think this part is worth improving.

BTW, should ARM processor record physical address when consumed an memory poison error for SEA? It is helpful to do error recovery. Is this mandatory for arm spec?

ERR<n>ADDR? Its not mandatory to be filled for any error. It can be some imp-def bus address or a virtual address.

virtual address ? but arm spec called it physical address.

Quoted 1 line

Does your implementation always give a physical-address for a synchronous external abort?

We hope so. But hardware guys say it is hard to record physical address for every situation.

Quoted 11 lines · Xiaofei Tan

Quoted 10 lines · James Morse

Quoted 8 lines · Xiaofei Tan

Quoted 4 lines · James Morse

Do you have patches to get linux to do something useful with the processor error nodes?

We'd need it to handle uncorrected cache errors with a physical address, as if they were memory errors...

Yes, we have some patches to do this thing inside. Then memory_failure() will be called for arm processor error section when physical address is available.

Quoted 5 lines

Quoted 1 line

I look forward to reading them!

https://lkml.org/lkml/2018/1/26/197

Our guy tried to upstream it, but not accepted. :(

Wrong series?

https://lkml.org/lkml/2018/1/26/194 is not creating any handing for processor error nodes.

That series tried to to suck all the pending errors out of the core code, into an arch specific queue:
| arch/arm64/kernel/ras.c | 173 +++++++++++++++++++++++++++++++++++

As far as I understand it, that was to ensure the memory_failure() work was done before we return to user-space.

My attempt to fix that got rolled up in the SDEI series. It was posted again here:
https://lore.kernel.org/linux-acpi/[email protected]/

If you need processor errors handling, there should be code added to the CPER_SEC_PROC_ARM else-if in ghes_do_proc() to do the handling.

You may end up duplicating bits of ghes_handle_memory_failure(), to report the memory errors that happened in the cache.
If you want to count corrected errors, a device in ghes_edac is probably the way to do that.

Quoted 5 lines

Quoted 5 lines

[...]

Quoted 1 line

I think this part is worth improving.

Quoted 7 lines

Quoted 5 lines

Quoted 2 lines

BTW, should ARM processor record physical address when consumed an memory poison error for SEA? It is helpful to do error recovery. Is this mandatory for arm spec?

ERR<n>ADDR? Its not mandatory to be filled for any error. It can be some imp-def bus address or a virtual address.

virtual address ? but arm spec called it physical address.

That was my recollection too! But I checked again before writing this:

"4.4.5 ERR<n>ADDR, Error Record Address Register" in
https://static.docs.arm.com/ddi0587/cb/2019_07_05_DD_0587_C_b.pdf

has a VA bit for a virtual-address, and 'AI' for this imp-def bus address, more properly described as on that "might not match the programmers' view of the physical address for the recorded location."

[...]

Quoted 3 lines

Quoted 1 line

Does your implementation always give a physical-address for a synchronous external abort?

We hope so. But hardware guys say it is hard to record physical address for every situation.

Yeah ...

Hopefully the situations where its too-hard are also the rarest, we can class these as fatal (because we can't handle them).

Thanks,

James

.

Hi Xiaofei,

Quoted 8 lines · Xiaofei Tan

I'm a little confused about the handling process of SEA happened in user space.

Following the description of FnV bit of register ESR_ELx in ARMv8.4 SPEC,FAR is valid only for synchronous External abort on a translation table walk.

But for this FAR valid scenario(attached code from line 684 to 687),
we send signal SIGKILL to kill process. For some other scenario, such as line 680, FAR is not valid, but we send SIGBUS and transfer error address to process to try to do some recovery.

'FAR is not valid': its optional. The ESR_EL1.FnV bit can be set for the 'catch all' external abort fault-status-code. This lets the CPU tell us that it doesn't know what the faulting virtual address is.

do_sea() checks for this:

|	if (esr & ESR_ELx_FnV)
|		siaddr = NULL;
|	else
|		siaddr  = (void __user *)addr;

If we can't know the address, there isn't much we can do.
(This check is really making sure we don't pass junk to user-space when FnV is set)

Quoted 1 line

should it be an problem ?

I'm not quite sure what your question is.

If the CPU doesn't tell us the address, we can't tell user-space what it is. The alternative is to upgrade to SIGKILL in that case.

If you see this instead of the address provided via firmware-first, there is a series to improve that here:
https://lore.kernel.org/linux-acpi/[email protected]/

(We skip this signal code of APEI promises it did all the work. This lets you take the signal from memory_failure() instead, which may have better information.)

If its the SIGKILL entries: these are for the translation table walk. There is no point telling user-space about corruption in its page tables as it can't do anything about it. The kernel's handling of this is to kill the process. (page tables make up a very small amount of memory, so this should be rarer than the regular 'external abort' case)

Thanks,

James

Quoted 11 lines

680         { do_sea,               SIGBUS,  BUS_OBJERR,    "synchronous external abort"    },
684         { do_sea,               SIGKILL, SI_KERNEL,     "level 0 (translation table walk)"      },
685         { do_sea,               SIGKILL, SI_KERNEL,     "level 1 (translation table walk)"      },
686         { do_sea,               SIGKILL, SI_KERNEL,     "level 2 (translation table walk)"      },
687         { do_sea,               SIGKILL, SI_KERNEL,     "level 3 (translation table walk)"      },
688         { do_sea,               SIGBUS,  BUS_OBJERR,    "synchronous parity or ECC error" },    // Reserved when RAS is implemented
692         { do_sea,               SIGKILL, SI_KERNEL,     "level 0 synchronous parity error (translation table walk)"     },      // Reserved when RAS is implemented
693         { do_sea,               SIGKILL, SI_KERNEL,     "level 1 synchronous parity error (translation table walk)"     },      // Reserved when RAS is implemented
694         { do_sea,               SIGKILL, SI_KERNEL,     "level 2 synchronous parity error (translation table walk)"     },      // Reserved when RAS is implemented
695         { do_sea,               SIGKILL, SI_KERNEL,     "level 3 synchronous parity error (translation table walk)"     },      // Reserved when RAS is implemented
696         { do_bad,               SIGKILL, SI_KERNEL,     "unknown 32"                    },

.

Quoted 8 lines · James Morse

If the CPU doesn't tell us the address, we can't tell user-space what it is. The alternative is to upgrade to SIGKILL in that case.

If you see this instead of the address provided via firmware-first, there is a series to improve that here:
https://lore.kernel.org/linux-acpi/[email protected]/

Quoted 2 lines

(We skip this signal code of APEI promises it did all the work. This lets you take the signal from memory_failure() instead, which may have better information.)

There may be an competition issue.
APEI run memory_failure() in an bottom half for memory errors. Then it may be not finished before here SEA handling end, and application process may back to run.

Quoted 9 lines · Xiaofei Tan

Quoted 8 lines · James Morse

If the CPU doesn't tell us the address, we can't tell user-space what it is. The alternative is to upgrade to SIGKILL in that case.

If you see this instead of the address provided via firmware-first, there is a series to improve that here:
https://lore.kernel.org/linux-acpi/[email protected]/

Quoted 6 lines

Quoted 2 lines

(We skip this signal code of APEI promises it did all the work. This lets you take the signal from memory_failure() instead, which may have better information.)

There may be an competition issue.
APEI run memory_failure() in an bottom half for memory errors. Then it may be not finished before here SEA handling end, and application process may back to run.

I'm not sure what you mean by 'bottom half', isn't this a softirq term?

With that series, it runs in process-context as task-work. memory_failure() needs to sleep, so it has to run in process-context.

Doing it as task-work means it runs before the thread returns to user-space.

If another thread in the same process accesses the affected memory, I'd expect to take a second external abort. If another process had the page mapped, it could access the affected memory, again taking an external abort.

These two could happen while the first CPU was in firmware generating the CPER records, so its not a race we can fix. It should be harmless, the recovery action is the same, its just the error counters that count more events than errors. If you actually see it happen, we can try and make it smaller...

Thanks,

James

.

Quoted 16 lines · James Morse

Quoted 15 lines · Xiaofei Tan

Quoted 10 lines · James Morse

If the CPU doesn't tell us the address, we can't tell user-space what it is. The alternative is to upgrade to SIGKILL in that case.

If you see this instead of the address provided via firmware-first, there is a series to improve that here:
https://lore.kernel.org/linux-acpi/[email protected]/

(We skip this signal code of APEI promises it did all the work. This lets you take the signal from memory_failure() instead, which may have better information.)

There may be an competition issue.
APEI run memory_failure() in an bottom half for memory errors. Then it may be not finished before here SEA handling end, and application process may back to run.

Quoted 5 lines

With that series, it runs in process-context as task-work. memory_failure() needs to sleep, so it has to run in process-context.

Doing it as task-work means it runs before the thread returns to user-space.

Sorry, i don't understand this. i thought the task-work need to reschedule, and current thread should have returned to user-space before it.

BTW, What context synchronous exception abort is? I thought it was process-context.

Because in_interrupt() return false called in do_sea().

Quoted 3 lines

If another thread in the same process accesses the affected memory, I'd expect to take a second external abort. If another process had the page mapped, it could access the affected memory, again taking an external abort.

Yes, it is hard to avoid another thread to access the affected memory. I just worry the same thread access it again.

Quoted 4 lines

These two could happen while the first CPU was in firmware generating the CPER records, so its not a race we can fix. It should be harmless, the recovery action is the same, its just the error counters that count more events than errors. If you actually see it happen, we can try and make it smaller...

Hmm, maybe this double SEA handling is an solution.

Quoted 24 lines · Xiaofei Tan

Quoted 22 lines · James Morse

Quoted 9 lines · Xiaofei Tan

Quoted 8 lines · James Morse

If the CPU doesn't tell us the address, we can't tell user-space what it is. The alternative is to upgrade to SIGKILL in that case.

If you see this instead of the address provided via firmware-first, there is a series to improve that here:
https://lore.kernel.org/linux-acpi/[email protected]/

Quoted 6 lines

Quoted 2 lines

(We skip this signal code of APEI promises it did all the work. This lets you take the signal from memory_failure() instead, which may have better information.)

There may be an competition issue.
APEI run memory_failure() in an bottom half for memory errors. Then it may be not finished before here SEA handling end, and application process may back to run.

With that series, it runs in process-context as task-work. memory_failure() needs to sleep, so it has to run in process-context.

Doing it as task-work means it runs before the thread returns to user-space.

Quoted 2 lines

Sorry, i don't understand this. i thought the task-work need to reschedule, and current thread should have returned to user-space before it.

ret_to_user has a loop around do_notify_resume(), if the _TIF_NOTIFY_RESUME flag is set and we call tracehook_notify_resume() which ends up in task_work_run()...

That TIF flag effectively prevents this thread returning to user-space until that task work has run.

Quoted 2 lines

BTW, What context synchronous exception abort is? I thought it was process-context.

It depends what you interrupted.
32bit had different CPU modes for different contexts, we don't have that in 64bit. Instead we mask asynchronous interrupts, and tinker with the preempt count to track the context. Synchronous exceptions can't be masked, so they happen in whatever context you were already in.
This means the exception handlers have to be be prepared for each eventuality. (which is why that code is starting to look complex)

Quoted 2 lines

Because in_interrupt() return false called in do_sea().

If you took the exception from EL0, or EL1 process context, yes. If you took the exception from an IRQ handler, in_interrupt() would return true.

Quoted 4 lines

Quoted 4 lines

If another thread in the same process accesses the affected memory, I'd expect to take a second external abort. If another process had the page mapped, it could access the affected memory, again taking an external abort.

Quoted 2 lines

Yes, it is hard to avoid another thread to access the affected memory. I just worry the same thread access it again.

This is the race that that series fixes.
It can't happen with mainline as the arch code unconditionally signals the affected process, which was the pre-RAS behaviour.

Quoted 4 lines

Quoted 4 lines

These two could happen while the first CPU was in firmware generating the CPER records, so its not a race we can fix. It should be harmless, the recovery action is the same, its just the error counters that count more events than errors. If you actually see it happen, we can try and make it smaller...

Quoted 1 line

Hmm, maybe this double SEA handling is an solution.

It assumes you get a second external-abort. We know this thread is affected, and will try and consume the error again if we restart it. We shouldn't restart it until we've given the recovery our best shot.
Letting it loose is a poor choice if you have any kind of threshold for error-counts. They may jump NR_CPUs at a time until every CPU is waiting in memory_failure()...

Thanks,

James

.

Quoted 10 lines · James Morse

Quoted 8 lines · Xiaofei Tan

Quoted 4 lines · James Morse

With that series, it runs in process-context as task-work. memory_failure() needs to sleep, so it has to run in process-context.

Doing it as task-work means it runs before the thread returns to user-space.

Sorry, i don't understand this. i thought the task-work need to reschedule, and current thread should have returned to user-space before it.

Quoted 5 lines

ret_to_user has a loop around do_notify_resume(), if the _TIF_NOTIFY_RESUME flag is set and we call tracehook_notify_resume() which ends up in task_work_run()...

That TIF flag effectively prevents this thread returning to user-space until that task work has run.

Got it. This function is great.

BTW, i have not found the place of setting the flag _TIF_NOTIFY_RESUME. Is it set by default for each thread?

Quoted 14 lines · Xiaofei Tan

Quoted 13 lines · James Morse

Quoted 8 lines · Xiaofei Tan

Quoted 4 lines · James Morse

With that series, it runs in process-context as task-work. memory_failure() needs to sleep, so it has to run in process-context.

Doing it as task-work means it runs before the thread returns to user-space.

Sorry, i don't understand this. i thought the task-work need to reschedule, and current thread should have returned to user-space before it.

ret_to_user has a loop around do_notify_resume(), if the _TIF_NOTIFY_RESUME flag is set and we call tracehook_notify_resume() which ends up in task_work_run()...

Quoted 4 lines

Quoted 2 lines

That TIF flag effectively prevents this thread returning to user-space until that task work has run.

Got it. This function is great.

I think PeterZ pointed me at it,

Quoted 2 lines

BTW, i have not found the place of setting the flag _TIF_NOTIFY_RESUME. Is it set by default for each thread?

With that series, APEI calls task_work_add(), which calls set_notify_resume() from include/linux/tracehook.h
| if (!test_and_set_tsk_thread_flag(task, TIF_NOTIFY_RESUME))

Thanks,

James

.