On Tue, Jun 04, 2024 at 08:47:22AM -0700, Dave Hansen wrote:
> On 6/4/24 08:32, Kirill A. Shutemov wrote:
> > What about the comment below?
> >
> > /*
> > * One possible reason for the failure is if kexec raced
> > * with memory conversion. In this case shared bit in
> > * page table got set (or not cleared) during
> > * shared<->private conversion, but the page is actually
> > * private. So this failure is not going to affect the
> > * kexec'ed kernel.
> > *
> > * The only thing one can do at this point on failure
> > * at this point is panic. In absence of better options,
> > * it is reasonable to proceed, hoping the failure is a
> > * benign shared bit mismatch due to the race.
> > *
> > * Also, even if the failure is real and the page cannot
> > * be touched as private, the kdump kernel will boot
> > * fine as it uses pre-reserved memory. What happens
> > * next depends on what the dumping process does and
> > * there's a reasonable chance to produce useful dump
> > * on crash.
> > *
> > * Regardless, the print leaves a trace in the log to
> > * give a clue for debug.
> > */
>
> It's rambling too much for my taste.
>
> Let's boil this down to what matters:
>
> 1. Failures to change encryption status here can lead a future kernel
> to touch shared memory with a private mapping
> 2. That causes an immediate unrecoverable guest shutdown (right?)
Right.
> 3. kdump kernels should not be affected since they have their own
> memory ranges and its encryption status is not being tweawked here
> 4. The pr_err() may help make some sense out of #2 when it happens
>
> I'm not sure the reason behind the failed conversion is important here.
The important part is that failure can be benign. It explains "can" in #1.
But okay.
> I wouldn't mention panic().
>
> We don't need to opine about what the next kernel might or might not do.
Is this any better?
/*
* If tdx_enc_status_changed() fails, it leaves memory
* in an unknown state. If the memory remains shared,
* it can result in an unrecoverable guest shutdown on
* the first accessed through a private mapping.
*
* The kdump kernel boot is not impacted as it uses
* a pre-reserved memory range that is always private.
* However, gathering crash information could lead to
* a crash if it accesses unconverted memory through
* a private mapping.
*
* pr_err() may assist in understanding such crashes.
*/
--
Kiryl Shutsemau / Kirill A. Shutemov
