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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 02/04/2025 10:40 am, Jan Beulich wrote:
> 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>

Oh.  I was expecting there to be far more debate on this.

In which case I expect it will be easiest for this patch to go in first,
and supersede Denis' cleanup to __vmxon(), so as not to rework it twice
in quick succession.  (Sorry.)

>
>> 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 )".

Actually, doing that is still a nice diff to read.  I think I'll take
this approach.  I'll send out a v2 in just a moment.

~Andrew