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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 16/02/2023 1:36 pm, Xenia Ragiadakou wrote:
> Hi Andrew,
>
> On 2/16/23 12:28, Andrew Cooper wrote:
>> On 13/02/2023 11:50 am, Xenia Ragiadakou wrote:
>>> diff --git a/xen/arch/x86/include/asm/hvm/vmx/vmx.h
>>> b/xen/arch/x86/include/asm/hvm/vmx/vmx.h
>>> index 234da4a7f4..97d6b810ec 100644
>>> --- a/xen/arch/x86/include/asm/hvm/vmx/vmx.h
>>> +++ b/xen/arch/x86/include/asm/hvm/vmx/vmx.h
>>> @@ -85,7 +85,7 @@ typedef enum {
>>>   void vmx_asm_vmexit_handler(struct cpu_user_regs);
>>>   void vmx_intr_assist(void);
>>>   void noreturn cf_check vmx_do_resume(void);
>>> -void vmx_vlapic_msr_changed(struct vcpu *v);
>>> +void cf_check vmx_vlapic_msr_changed(struct vcpu *v);
>>
>> Hi,
>>
>> I see this patch has been committed, but this public declaration should
>> deleted, and vmx_vlapic_msr_changed() made static now that it's only
>> referenced in vmx.c.
>
> It is also used in vmcs.c

Oh, so it is.  It just doesn't show up on the patch diff.

That use likely won't survive fixing the Intel APIC-V logic, but I guess
we're stuck with it for now.

Sorry for the noise.

>
>>
>> It needs a forward declaration in vmx.c because of its position relative
>> to the vmx_function_table, but that's fine - we've got plenty of other
>> examples like this.
>>
>> Could I talk you into doing an incremental fix?
>>
>> Alternatively, we could get better cleanup by forward declaring just
>> {vmx,svm}_function_table, then moving the tables to the very bottom of
>> {vmx,svm}.c at which point we can drop all the forward declarations.
>>
>> Oh top of that, I suspect we have other public function definitions
>> which can turn static, if you happen to spot any while doing this.
>
> Sure I could try to cleanup {svm,vmx}.c and the corresponding headers.

Thankyou.

~Andrew