Re: [PATCH V6 3/8] x86/entry: Move PUSH_AND_CLEAR_REGS out of error_entry()

[Lai Jiangshan]

On Thu, Apr 28, 2022 at 1:45 AM Borislav Petkov <bp@xxxxxxxxx> wrote:
>
> On Thu, Apr 21, 2022 at 10:10:50PM +0800, Lai Jiangshan wrote:
> > From: Lai Jiangshan <jiangshan.ljs@xxxxxxxxxxxx>
> >
> > The macro idtentry calls error_entry() unconditionally even on XENPV.
> > But the code XENPV needs in error_entry() is PUSH_AND_CLEAR_REGS only.
> > And error_entry() also calls sync_regs() which has to deal with the
> > case of XENPV via an extra branch so that it doesn't copy the pt_regs.
>
> What extra branch?
>
> Do you mean the
>
>         if (regs != eregs)
>
> test in sync_regs()?

Hello, Borislav

Yes.

>
> I'm confused. Are you, per chance, aiming to optimize XENPV here or
> what's up?


The branch in sync_regs() can be optimized out for the non-XENPV case
since XENPV doesn't call sync_regs() after patch5 which makes XENPV
not call error_entry().

The aim of this patch and most of the patchset is to make
error_entry() be able to be converted to C.  And XENPV cases can
also be optimized in the patchset although it is not the major main.

>
> > And PUSH_AND_CLEAR_REGS in error_entry() makes the stack not return to
> > its original place when the function returns, which means it is not
> > possible to convert it to a C function.
> >
> > Move PUSH_AND_CLEAR_REGS out of error_entry(), add a function to wrap
> > PUSH_AND_CLEAR_REGS and call it before error_entry().
> >
> > The new function call adds two instructions (CALL and RET) for every
> > interrupt or exception.
>
> Not only - it pushes all the regs in PUSH_AND_CLEAR_REGS too. I don't
> understand why that matters here? It was done in error_entry anyway.
>

Compared to the original code, adding the new function call adds two
instructions (CALL and RET) for every interrupt or exception.

PUSH_AND_CLEAR_REGS is not extra instructions added here.

Since this patch adds extra overhead (CALL and RET), the changelog
has to explain why it is worth it not just for converting ASM to C.

The explanation in the changelog is that it can be offsetted by later
reduced overhead.

Thanks
Lai

> --
> Regards/Gruss,
>     Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette