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

[Xenia Ragiadakou <burzalodowa@xxxxxxxxx>]

On 2/21/23 13:23, Jan Beulich wrote:
> 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.
Ok I will leave it where it is.

> > 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".
I could create a private header.

> > 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/.
Ok will do.

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

> Jan
--
Xenia