Re: [PATCH 4/4] x86/vmx: cleanup vmx.h

[Jan Beulich <jbeulich@xxxxxxxx>]

On 17.02.2023 19:48, Xenia Ragiadakou wrote:
> Do not include the headers:
>   asm/i387.h
>   asm/hvm/trace.h
>   asm/processor.h
>   asm/regs.h
> because none of the declarations and macro definitions in them is used in
> this file. Sort alphabetically the rest of the headers.
> Fix build by including asm/i387.h in vmx.c, needed for 
> vcpu_restore_fpu_lazy().
> 
> Move the definition of GAS_VMX_OP just above the functions that use it and
> undefine it after its usage.
> 
> Move in vmcs.c the definitions of:
>   ept_sync_all()
>   __vmxoff()
>   __vmxon()
> because they are used only in this file. Take the opportunity to remove a
> trailing white space.

While the latter two are probably fine to be moved, I think the first one
wants to remain where it is, as new uses might appear.

> Move in vmx.c the definitions of:
>   pi_test_and_set_pir()
>   pi_test_pir()
>   pi_test_and_set_on()
>   pi_set_on()
>   pi_test_and_clear_on()
>   pi_test_on()
>   pi_get_pir()
>   pi_test_sn()
>   pi_set_sn()
>   pi_clear_sn()
>   vpid_sync_vcpu_gva()
> because they are used only in this file.

I'm not fully convinced of such movement. As a general remark: We typically
avoid "inline" for functions in .c files. Yet most if not all of the above
are pretty good candidates for being explicitly marked "inline".

> Move in vmx.c the declarations of:
>   ve_info_t
>   ept_qual_t
>   idt_or_gdt_instr_info_t
>   ldt_or_tr_instr_info_t
> because they are used only in this file.

I disagree with the movement of such types. Sooner or later they may want
using by vvmx.c as well. If you want to move them, then to a private header
under xen/arch/x86/hvm/vmx/.

Finally I think at least the individual groups of what is being moved or
adjusted want splitting into separate patches.

Jan