Re: [PATCH RFC] x86/vmx: Use asm goto() in _vmx_cpu_up()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 02.04.2025 01:34, Andrew Cooper wrote:
> With the new toolchain baseline, we can make use of asm goto() in certain
> places, and the VMXON invocation is one example.
> 
> This removes the logic to set up rc (including a fixup section where bactraces
> have no connection to the invoking function), the logic to decode it, and the
> default case which was dead but in a way the compiler couldn't prove
> previously.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> CC: dmkhn@xxxxxxxxx
> 
> RFC.  To be rebased over Denis' general cleanup.

LGTM. Can't this actually replace some of his cleanup? Judging from
base-commit: at the bottom this isn't based on his work. In which case:
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

> In principle, we can split fail into fail_valid and fail_invalid, allowing us
> to spot the VMfail("VMXON executed in VMX root operation") case from the
> pseduocode.  However, getting that involves a VMREAD of VM_INSTRUCTION_ERROR,
> and error handling in case there isn't a loaded VMCS, so I think the
> complexity is unwarranted in this case.

+1

> Bloat-o-meter:
>   add/remove: 0/0 grow/shrink: 1/1 up/down: 13/-32 (-19)
>   Function                                     old     new   delta
>   _vmx_cpu_up.cold                            2460    2473     +13
>   _vmx_cpu_up                                 1803    1771     -32
> 
> The if ( 0 ) isn't terribly nice, but it's the least bad option I could come
> up with.  It does allow the structure of the switch() to remain largely
> intact.

For the purpose of the diff here I agree. In a subsequent change we could then
still move the whole blob to the end of the function. Especially if some of
the static analysis tools didn't like the "if ( 0 )".

Jan