Re: [PATCH v2] x86/vlapic: call vmx_vlapic_msr_changed through an hvm_function callback

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.02.2023 10:43, Xenia Ragiadakou wrote:
> APIC virtualization support is currently implemented only for Intel VT-x.
> To aid future work on separating AMD-V from Intel VT-x code, instead of
> calling directly vmx_vlapic_msr_changed() from common hvm code, add a stub
> to the hvm_function_table, named update_vlapic_mode, and create a wrapper
> function, called hvm_update_vlapic_mode(), to be used by common hvm code.
> 
> After the change above, do not include header asm/hvm/vmx/vmx.h as it is
> not required anymore and resolve subsequent build errors for implicit
> declaration of functions ‘TRACE_2_LONG_3D’ and ‘TRC_PAR_LONG’ by including
> missing asm/hvm/trace.h header.
> 
> No functional change intended.
> 
> Signed-off-by: Xenia Ragiadakou <burzalodowa@xxxxxxxxx>
> ---
> 
> Changes in v2:
>   - rename set_virtual_apic_mode to update_vlapic_mode, suggested by Andrew
>   - in hvm_update_vlapic_mode(), call the stub only if available, otherwise
>     a BUG() will be triggered every time an svm guest writes the APIC_BASE 
> MSR,
>     bug reported by Andrew
>   - initialize the stub for vmx unconditionally to maintain current behavior
>     since no functional change is intended, bug reported by Andrew (here, I
>     decided to place the initialization in start_vmx to be closer to the
>     initializations of the other stubs that are relevant to apic 
> virtualization)

I disagree with this - unconditional hooks are better put in place right
in vmx_function_table's initializer.

Also now that you use the function as a callback, vmx_vlapic_msr_changed()
will need to have cf_check added (on both declaration and definition, albeit
I keep forgetting why putting it on just the declaration isn't sufficient; I
guess a short comment to that effect next to cf_check's definition in
compiler.h would help).

Jan